Interop: an operator overload is chosen by the arguments in hand (backport of #3578)

Jint.Tests.PublicInterface/OperatorOverloadResolutionCacheTests.cs

@@ -181,4 +181,87 @@ public void ANonCoercingEngineDoesNotDecideForACoercingOne()
     }
 
     #endregion
+
+    #region value-decided resolution
+
+    /// <summary>
+    /// A host type carrying a narrow overload beside a catch-all one. Which of the two applies is decided by
+    /// the <em>value</em> on the right - <c>5</c> fits a <see cref="byte"/> and <c>300</c> does not - so the
+    /// pair of CLR types the two arguments have does not determine the answer, both numbers arriving as
+    /// <see cref="double"/>.
+    /// </summary>
+    public sealed class RangedA
+    {
+        public static string operator +(RangedA left, byte right) => "byte:" + right;
+        public static string operator +(RangedA left, object right) => "object:" + right;
+    }
+
+    public sealed class RangedB
+    {
+        public static string operator +(RangedB left, byte right) => "byte:" + right;
+        public static string operator +(RangedB left, object right) => "object:" + right;
+    }
+
+    public sealed class RangedC
+    {
+        public static string operator +(RangedC left, byte right) => "byte:" + right;
+        public static string operator +(RangedC left, object right) => "object:" + right;
+    }
+
+    public sealed class RangedD
+    {
+        public static string operator +(RangedD left, byte right) => "byte:" + right;
+        public static string operator +(RangedD left, object right) => "object:" + right;
+    }
+
+    public sealed class RangedE
+    {
+        public static string operator +(RangedE left, byte right) => "byte:" + right;
+        public static string operator +(RangedE left, object right) => "object:" + right;
+    }
+
+    private static string AddNumber(Engine engine, object host, string expression)
+    {
+        engine.SetValue("m", host);
+        return engine.Evaluate(expression).AsString();
+    }
+
+    [Fact]
+    public void ASmallNumberAloneTakesTheNarrowOverload()
+    {
+        AddNumber(StockEngine(), new RangedA(), "m + 5").Should().Be("byte:5");
+    }
+
+    [Fact]
+    public void ALargeNumberAloneTakesTheCatchAllOverload()
+    {
+        AddNumber(StockEngine(), new RangedB(), "m + 300").Should().Be("object:300");
+    }
+
+    [Fact]
+    public void ASmallNumberDoesNotDecideForALargeOne()
+    {
+        AddNumber(StockEngine(), new RangedC(), "m + 5").Should().Be("byte:5");
+        AddNumber(StockEngine(), new RangedC(), "m + 300").Should().Be("object:300");
+    }
+
+    [Fact]
+    public void ALargeNumberDoesNotDecideForASmallOne()
+    {
+        AddNumber(StockEngine(), new RangedD(), "m + 300").Should().Be("object:300");
+        AddNumber(StockEngine(), new RangedD(), "m + 5").Should().Be("byte:5");
+    }
+
+    [Fact]
+    public void OneEngineSelectsPerEvaluationToo()
+    {
+        var engine = StockEngine();
+        engine.SetValue("m", new RangedE());
+
+        engine.Evaluate("m + 5").AsString().Should().Be("byte:5");
+        engine.Evaluate("m + 300").AsString().Should().Be("object:300");
+        engine.Evaluate("m + 5").AsString().Should().Be("byte:5");
+    }
+
+    #endregion
 }

Jint.Tests/Runtime/OperatorOverloadResolutionCacheTests.cs

@@ -1,13 +1,14 @@
 #nullable enable
 
 using Jint.Runtime.Interop;
+using Jint.Runtime.Interpreter.Expressions;
 
 namespace Jint.Tests.Runtime;
 
 /// <summary>
