Interop: a host type converter's answer stays with the engine whose converter gave it

Jint.Tests/Runtime/InteropAccessorCacheSharingTests.cs

@@ -49,6 +49,23 @@ public sealed class StringIndexed
         public string this[string key] => key + "!";
     }
 
+    /// <summary>
+    /// Carries both halves of the decision a host <see cref="ITypeConverter"/> makes during resolution: a
+    /// declared property an indexer of the same name shadows, and a key only the indexer can answer.
+    /// </summary>
+    public sealed class Bag
+    {
+        private readonly Dictionary<string, string> _entries = new()
+        {
+            ["Name"] = "from-indexer",
+            ["Extra"] = "indexer-only",
+        };
+
+        public string Name => "from-property";
+
+        public string? this[string key] => _entries.TryGetValue(key, out var value) ? value : null;
+    }
+
     private sealed class CountingResolver
     {
         private int _memberFilterCalls;
@@ -296,6 +313,88 @@ public void CustomTypeConvertersPartitionTheCache()
         custom.Evaluate("host.key").Should().Be("key!");
     }
 
+    [Fact]
+    public void TwoCustomTypeConvertersDoNotDecideForEachOther()
+    {
+        // Both engines report a host-installed converter, so the profile puts them in the same partition -
+        // but their converters answer differently, and that answer is what decides whether the declared
+        // property is handed the indexer to probe ahead of itself (#3560).
+        var resolver = new TypeResolver();
+        var narrow = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(_ => new NarrowTypeConverter()));
+        var wide = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine)));
+
+        narrow.Evaluate("host.Name").Should().Be("from-property", "the narrow converter finds no usable indexer");
+        narrow.Evaluate("typeof host.Extra").Should().Be("undefined");
+
+        wide.Evaluate("host.Name").Should().Be("from-indexer", "the wide converter converts the member name to the indexer's key");
+        wide.Evaluate("host.Extra").Should().Be("indexer-only");
+    }
+
+    [Fact]
+    public void TwoCustomTypeConvertersDoNotDecideForEachOtherInEitherOrder()
+    {
+        var resolver = new TypeResolver();
+        var wide = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine)));
+        var narrow = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(_ => new NarrowTypeConverter()));
+
+        wide.Evaluate("host.Name").Should().Be("from-indexer");
+        narrow.Evaluate("host.Name").Should().Be("from-property");
+
+        // a fresh wrapper, so the answer is resolved again rather than served from the first one's own store
+        wide.SetValue("second", new Bag());
+        wide.Evaluate("second.Name").Should().Be("from-indexer");
+        wide.Evaluate("second.Extra").Should().Be("indexer-only");
+    }
+
+    [Fact]
+    public void CustomTypeConverterEnginesStillShareWhatTheirConverterCannotDecide()
+    {
+        // The withholding is per type, not per engine: a type whose members no converter is consulted about
+        // keeps being resolved once for every engine sharing the resolver.
+        var counting = new CountingResolver();
+        var first = CreateEngine(counting.Resolver, new Host(1), options => options.SetTypeConverter(_ => new NarrowTypeConverter()));
+        var second = CreateEngine(counting.Resolver, new Host(2), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine)));
+        counting.Reset();
+
+        first.Evaluate("host.Value").Should().Be(1);
+        counting.Reset().Should().BeGreaterThan(0, "the first engine has to resolve the member");
+
+        second.Evaluate("host.Value").Should().Be(2);
+        counting.Reset().Should().Be(0, "Host declares no indexer, so nothing here depends on the converter");
+    }
+
+    [Fact]
+    public void CustomTypeConverterEnginesDoNotGrowTheSharedCachePerEngine()
+    {
+        // Keying the partition on the converter itself would be one entry set per engine in a cache that
+        // never evicts, and would pin every host converter for the life of the process.
+        var resolver = new TypeResolver();
+
+        Engine Create()
+        {
+            var engine = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(e => new WrappingTypeConverter(e)));
+            engine.SetValue("plain", new Host(1));
+            return engine;
+        }
+
+        void Exercise(Engine engine)
+        {
+            engine.Evaluate("host.Name").Should().Be("from-indexer");
+            engine.Evaluate("plain.Value").Should().Be(1);
+        }
+
+        Exercise(Create());
+        var countAfterFirstEngine = resolver.ResolvedAccessorCount;
+        countAfterFirstEngine.Should().BeGreaterThan(0, "Host carries no indexer, so its members are still shared");
+
+        for (var i = 0; i < 20; i++)
+        {
+            Exercise(Create());
+        }
+
+        resolver.ResolvedAccessorCount.Should().Be(countAfterFirstEngine);
+    }
+
     #endregion
 
     #region 3. nothing engine-affine is shared
@@ -325,6 +424,22 @@ public void NestedTypeReferencesStayWithTheirOwnEngine()
     /// Behaves exactly like the stock converter but is not it, so the engine counts as having a
     /// host-installed <see cref="ITypeConverter"/>.
     /// </summary>
