Improve and cleanup iterators a bit (#1422)

Jint/Engine.cs

@@ -1,5 +1,4 @@
-using System.Reflection;
-using System.Runtime.CompilerServices;
+using System.Runtime.CompilerServices;
 using Esprima;
 using Esprima.Ast;
 using Jint.Native;
@@ -1436,7 +1435,7 @@ namespace Jint
             _objectWrapperCache.Clear();
 #else
             // we can expect that reflection is OK as we've been generating object wrappers already
-            var clearMethod = _objectWrapperCache.GetType().GetMethod("Clear", BindingFlags.Instance | BindingFlags.NonPublic);
+            var clearMethod = _objectWrapperCache.GetType().GetMethod("Clear", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);
             clearMethod?.Invoke(_objectWrapperCache, Array.Empty<object>());
 #endif
         }

Jint/Native/Array/ArrayIteratorPrototype.cs

@@ -38,14 +38,51 @@ internal sealed class ArrayIteratorPrototype : IteratorPrototype
 
     internal IteratorInstance Construct(ObjectInstance array, ArrayIteratorType kind)
     {
-        var instance = new ArrayLikeIterator(Engine, array, kind)
-        {
-            _prototype = this
-        };
+        IteratorInstance instance = array is JsArray jsArray
+            ? new ArrayIterator(Engine, jsArray, kind)  { _prototype = this }
+            : new ArrayLikeIterator(Engine, array, kind) { _prototype = this };
 
         return instance;
     }
 
+    private sealed class ArrayIterator : IteratorInstance
+    {
+        private readonly ArrayIteratorType _kind;
+        private readonly JsArray _array;
+        private uint _position;
+        private bool _closed;
+
+        public ArrayIterator(Engine engine, JsArray array, ArrayIteratorType kind) : base(engine)
+        {
+            _kind = kind;
+            _array = array;
+            _position = 0;
+        }
+
+        public override bool TryIteratorStep(out ObjectInstance nextItem)
+        {
+            var len = _array.Length;
+            var position = _position;
+            if (!_closed && position < len)
+            {
+                _array.TryGetValue(position, out var value);
+                nextItem = _kind switch
+                {
+                    ArrayIteratorType.Key => IteratorResult.CreateValueIteratorPosition(_engine, JsNumber.Create(position)),
+                    ArrayIteratorType.Value => IteratorResult.CreateValueIteratorPosition(_engine, value),
+                    _ => IteratorResult.CreateKeyValueIteratorPosition(_engine, JsNumber.Create(position), value)
+                };
+
+                _position++;
+                return true;
+            }
+
+            _closed = true;
+            nextItem = IteratorResult.CreateKeyValueIteratorPosition(_engine);
+            return false;
+        }
+    }
+
     private sealed class ArrayLikeIterator : IteratorInstance
     {
         private readonly ArrayIteratorType _kind;
@@ -93,18 +130,12 @@ internal sealed class ArrayIteratorPrototype : IteratorPrototype
                 else
                 {
                     _operations!.TryGetValue(_position, out var value);
-                    if (_kind == ArrayIteratorType.Key)
-                    {
-                        nextItem = IteratorResult.CreateValueIteratorPosition(_engine, JsNumber.Create(_position));
-                    }
-                    else if (_kind == ArrayIteratorType.Value)
-                    {
-                        nextItem = IteratorResult.CreateValueIteratorPosition(_engine, value);
-                    }
-                    else
+                    nextItem = _kind switch
                     {
-                        nextItem = IteratorResult.CreateKeyValueIteratorPosition(_engine, JsNumber.Create(_position), value);
-                    }
+                        ArrayIteratorType.Key => IteratorResult.CreateValueIteratorPosition(_engine, JsNumber.Create(_position)),
+                        ArrayIteratorType.Value => IteratorResult.CreateValueIteratorPosition(_engine, value),
+                        _ => IteratorResult.CreateKeyValueIteratorPosition(_engine, JsNumber.Create(_position), value)
+                    };
                 }
 
                 _position++;