-/// Which table an engine remembers its operator-overload resolutions in. The public-interface suite pins the
-/// answers two engines must not take from each other; this pins the mechanism that keeps them apart, since
-/// "both engines were right" is also what a cache that was never consulted looks like.
+/// What an engine is allowed to remember about an operator overload. The public-interface suite pins the
+/// answers two engines, and two argument values, must not take from each other; this pins the mechanism that
+/// keeps them apart, since "every answer was right" is also what a cache that was never consulted looks like.
 /// </summary>
 public class OperatorOverloadResolutionCacheTests
 {
@@ -16,6 +17,19 @@ public sealed class Amount
         public static string operator +(Amount left, Amount right) => "operator";
     }
 
+    /// <summary>Declares no operator at all, so every pair it takes part in has an empty candidate set.</summary>
+    public sealed class Plain
+    {
+        public override string ToString() => "plain";
+    }
+
+    /// <summary>Carries two <c>+</c> overloads only an argument's value can choose between.</summary>
+    public sealed class Ranged
+    {
+        public static string operator +(Ranged left, byte right) => "byte:" + right;
+        public static string operator +(Ranged left, object right) => "object:" + right;
+    }
+
     /// <summary>Behaves exactly like the stock converter but is not it, which is what makes it a host one.</summary>
     private sealed class WrappingTypeConverter : DefaultTypeConverter
     {
@@ -24,51 +38,72 @@ public WrappingTypeConverter(Engine engine) : base(engine)
         }
     }
 
-    private static Engine Evaluate(Action<Options>? configure = null)
+    private static Engine NewEngine(Action<Options>? configure = null) => new(options =>
     {
-        var engine = new Engine(options =>
-        {
-            options.Interop.AllowOperatorOverloading = true;
-            configure?.Invoke(options);
-        });
+        options.Interop.AllowOperatorOverloading = true;
+        configure?.Invoke(options);
+    });
 
-        engine.SetValue("a", new Amount());
-        engine.SetValue("b", new Amount());
-        engine.Evaluate("a + b").AsString().Should().Be("operator");
-        return engine;
+    private static MethodDescriptor[]? CandidatesFor(Type left, Type right)
+    {
+        var key = new JintBinaryExpression.OperatorKey("op_Addition", left, right);
+        return JintBinaryExpression._operatorCandidates.TryGetValue(key, out var candidates) ? candidates : null;
     }
 
     [Fact]
-    public void AStockEngineRemembersInTheProcessWideTable()
+    public void WhatIsRememberedIsTheCandidateSetAndNotTheSelection()
     {
-        var engine = Evaluate();
+        var engine = NewEngine();
+        engine.SetValue("m", new Ranged());
 
-        engine._engineOperatorOverloads.Should().BeNull(
-            "the stock converter resolves what every other stock engine would, so the answer is shareable");
+        engine.Evaluate("m + 5").AsString().Should().Be("byte:5");
+
+        CandidatesFor(typeof(Ranged), typeof(double)).Should().NotBeNull().And.HaveCount(2,
+            "the overload this value did not select has to stay available to the next evaluation");
     }
 
     [Fact]
-    public void AnEngineWithItsOwnConverterRemembersOnItself()
+    public void TheSetIsScannedOnceAndSharedByEveryEngine()
     {
-        var engine = Evaluate(options => options.SetTypeConverter(e => new WrappingTypeConverter(e)));
+        var stock = NewEngine();
+        stock.SetValue("a", new Amount());
+        stock.SetValue("b", new Amount());
+        stock.Evaluate("a + b");
+
+        var first = CandidatesFor(typeof(Amount), typeof(Amount));
 
-        engine._engineOperatorOverloads.Should().NotBeNull().And.HaveCount(1,
-            "this engine's converter answers overload scoring's last rule, so its resolution is its own");
+        // A converter of its own used to give an engine a resolution table of its own, because a resolution
+        // was scored against that converter. A candidate set is not, so there is one table again.
+        var withConverter = NewEngine(options => options.SetTypeConverter(e => new WrappingTypeConverter(e)));
+        withConverter.SetValue("a", new Amount());
+        withConverter.SetValue("b", new Amount());
+        withConverter.Evaluate("a + b");
 
-        // and it really is a cache: a second evaluation of the same pair adds nothing
-        engine.Evaluate("a + b").AsString().Should().Be("operator");
-        engine._engineOperatorOverloads.Should().HaveCount(1);
+        CandidatesFor(typeof(Amount), typeof(Amount)).Should().BeSameAs(first,
+            "a reflection scan of two types is the same answer for every engine in the process");
     }
 
     [Fact]
