Improve TypeDescriptor analysis logic (#1788)

* use TypeDescriptor when composing IndexerAccessor

Directory.Packages.props

@@ -7,6 +7,7 @@
     <PackageVersion Include="BenchmarkDotNet" Version="0.13.12" />
     <PackageVersion Include="BenchmarkDotNet.TestAdapter" Version="0.13.12" />
     <PackageVersion Include="Esprima" Version="3.0.4" />
+    <PackageVersion Include="FluentAssertions" Version="6.12.0" />
     <PackageVersion Include="Flurl.Http.Signed" Version="3.2.4" />
     <PackageVersion Include="Jurassic" Version="3.2.7" />
     <PackageVersion Include="Meziantou.Analyzer" Version="2.0.141" />

Jint.Tests/Jint.Tests.csproj

@@ -24,6 +24,7 @@
   </ItemGroup>
 
   <ItemGroup>
+    <PackageReference Include="FluentAssertions" />
     <PackageReference Include="Flurl.Http.Signed" />
     <PackageReference Include="Microsoft.Extensions.DependencyInjection" />
     <PackageReference Include="Microsoft.NET.Test.Sdk" />

Jint.Tests/Runtime/TypeDescriptorTests.cs

@@ -0,0 +1,39 @@
+using System.Collections;
+using FluentAssertions;
+using Jint.Runtime.Interop;
+
+namespace Jint.Tests.Runtime;
+
+public class TypeDescriptorTests
+{
+    [Fact]
+    public void AnalyzesBclCollectionTypesCorrectly()
+    {
+        AssertInformation(typeof(ICollection), isArrayLike: true, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: true);
+
+        AssertInformation(typeof(IList), isArrayLike: true, iterable: true, isIntegerIndexedArray: true, shouldHaveLength: true);
+        AssertInformation(typeof(ArrayList), isArrayLike: true, iterable: true, isIntegerIndexedArray: true, shouldHaveLength: true);
+        AssertInformation(typeof(string[]), isArrayLike: true, iterable: true, isIntegerIndexedArray: true, shouldHaveLength: true);
+        AssertInformation(typeof(List<string>), isArrayLike: true, iterable: true, isIntegerIndexedArray: true, shouldHaveLength: true);
+        AssertInformation(typeof(IList<string>), isArrayLike: true, iterable: true, isIntegerIndexedArray: true, shouldHaveLength: true);
+
+        AssertInformation(typeof(Dictionary<string, object>), isArrayLike: false, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: false);
+        AssertInformation(typeof(IDictionary<string, object>), isArrayLike: false, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: false);
+        AssertInformation(typeof(Dictionary<decimal, object>), isArrayLike: false, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: false);
+        AssertInformation(typeof(IDictionary<decimal, object>), isArrayLike: false, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: false);
+
+        AssertInformation(typeof(ISet<string>), isArrayLike: true, iterable: true, isIntegerIndexedArray: false, shouldHaveLength: true);
+    }
+
+    private static void AssertInformation(Type type, bool isArrayLike, bool iterable, bool isIntegerIndexedArray, bool shouldHaveLength)
+    {
+        var descriptor = TypeDescriptor.Get(type);
+        descriptor.IsArrayLike.Should().Be(isArrayLike);
+        descriptor.Iterable.Should().Be(iterable);
+        descriptor.IsIntegerIndexed.Should().Be(isIntegerIndexedArray);
+        if (shouldHaveLength)
+        {
+            descriptor.LengthProperty.Should().NotBeNull();
+        }
+    }
+}

Jint/Runtime/Interop/ObjectWrapper.cs

@@ -46,7 +46,7 @@ namespace Jint.Runtime.Interop
 
         internal override bool HasOriginalIterator => IsArrayLike;
 
-        internal override bool IsIntegerIndexedArray => _typeDescriptor.IsIntegerIndexedArray;
+        internal override bool IsIntegerIndexedArray => _typeDescriptor.IsIntegerIndexed;
 
         public override bool Set(JsValue property, JsValue value, JsValue receiver)
         {

Jint/Runtime/Interop/Reflection/IndexerAccessor.cs

@@ -19,8 +19,6 @@ namespace Jint.Runtime.Interop.Reflection
         private readonly MethodInfo? _setter;
         private readonly MethodInfo? _containsKey;
 
-        private static readonly PropertyInfo _iListIndexer = typeof(IList).GetProperty("Item")!;
-
         private IndexerAccessor(PropertyInfo indexer, MethodInfo? containsKey, object key)
             : base(indexer.PropertyType, key)
         {
@@ -87,12 +85,13 @@ namespace Jint.Runtime.Interop.Reflection
             var filter = new Func<MemberInfo, bool>(m => engine.Options.Interop.TypeResolver.Filter(engine, m));
 
             // default indexer wins
-            if (typeof(IList).IsAssignableFrom(targetType) && filter(_iListIndexer))
+            var descriptor = TypeDescriptor.Get(targetType);
+            if (descriptor.IntegerIndexerProperty is not null && filter(descriptor.IntegerIndexerProperty))
             {
-                indexerAccessor = ComposeIndexerFactory(_iListIndexer, typeof(int));
+                indexerAccessor = ComposeIndexerFactory(descriptor.IntegerIndexerProperty, typeof(int));
                 if (indexerAccessor != null)
                 {
-                    indexer = _iListIndexer;
+                    indexer = descriptor.IntegerIndexerProperty;
                     return true;
                 }
             }

Jint/Runtime/Interop/TypeDescriptor.cs

@@ -12,6 +12,10 @@ namespace Jint.Runtime.Interop
     internal sealed class TypeDescriptor
     {
         private static readonly ConcurrentDictionary<Type, TypeDescriptor> _cache = new();
+
+        private static readonly Type _listType = typeof(IList);
+        private static readonly PropertyInfo _listIndexer = typeof(IList).GetProperty("Item")!;
+
         private static readonly Type _genericDictionaryType = typeof(IDictionary<,>);
         private static readonly Type _stringType = typeof(string);
 
@@ -21,39 +25,47 @@ namespace Jint.Runtime.Interop
         private readonly Type? _valueType;
 
         private TypeDescriptor(
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type)
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)]
+            Type type)
         {
-            // check if object has any generic dictionary signature that accepts string as key
-            foreach (var i in type.GetInterfaces())
-            {
-                if (i.IsGenericType
-                    && i.GetGenericTypeDefinition() == _genericDictionaryType
-                    && i.GenericTypeArguments[0] == _stringType)
-                {
-                    _tryGetValueMethod = i.GetMethod("TryGetValue");
-                    _removeMethod = i.GetMethod("Remove");
-                    _keysAccessor = i.GetProperty("Keys");
-                    _valueType = i.GenericTypeArguments[1];
-                    break;
-                }
-            }
-
-            IsDictionary = _tryGetValueMethod is not null || typeof(IDictionary).IsAssignableFrom(type);
+            Analyze(
+                type,
+                out var isCollection,
+                out var isEnumerable,
+                out var isDictionary,
+                out _tryGetValueMethod,
+                out _removeMethod,
+                out _keysAccessor,
+                out _valueType,
+                out var lengthProperty,
+                out var integerIndexer);
+
+            IntegerIndexerProperty = integerIndexer;
+            IsDictionary = _tryGetValueMethod is not null || isDictionary;
 
             // dictionaries are considered normal-object-like
-            IsArrayLike = !IsDictionary && DetermineIfObjectIsArrayLikeClrCollection(type);
+            IsArrayLike = !IsDictionary && isCollection;
 
-            IsEnumerable = typeof(IEnumerable).IsAssignableFrom(type);
+            IsEnumerable = isEnumerable;
 
             if (IsArrayLike)
             {
-                LengthProperty = type.GetProperty("Count") ?? type.GetProperty("Length");
-                IsIntegerIndexedArray = typeof(IList).IsAssignableFrom(type);
+                LengthProperty = lengthProperty;
             }
         }
 
         public bool IsArrayLike { get; }
-        public bool IsIntegerIndexedArray { get; }
+
+        /// <summary>
+        /// Is this read-write indexed.
+        /// </summary>
+        public bool IsIntegerIndexed => IntegerIndexerProperty is not null;
+
+        /// <summary>
+        /// Read-write indexer.
+        /// </summary>
+        public PropertyInfo? IntegerIndexerProperty { get; }
+
         public bool IsDictionary { get; }
         public bool IsStringKeyedGenericDictionary => _tryGetValueMethod is not null;
         public bool IsEnumerable { get; }
@@ -62,33 +74,112 @@ namespace Jint.Runtime.Interop
         public bool Iterable => IsArrayLike || IsDictionary || IsEnumerable;
 
         public static TypeDescriptor Get(
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type)
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)]
+            Type type)
         {
             return _cache.GetOrAdd(type, t => new TypeDescriptor(t));
         }
 