Jint/Native/Iterator/IteratorInstance.cs

@@ -5,22 +5,12 @@ using Jint.Runtime;
 
 namespace Jint.Native.Iterator
 {
-    internal class IteratorInstance : ObjectInstance
+    internal abstract class IteratorInstance : ObjectInstance
     {
-        private readonly IEnumerator<JsValue> _enumerable;
-
-        protected IteratorInstance(Engine engine)
-            : this(engine, Enumerable.Empty<JsValue>())
-        {
-        }
-
-        public IteratorInstance(
-            Engine engine,
-            IEnumerable<JsValue> enumerable) : base(engine)
+        protected IteratorInstance(Engine engine) : base(engine)
         {
-            _enumerable = enumerable.GetEnumerator();
             _prototype = engine.Realm.Intrinsics.ArrayIteratorPrototype;
-       }
+        }
 
         public override object ToObject()
         {
@@ -28,17 +18,7 @@ namespace Jint.Native.Iterator
             return null;
         }
 
-        public virtual bool TryIteratorStep(out ObjectInstance nextItem)
-        {
-            if (_enumerable.MoveNext())
-            {
-                nextItem = IteratorResult.CreateValueIteratorPosition(_engine, _enumerable.Current);
-                return true;
-            }
-
-            nextItem = IteratorResult.CreateValueIteratorPosition(_engine, done: JsBoolean.True);
-            return false;
-        }
+        public abstract bool TryIteratorStep(out ObjectInstance nextItem);
 
         public virtual void Close(CompletionType completion)
         {
@@ -65,6 +45,7 @@ namespace Jint.Native.Iterator
                     ExceptionHelper.ThrowTypeError(target.Engine.Realm);
                     return;
                 }
+
                 _nextMethod = callable;
             }
 
@@ -119,6 +100,7 @@ namespace Jint.Native.Iterator
                         throw;
                     }
                 }
+
                 if (completion != CompletionType.Throw && !innerResult.IsObject())
                 {
                     ExceptionHelper.ThrowTypeError(_target.Engine.Realm, "Iterator returned non-object");
@@ -206,5 +188,27 @@ namespace Jint.Native.Iterator
                 return true;
             }
         }
+
+        internal sealed class EnumerableIterator : IteratorInstance
+        {
+            private readonly IEnumerator<JsValue> _enumerable;
+
+            public EnumerableIterator(Engine engine, IEnumerable<JsValue> obj) : base(engine)
+            {
+                _enumerable = obj.GetEnumerator();
+            }
+
+            public override bool TryIteratorStep(out ObjectInstance nextItem)
+            {
+                if (_enumerable.MoveNext())
+                {
+                    nextItem = IteratorResult.CreateValueIteratorPosition(_engine, _enumerable.Current);
+                    return true;
+                }
+
+                nextItem = IteratorResult.CreateValueIteratorPosition(_engine, done: JsBoolean.True);
+                return false;
+            }
+        }
     }
 }

Jint/Native/Iterator/IteratorResult.cs

@@ -6,7 +6,7 @@ namespace Jint.Native.Iterator;
 /// <summary>
 /// https://tc39.es/ecma262/#sec-createiterresultobject
 /// </summary>