-    public void SwappingTheConverterDropsWhatTheOldOneDecided()
+    public void APairWithNoOperatorIsRememberedAsHavingNone()
     {
-        var engine = Evaluate(options => options.SetTypeConverter(e => new WrappingTypeConverter(e)));
-        engine._engineOperatorOverloads.Should().HaveCount(1);
+        var engine = NewEngine();
+        engine.SetValue("p", new Plain());
+
+        engine.Evaluate("p + 'x'").AsString().Should().Be("plainx");
+
+        CandidatesFor(typeof(Plain), typeof(string)).Should().NotBeNull().And.BeEmpty(
+            "an empty set is what lets the next evaluation of the same pair leave without allocating anything");
+    }
 
-        engine.TypeConverter = new WrappingTypeConverter(engine);
+    [Fact]
+    public void ACandidateIsListedOnceEvenWhenBothOperandsDeclareIt()
+    {
+        var engine = NewEngine();
+        engine.SetValue("a", new Amount());
+        engine.SetValue("b", new Amount());
+        engine.Evaluate("a + b");
 
-        engine._engineOperatorOverloads.Should().BeEmpty(
-            "every entry was scored against the converter that has just been replaced");
+        CandidatesFor(typeof(Amount), typeof(Amount)).Should().NotBeNull().And.HaveCount(1,
+            "both operand types are scanned, and `T + T` finds the same operator on each");
     }
 }

Jint/Engine.cs

@@ -347,24 +347,6 @@ internal void CacheFunctionDefinition(Node key, JintFunctionDefinition definitio
     // one Options instance be shared by engines that are already running.
     internal readonly bool _operatorOverloadingAllowed;
 
-    // Snapshot of Options.Interop.ValueCoercion, read the same way and for the same reason. It steers
-    // overload scoring's gray-zone rule, so it is part of the key JintBinaryExpression remembers an
-    // operator-overload resolution under: two engines that coerce differently can rank the same candidates
-    // differently, and must not answer from one another's entries.
-    internal readonly ValueCoercionType _valueCoercion;
-
-    /// <summary>
-    /// Operator-overload resolutions belonging to this engine alone, created on first use and only by an
-    /// engine whose <see cref="TypeConverter"/> is not the stock one.
-    /// </summary>
-    /// <remarks>
-    /// Such an engine's converter answers overload scoring's last rule, so its resolutions are not the ones a
-    /// stock engine would reach and cannot go in the process-wide table - nor could that table be keyed on the
-    /// converter, which is a host object handed this engine by its factory and would pin both for the life of
-    /// the process. The <see cref="TypeConverter"/> setter drops these when the converter changes.
-    /// </remarks>
-    internal ConcurrentDictionary<JintBinaryExpression.OperatorKey, MethodDescriptor?>? _engineOperatorOverloads;
-
     internal readonly ReferencePool _referencePool;
     internal readonly ArgumentsInstancePool _argumentsInstancePool;
     internal readonly JsValueArrayPool _jsValueArrayPool;
@@ -385,11 +367,6 @@ internal set
         {
             _typeConverter = value;
             _typeConverterIsDefault = value.GetType() == typeof(DefaultTypeConverter);
-
-            // Every operator-overload resolution this engine remembered was scored against the converter
-            // being replaced, and the stock table it may now be entitled to was not. Nothing is resolved
-            // before the first assignment, so this is a no-op during construction.
-            _engineOperatorOverloads?.Clear();
         }
     }
 
