Fix indexing JArray / faster IList indexing (#798)

* Fix indexing JArray / faster IList indexing
* improve CLR interop IList access performance

Jint.Tests/Runtime/EngineTests.cs

@@ -2845,10 +2845,9 @@ x.test = {
         [Fact]
         public void ShouldOverrideDefaultTypeConverter()
         {
-            var engine = new Engine
-            {
-                ClrTypeConverter = new TestTypeConverter()
-            };
+            var engine = new Engine(options => options
+                .SetTypeConverter(e => new TestTypeConverter())
+            );
             Assert.IsType<TestTypeConverter>(engine.ClrTypeConverter);
             engine.SetValue("x", new Testificate());
             Assert.Throws<JavaScriptException>(() => engine.Execute("c.Name"));

Jint.Tests/Runtime/InteropTests.cs

@@ -2278,5 +2278,14 @@ namespace Jint.Tests.Runtime
             _engine.SetValue("o", o);
             Assert.True(_engine.Execute("return o.name == 'test-name'").GetCompletionValue().AsBoolean());
         }
+
+        [Fact]
+        public void AccessingJArrayViaIntegerIndexShouldWork()
+        {
+            var o = new JArray("item1", "item2");
+            _engine.SetValue("o", o);
+            Assert.True(_engine.Execute("return o[0] == 'item1'").GetCompletionValue().AsBoolean());
+            Assert.True(_engine.Execute("return o[1] == 'item2'").GetCompletionValue().AsBoolean());
+        }
     }
 }

Jint/Engine.cs

@@ -78,7 +78,7 @@ namespace Jint
         internal readonly ArgumentsInstancePool _argumentsInstancePool;
         internal readonly JsValueArrayPool _jsValueArrayPool;
 
-        public ITypeConverter ClrTypeConverter { get; set; }
+        public ITypeConverter ClrTypeConverter { get; internal set; }
 
         // cache of types used when resolving CLR type names
         internal readonly Dictionary<string, Type> TypeCache = new Dictionary<string, Type>();
@@ -209,11 +209,10 @@ namespace Jint
                     (thisObj, arguments) => new NamespaceReference(this, TypeConverter.ToString(arguments.At(0)))), PropertyFlag.AllForbidden));
             }
 
-            ClrTypeConverter = new DefaultTypeConverter(this);
-
             Options.Apply(this);
+
+            ClrTypeConverter ??= new DefaultTypeConverter(this);
         }
-    
 
         internal LexicalEnvironment GlobalEnvironment { get; }
         public GlobalObject Global { get; }

Jint/Native/Array/ArrayInstance.cs

@@ -827,6 +827,8 @@ namespace Jint.Native.Array
 
         public override uint Length => GetLength();
 
+        internal override bool IsIntegerIndexedArray => true;
+
         public JsValue this[uint index]
         {
             get

Jint/Native/JsValue.cs

@@ -128,6 +128,8 @@ namespace Jint.Native
             return _type == InternalTypes.Null;
         }
 
+        internal virtual bool IsIntegerIndexedArray => false;
+
         [Pure]
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public bool IsSymbol()

Jint/Native/Object/ObjectInstance.cs

@@ -1095,6 +1095,7 @@ namespace Jint.Native.Object
                                            && lengthValue.IsNumber()
                                            && ((JsNumber) lengthValue)._value >= 0;
 
+        internal override bool IsIntegerIndexedArray => false;
 
         public virtual uint Length => (uint) TypeConverter.ToLength(Get(CommonProperties.Length));
 

Jint/Options.cs

@@ -90,6 +90,14 @@ namespace Jint
             return this;
         }
 
+        /// <summary>
+        /// Sets the type converter to use.
+        /// </summary>
+        public Options SetTypeConverter(Func<Engine, ITypeConverter> typeConverterFactory)
+        {
+            _configurations.Add(engine => engine.ClrTypeConverter = typeConverterFactory(engine));
+            return this;
+        }
 
         /// <summary>
         /// Registers a delegate that is called when CLR members are invoked. This allows

Jint/Runtime/Descriptors/Specialized/FieldInfoDescriptor.cs