-        private static bool DetermineIfObjectIsArrayLikeClrCollection([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)] Type type)
+        private static void Analyze(
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)]
+            Type type,
+            out bool isCollection,
+            out bool isEnumerable,
+            out bool isDictionary,
+            out MethodInfo? tryGetValueMethod,
+            out MethodInfo? removeMethod,
+            out PropertyInfo? keysAccessor,
+            out Type? valueType,
+            out PropertyInfo? lengthProperty,
+            out PropertyInfo? integerIndexer)
         {
-            if (typeof(ICollection).IsAssignableFrom(type))
+            AnalyzeType(
+                type,
+                out isCollection,
+                out isEnumerable,
+                out isDictionary,
+                out tryGetValueMethod,
+                out removeMethod,
+                out keysAccessor,
+                out valueType,
+                out lengthProperty,
+                out integerIndexer);
+
+            foreach (var t in type.GetInterfaces())
             {
-                return true;
+#pragma warning disable IL2072
+                AnalyzeType(
+                    t,
+                    out var isCollectionForSubType,
+                    out var isEnumerableForSubType,
+                    out var isDictionaryForSubType,
+                    out var tryGetValueMethodForSubType,
+                    out var removeMethodForSubType,
+                    out var keysAccessorForSubType,
+                    out var valueTypeForSubType,
+                    out var lengthPropertyForSubType,
+                    out var integerIndexerForSubType);
+#pragma warning restore IL2072
+
+                isCollection |= isCollectionForSubType;
+                isEnumerable |= isEnumerableForSubType;
+                isDictionary |= isDictionaryForSubType;
+
+                tryGetValueMethod ??= tryGetValueMethodForSubType;
+                removeMethod ??= removeMethodForSubType;
+                keysAccessor ??= keysAccessorForSubType;
+                valueType ??= valueTypeForSubType;
+                lengthProperty ??= lengthPropertyForSubType;
+                integerIndexer ??= integerIndexerForSubType;
             }
+        }
+
+        private static void AnalyzeType(
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties)]
+            Type type,
+            out bool isCollection,
+            out bool isEnumerable,
+            out bool isDictionary,
+            out MethodInfo? tryGetValueMethod,
+            out MethodInfo? removeMethod,
+            out PropertyInfo? keysAccessor,
+            out Type? valueType,
+            out PropertyInfo? lengthProperty,
+            out PropertyInfo? integerIndexer)
+        {
+            isCollection = typeof(ICollection).IsAssignableFrom(type);
+            isEnumerable = typeof(IEnumerable).IsAssignableFrom(type);
+            integerIndexer = _listType.IsAssignableFrom(type) ? _listIndexer : null;
 
-            foreach (var interfaceType in type.GetInterfaces())
+            isDictionary = typeof(IDictionary).IsAssignableFrom(type);
+            lengthProperty = type.GetProperty("Count") ?? type.GetProperty("Length");
+
+            tryGetValueMethod = null;
+            removeMethod = null;
+            keysAccessor = null;
+            valueType = null;
+
+            if (type.IsGenericType)
             {
-                if (!interfaceType.IsGenericType)
+                var genericTypeDefinition = type.GetGenericTypeDefinition();
+
+                // check if object has any generic dictionary signature that accepts string as key
+                var b = genericTypeDefinition == _genericDictionaryType;
+                if (b && type.GenericTypeArguments[0] == _stringType)
                 {
-                    continue;
+                    tryGetValueMethod = type.GetMethod("TryGetValue");
+                    removeMethod = type.GetMethod("Remove");
+                    keysAccessor = type.GetProperty("Keys");
+                    valueType = type.GenericTypeArguments[1];
                 }
 
-                if (interfaceType.GetGenericTypeDefinition() == typeof(IReadOnlyCollection<>)
-                    || interfaceType.GetGenericTypeDefinition() == typeof(ICollection<>))
+                isCollection |= genericTypeDefinition == typeof(IReadOnlyCollection<>) || genericTypeDefinition == typeof(ICollection<>);
+                if (genericTypeDefinition == typeof(IList<>))
                 {
-                    return true;
+                    integerIndexer ??= type.GetProperty("Item");
                 }
+                isDictionary |= genericTypeDefinition == _genericDictionaryType;
             }
-
-            return false;
         }
 
         public bool TryGetValue(object target, string member, [NotNullWhen(true)] out object? o)
@@ -120,7 +211,7 @@ namespace Jint.Runtime.Interop
                 return false;
             }
 
-            return (bool) _removeMethod.Invoke(target , new object[] { key })!;
+            return (bool) _removeMethod.Invoke(target, new object[] { key })!;
         }
 
         public ICollection<string> GetKeys(object target)