-internal class IteratorResult : ObjectInstance
+internal sealed class IteratorResult : ObjectInstance
 {
     private readonly JsValue _value;
     private readonly JsBoolean _done;
@@ -24,7 +24,7 @@ internal class IteratorResult : ObjectInstance
 
     public static IteratorResult CreateKeyValueIteratorPosition(Engine engine, JsValue? key = null, JsValue? value = null)
     {
-        var done = ReferenceEquals(null, key) && ReferenceEquals(null, value);
+        var done = key is null && value is null;
         var array = done ? Undefined : new JsArray(engine, new[] { key!, value! });
 
         return new IteratorResult(engine, array, JsBoolean.Create(done));

Jint/Native/Map/MapIteratorPrototype.cs

@@ -47,7 +47,7 @@ internal sealed class MapIteratorPrototype : IteratorPrototype
 
     internal IteratorInstance ConstructKeyIterator(MapInstance map)
     {
-        var instance = new IteratorInstance(Engine, map._map.Keys)
+        var instance = new IteratorInstance.EnumerableIterator(Engine, map._map.Keys)
         {
             _prototype = this
         };
@@ -57,7 +57,7 @@ internal sealed class MapIteratorPrototype : IteratorPrototype
 
     internal IteratorInstance ConstructValueIterator(MapInstance map)
     {
-        var instance = new IteratorInstance(Engine, map._map.Values)
+        var instance = new IteratorInstance.EnumerableIterator(Engine, map._map.Values)
         {
             _prototype = this
         };

Jint/Native/Object/ObjectInstance.cs

@@ -1402,6 +1402,36 @@ namespace Jint.Native.Object
             return false;
         }
 
+        internal IEnumerable<JsValue> GetKeys()
+        {
+            var visited = new HashSet<JsValue>();
+            foreach (var key in GetOwnPropertyKeys(Types.String))
+            {
+                var desc = GetOwnProperty(key);
+                if (desc != PropertyDescriptor.Undefined)
+                {
+                    visited.Add(key);
+                    if (desc.Enumerable)
+                    {
+                        yield return key;
+                    }
+                }
+            }
+
+            if (Prototype is null)
+            {
+                yield break;
+            }
+
+            foreach (var protoKey in Prototype.GetKeys())
+            {
+                if (!visited.Contains(protoKey))
+                {
+                    yield return protoKey;
+                }
+            }
+        }
+
         public override string ToString()
         {
             return TypeConverter.ToString(this);

Jint/Runtime/Interpreter/Statements/JintForInForOfStatement.cs

@@ -2,8 +2,6 @@ using System.Diagnostics.CodeAnalysis;
 using Esprima.Ast;
 using Jint.Native;
 using Jint.Native.Iterator;
-using Jint.Native.Object;
-using Jint.Runtime.Descriptors;
 using Jint.Runtime.Environments;
 using Jint.Runtime.Interpreter.Expressions;
 using Jint.Runtime.References;
@@ -131,7 +129,7 @@ namespace Jint.Runtime.Interpreter.Statements
                 }
 
                 var obj = TypeConverter.ToObject(engine.Realm, exprValue);
-                result = new ObjectKeyVisitor(engine, obj);
+                result = new IteratorInstance.EnumerableIterator(engine, obj.GetKeys());
             }
             else
             {
@@ -362,43 +360,5 @@ namespace Jint.Runtime.Interpreter.Statements
             Iterate,
             AsyncIterate
         }
-
-        private sealed class ObjectKeyVisitor : IteratorInstance
-        {
-            public ObjectKeyVisitor(Engine engine, ObjectInstance obj)
-                : base(engine, CreateEnumerator(obj))
-            {
-            }
-
-            private static IEnumerable<JsValue> CreateEnumerator(ObjectInstance obj)
-            {
-                var visited = new HashSet<JsValue>();
-                foreach (var key in obj.GetOwnPropertyKeys(Types.String))
-                {
-                    var desc = obj.GetOwnProperty(key);
-                    if (desc != PropertyDescriptor.Undefined)
-                    {
-                        visited.Add(key);
-                        if (desc.Enumerable)
-                        {
-                            yield return key;
-                        }
-                    }
-                }
-
-                if (obj.Prototype is null)
-                {
-                    yield break;
-                }
-
-                foreach (var protoKey in CreateEnumerator(obj.Prototype))
-                {
-                    if (!visited.Contains(protoKey))
-                    {
-                        yield return protoKey;
-                    }
-                }
-            }
-        }
     }
 }