@@ -4,7 +4,7 @@ using Jint.Native;
 
 namespace Jint.Runtime.Descriptors.Specialized
 {
-    public sealed class FieldInfoDescriptor : PropertyDescriptor
+    internal sealed class FieldInfoDescriptor : PropertyDescriptor
     {
         private readonly Engine _engine;
         private readonly FieldInfo _fieldInfo;

Jint/Runtime/Descriptors/Specialized/GetSetPropertyDescriptor.cs

@@ -3,7 +3,7 @@ using Jint.Native.Function;
 
 namespace Jint.Runtime.Descriptors.Specialized
 {
-    public sealed class GetSetPropertyDescriptor : PropertyDescriptor
+    internal sealed class GetSetPropertyDescriptor : PropertyDescriptor
     {
         private JsValue _get;
         private JsValue _set;

Jint/Runtime/Descriptors/Specialized/IndexDescriptor.cs

@@ -1,11 +1,13 @@
 using System;
+using System.Collections;
+using System.Collections.Generic;
 using System.Globalization;
 using System.Reflection;
 using Jint.Native;
 
 namespace Jint.Runtime.Descriptors.Specialized
 {
-    public sealed class IndexDescriptor : PropertyDescriptor
+    internal sealed class IndexDescriptor : PropertyDescriptor
     {
         private readonly Engine _engine;
         private readonly object _key;
@@ -13,35 +15,56 @@ namespace Jint.Runtime.Descriptors.Specialized
         private readonly PropertyInfo _indexer;
         private readonly MethodInfo _containsKey;
 
-        public IndexDescriptor(Engine engine, Type targetType, string key, object target)
+        private static readonly PropertyInfo _iListIndexer = typeof(IList).GetProperty("Item");
+
+        internal IndexDescriptor(Engine engine, object target, PropertyInfo indexer, MethodInfo containsKey, object key)
             : base(PropertyFlag.Enumerable | PropertyFlag.CustomJsValue)
         {
             _engine = engine;
             _target = target;
-
-            if (!TryFindIndexer(engine, targetType, key, out _indexer, out _containsKey, out _key))
-            {
-                ExceptionHelper.ThrowArgumentException("invalid indexing configuration, target indexer not found");
-            }
-
+            _indexer = indexer;
+            _containsKey = containsKey;
+            _key = key;
             Writable = engine.Options._IsClrWriteAllowed;
         }
 
-        public IndexDescriptor(Engine engine, string key, object item)
-            : this(engine, item.GetType(), key, item)
-        {
-        }
-
         internal static bool TryFindIndexer(
             Engine engine,
             Type targetType,
             string propertyName,
-            out PropertyInfo indexerProperty,
-            out MethodInfo containsKeyMethod,
-            out object indexerKey)
+            out Func<object, IndexDescriptor> factory)
         {
-            // get all instance indexers with exactly 1 argument
             var paramTypeArray = new Type[1];
+            Func<object, IndexDescriptor> ComposeIndexerFactory(PropertyInfo candidate, Type paramType)
+            {
+                if (engine.ClrTypeConverter.TryConvert(propertyName, paramType, CultureInfo.InvariantCulture, out var key))
+                {
+                    // the key can be converted for this indexer
+                    var indexerProperty = candidate;
+                    // get contains key method to avoid index exception being thrown in dictionaries
+                    paramTypeArray[0] = paramType;
+                    var containsKeyMethod = targetType.GetMethod(nameof(IDictionary<string,string>.ContainsKey), paramTypeArray);
+                    if (containsKeyMethod is null)
+                    {
+                        paramTypeArray[0] = typeof(object);
+                        containsKeyMethod = targetType.GetMethod(nameof(IDictionary.Contains), paramTypeArray);
+                    }
+                    return (target) => new IndexDescriptor(engine, target, indexerProperty, containsKeyMethod, key);
+                }
+
+                // the key type doesn't work for this indexer
+                return null;
+            }
+
+            // default indexer wins
+            if (typeof(IList).IsAssignableFrom(targetType))
+            {
+                factory = ComposeIndexerFactory(_iListIndexer, typeof(int));
+                if (factory != null)
+                {
+                    return true;
+                }
+            }
 
             // try to find first indexer having either public getter or setter with matching argument type
             foreach (var candidate in targetType.GetProperties())
@@ -55,21 +78,15 @@ namespace Jint.Runtime.Descriptors.Specialized
                 if (candidate.GetGetMethod() != null || candidate.GetSetMethod() != null)
                 {
                     var paramType = indexParameters[0].ParameterType;
-
-                    if (engine.ClrTypeConverter.TryConvert(propertyName, paramType, CultureInfo.InvariantCulture, out indexerKey))
+                    factory = ComposeIndexerFactory(candidate, paramType);
+                    if (factory != null)
                     {
-                        indexerProperty = candidate;
-                        // get contains key method to avoid index exception being thrown in dictionaries
-                        paramTypeArray[0] = paramType;
-                        containsKeyMethod = targetType.GetMethod("ContainsKey", paramTypeArray);
                         return true;
                     }
                 }
             }
 
-            indexerProperty = default;
-            containsKeyMethod = default;
-            indexerKey = default;
+            factory = default;
             return false;
         }
 

Jint/Runtime/Descriptors/Specialized/PropertyInfoDescriptor.cs

@@ -4,7 +4,7 @@ using Jint.Native;
 
 namespace Jint.Runtime.Descriptors.Specialized
 {
-    public sealed class PropertyInfoDescriptor : PropertyDescriptor
+    internal sealed class PropertyInfoDescriptor : PropertyDescriptor
     {
         private readonly Engine _engine;
         private readonly PropertyInfo _propertyInfo;

Jint/Runtime/Interop/ObjectWrapper.cs

@@ -30,6 +30,7 @@ namespace Jint.Runtime.Interop
                     return;
                 }
                 IsArrayLike = true;
+                IsIntegerIndexedArray = typeof(IList).IsAssignableFrom(type);
 
                 var functionInstance = new ClrFunctionInstance(engine, "length", (thisObj, arguments) => JsNumber.Create((int) lengthProperty.GetValue(obj)));
                 var descriptor = new GetSetPropertyDescriptor(functionInstance, Undefined, PropertyFlag.Configurable);
@@ -65,6 +66,8 @@ namespace Jint.Runtime.Interop
 
         public override bool IsArrayLike { get; }
 
+        internal override bool IsIntegerIndexedArray { get; }
+
         public override bool Set(JsValue property, JsValue value, JsValue receiver)
         {
             if (!CanPut(property))
@@ -91,6 +94,12 @@ namespace Jint.Runtime.Interop
                 return Undefined;
             }
 
+            if (property.IsInteger() && Target is IList list)
+            {
+                var index = (int) ((JsNumber) property)._value;
+                return (uint) index < list.Count ? FromObject(_engine, list[index]) : Undefined;
+            }
+
             return base.Get(property, receiver);
         }
 
@@ -259,15 +268,16 @@ namespace Jint.Runtime.Interop
 
                 if (methods?.Count > 0)
                 {
-                    return (engine, target) => new PropertyDescriptor(new MethodInfoFunctionInstance(engine, methods.ToArray()), PropertyFlag.OnlyEnumerable);
+                    var array = methods.ToArray();
+                    return (engine, target) => new PropertyDescriptor(new MethodInfoFunctionInstance(engine, array), PropertyFlag.OnlyEnumerable);
                 }
 
             }
 
             // if no methods are found check if target implemented indexing
-            if (IndexDescriptor.TryFindIndexer(_engine, type, propertyName, out _, out _, out _))
+            if (IndexDescriptor.TryFindIndexer(_engine, type, propertyName, out var indexerFactory))
             {
-                return (engine, target) => new IndexDescriptor(engine, propertyName, target);
+                return (engine, target) => indexerFactory(target);
             }
 
             // try to find a single explicit property implementation
@@ -305,15 +315,16 @@ namespace Jint.Runtime.Interop
 
             if (explicitMethods?.Count > 0)
             {
-                return (engine, target) => new PropertyDescriptor(new MethodInfoFunctionInstance(engine, explicitMethods.ToArray()), PropertyFlag.OnlyEnumerable);
+                var array = explicitMethods.ToArray();
+                return (engine, target) => new PropertyDescriptor(new MethodInfoFunctionInstance(engine, array), PropertyFlag.OnlyEnumerable);
             }
 
             // try to find explicit indexer implementations
-            foreach (Type interfaceType in type.GetInterfaces())
+            foreach (var interfaceType in type.GetInterfaces())
             {
-                if (IndexDescriptor.TryFindIndexer(_engine, interfaceType, propertyName, out _, out _, out _))
+                if (IndexDescriptor.TryFindIndexer(_engine, interfaceType, propertyName, out var interfaceIndexerFactory))
                 {
-                    return (engine, target) => new IndexDescriptor(engine, interfaceType, propertyName, target);
+                    return (engine, target) => interfaceIndexerFactory(target);
                 }
             }
 

Jint/Runtime/Interpreter/Expressions/JintMemberExpression.cs

@@ -85,7 +85,13 @@ namespace Jint.Runtime.Interpreter.Expressions
 
             var property = _determinedProperty ?? _propertyExpression.GetValue();
             TypeConverter.CheckObjectCoercible(_engine, baseValue, (MemberExpression) _expression, _determinedProperty?.ToString() ?? baseReferenceName);
-            return _engine._referencePool.Rent(baseValue,  TypeConverter.ToPropertyKey(property), isStrictModeCode);
+
+            // only convert if necessary
+            var propertyKey = property.IsInteger() && baseValue.IsIntegerIndexedArray
+                ? property
+                : TypeConverter.ToPropertyKey(property);
+
+            return _engine._referencePool.Rent(baseValue, propertyKey, isStrictModeCode);
         }
     }
 }