+    /// <summary>
+    /// Declines every conversion, including the string to string the stock converter accepts outright, so
+    /// resolution finds no indexer this member name can be handed to.
+    /// </summary>
+    private sealed class NarrowTypeConverter : ITypeConverter
+    {
+        public object? Convert(object? value, Type type, IFormatProvider formatProvider)
+            => throw new NotSupportedException();
+
+        public bool TryConvert(object? value, Type type, IFormatProvider formatProvider, [NotNullWhen(true)] out object? converted)
+        {
+            converted = null;
+            return false;
+        }
+    }
+
     private sealed class WrappingTypeConverter : ITypeConverter
     {
         private readonly ITypeConverter _inner;

Jint/Runtime/Interop/TypeResolver.cs

@@ -36,6 +36,12 @@ public sealed class TypeResolver
     /// </summary>
     private readonly ConcurrentDictionary<Type, bool> _indexedElementExposure = new();
 
+    /// <summary>
+    /// Memo behind <see cref="ResolutionConsultsConverter"/>, populated only for engines carrying an
+    /// <see cref="ITypeConverter"/> of the host's own and therefore having a question to ask at all.
+    /// </summary>
+    private readonly ConcurrentDictionary<Type, bool> _converterKeyedIndexers = new();
+
     /// <summary>
     /// How many accessors this resolver currently holds. The cache never evicts, so this is the retention
     /// the resolver commits to: it must stay bounded by the distinct members the engines using it resolve,
@@ -87,6 +93,7 @@ private void InvalidateResolvedAccessors()
     {
         _reflectionAccessors.Clear();
         _indexedElementExposure.Clear();
+        _converterKeyedIndexers.Clear();
     }
 
     /// <summary>
@@ -304,8 +311,16 @@ internal ReflectionAccessor GetAccessor(
 
         var key = new AccessorCacheKey(type, member, requirement, profile);
 
+        // Every engine carrying an ITypeConverter of the host's own lands in one partition, because the
+        // profile can only record that the converter is not the stock one - naming the converter itself
+        // would pin a host object in a cache that lives for the process. So when resolving this type asks
+        // that converter a question, the engine neither answers from the shared cache nor adds to it: the
+        // answer belongs to the converter that gave it, not to the partition every such engine shares
+        // (#3560).
+        var converterDecides = !engine._typeConverterIsDefault && ResolutionConsultsConverter(type);
+
         var factories = _reflectionAccessors;
-        if (factories.TryGetValue(key, out var accessor))
+        if (!converterDecides && factories.TryGetValue(key, out var accessor))
         {
             if (throwOnError
                 && ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor)
@@ -324,7 +339,7 @@ internal ReflectionAccessor GetAccessor(
             return accessor;
         }
 
-        if (IsShareable(engine, accessor))
+        if (!converterDecides && IsShareable(engine, accessor))
         {
             // racy, we don't care: both racers resolved the same member the same way
             factories.TryAdd(key, accessor);
@@ -360,6 +375,66 @@ private static bool IsShareable(Engine engine, ReflectionAccessor accessor)
         return true;
     }
 
+    /// <summary>
+    /// Whether resolving a member of <paramref name="type"/> asks the engine's <see cref="ITypeConverter"/>
+    /// anything, and therefore produces an answer that belongs to that converter alone.
+    /// </summary>
+    /// <remarks>
+    /// <see cref="IndexerAccessor.TryFindIndexer"/> converts the member <em>name</em> to the index type of
+    /// every single-parameter indexer the type or one of its interfaces offers; an <see cref="int"/>-keyed
+    /// one is the only kind settled without the converter, from the name itself. That single answer decides
+    /// whether the declared member is handed an indexer to probe ahead of itself, so it reaches well past the
+    /// <see cref="IndexerAccessor"/> that <see cref="IsShareable"/> withholds. The scan is deliberately
+    /// blind to the member filter and to which accessors an indexer carries: answering "yes" too readily only
+    /// costs such an engine a re-resolution per wrapper, while answering "no" wrongly hands one host
+    /// converter's decision to another. Memoized per resolver and per type, and asked only by an engine whose
+    /// converter is not the stock one, so the default configuration pays nothing.
+    /// </remarks>
+    private bool ResolutionConsultsConverter(
+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type)
+    {
+        if (_converterKeyedIndexers.TryGetValue(type, out var consults))
+        {
+            return consults;
+        }
+
+        consults = HasConverterKeyedIndexer(type);
+        if (!consults)
+        {
+            foreach (var interfaceType in type.GetInterfaces())
+            {
+                if (HasConverterKeyedIndexer(interfaceType))
+                {
+                    consults = true;
+                    break;
+                }
+            }
+        }
+
+        // GetOrAdd's value overload rather than TryAdd, so a race still hands every caller the same answer
+        return _converterKeyedIndexers.GetOrAdd(type, consults);
+    }
+
+    /// <summary>
+    /// Whether <paramref name="type"/> offers a single-parameter indexer keyed by anything other than
+    /// <see cref="int"/> — the one index type <see cref="IndexerAccessor.TryFindIndexer"/> keys without
+    /// consulting the engine's <see cref="ITypeConverter"/>.
+    /// </summary>
+    private static bool HasConverterKeyedIndexer(
+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] Type type)
+    {
+        foreach (var candidate in type.GetProperties())
+        {
+            var indexParameters = candidate.GetIndexParameters();
+            if (indexParameters.Length == 1 && indexParameters[0].ParameterType != typeof(int))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     private ReflectionAccessor ResolvePropertyDescriptorFactory(
         Engine engine,
         [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type,