@@ -535,7 +512,6 @@ private Engine(Options? options, Action<Engine, Options>? configure)
         // documented place to finish configuring an engine, and enabling operator overloading from
         // one has to reach the expressions that consult it.
         _operatorOverloadingAllowed = Options.Interop.AllowOperatorOverloading;
-        _valueCoercion = Options.Interop.ValueCoercion;
 
         // likewise after Apply, which is where a custom ITypeConverter gets installed
         _interopResolutionProfile = new InteropResolutionProfile(

Jint/Runtime/Interop/InteropHelper.cs

@@ -456,7 +456,14 @@ internal static MethodMatches FindBestMatch<TState>(
         Func<MethodDescriptor, TState, JsValue[]> argumentProvider,
         TState state)
     {
+        // The first surviving candidate is held here rather than in a list, because there is nothing to sort
+        // until a second one survives — and one survivor is the usual outcome, not an edge case. It matters
+        // most on the operator lane, where scoring runs on every evaluation (JintBinaryExpression's candidate
+        // set is cached; which of them applies is not, since the argument values decide it).
+        MethodMatch survivor = default;
+        var haveSurvivor = false;
         List<MethodMatch>? matchingByParameterCount = null;
+
         foreach (var method in methods)
         {
             var parameterInfos = method.Parameters;
@@ -477,21 +484,30 @@ internal static MethodMatches FindBestMatch<TState>(
                     continue;
                 }
 
-                matchingByParameterCount ??= [];
-                matchingByParameterCount.Add(new MethodMatch(method, arguments, score));
+                var match = new MethodMatch(method, arguments, score);
+                if (!haveSurvivor)
+                {
+                    survivor = match;
+                    haveSurvivor = true;
+                    continue;
+                }
+
+                matchingByParameterCount ??= [survivor];
+                matchingByParameterCount.Add(match);
             }
         }
 
-        if (matchingByParameterCount == null)
+        if (!haveSurvivor)
         {
             return MethodMatches.Empty;
         }
 
-        if (matchingByParameterCount.Count > 1)
+        if (matchingByParameterCount is null)
         {
-            matchingByParameterCount.Sort();
+            return MethodMatches.Single(in survivor);
         }
 
+        matchingByParameterCount.Sort();
         return MethodMatches.Sorted(matchingByParameterCount);
     }
 }

Jint/Runtime/Interop/MethodDescriptor.cs

@@ -198,7 +198,7 @@ public static InteropParameterFlags ComputeParameterFlags(Type parameterType)
     // Trade-off: a static cache keyed by MethodBase pins that MethodBase - and therefore its
     // declaring assembly - for the lifetime of the process. That matches the precedent already set
     // by this codebase's other process-wide reflection caches (TypeDescriptor._cache,
-    // TypeReference._memberAccessors, JintBinaryExpression._knownOperators,
+    // TypeReference._memberAccessors, JintBinaryExpression._operatorCandidates,
     // DefaultTypeConverter._knownCastOperators).
     //
     // A concurrent duplicate build for the same key is benign (both produce equivalent invokers and

Jint/Runtime/Interop/Reflection/CompiledMemberAccessor.cs

@@ -47,7 +47,7 @@ namespace Jint.Runtime.Interop.Reflection;
 /// cache lives on <see cref="TypeResolver"/>, so engines configured with resolvers of their own
 /// would otherwise recompile every member each time. The trade-off is the same one the other
 /// process-wide reflection caches in this assembly make (<see cref="TypeDescriptor"/>,
-/// <see cref="TypeReference"/>, <c>JintBinaryExpression._knownOperators</c>): the cached
+/// <see cref="TypeReference"/>, <c>JintBinaryExpression._operatorCandidates</c>): the cached
 /// <see cref="MemberInfo"/> keeps its declaring assembly alive for the process lifetime.
 /// </para>
 /// </summary>