Interop: two engines in one process do not decide each other's conversions and operators (backport of #3521 and #3526) (#3559)

Three process-wide interop caches were keyed on less than the answer they hold depends on, so
whichever engine reached one first decided for every engine after it.

`DefaultTypeConverter` memoizes the CLR delegate a JavaScript function converts to - it has to, since
`host.on(f)` and `host.off(f)` only pair up if both conversions hand the host the same `Delegate`
instance. The memo was keyed on the function instance, and one level below it on that function's AST
node. Neither carries the target delegate `Type`, which is the one thing `BuildTargetBinderDelegate`
bakes in, so the second target type a function was converted for was served the first one's delegate
and the reflection invoke behind the call rejected it. Both caches are type-keyed now, through an
append-only `TypeKeyedCache<T>` published with one `Interlocked.CompareExchange`, since a shared
`Prepared<Script>` is documented as usable from several engines at once.

The AST node is process-wide state that outlives every engine that ran the preparation, and the
binder it held baked the *converting* engine in as a constant. So a second engine running the same
preparation marshalled its host callback's arguments through the first engine's realm - an
`instanceof Object` that answers `false` - and kept that engine, its realm and its intrinsics alive
for the life of the AST. The binder reads the engine off the target function instead, which is what
makes the entry shareable at all.

`JintBinaryExpression._knownOperators` remembers which CLR operator a `(name, left type, right type)`
triple selects, and the selection runs `InteropHelper.FindBestMatch`, which reads two things the
embedder configures: `Options.Interop.ValueCoercion`, which the gray-zone scoring rule consults, and
the installed `ITypeConverter`, whose answer is the last rule outright. The coercion setting is a
value, so it goes in the key and engines that share it go on sharing entries; the converter is a host
object the engine's factory was handed, so keying a process-lived static on it would pin that
converter and its engine forever - it selects the table instead, an engine with its own converter
keeping its resolutions on itself where they die with it.

Fixes #3434 and #3424 on 4.x.

Author: lahma

Jint.Tests.PublicInterface/HostDelegateConversionTests.cs

@@ -0,0 +1,128 @@
+#nullable enable
+
+namespace Jint.Tests.PublicInterface;
+
+/// <summary>
+/// Pins that the CLR delegate a JavaScript function converts to is the one built for the target type the
+/// host asked for, in every engine that asks (sebastienros/jint#3434).
+/// </summary>
+/// <remarks>
+/// <para>
+/// The conversion is memoized so <c>host.on(f)</c> and <c>host.off(f)</c> hand the host the same
+/// <see cref="Delegate"/> instance, which is the identity <c>-=</c> needs. The memo used to be keyed on the
+/// function alone, and one level down on that function's AST node — neither of which carries the delegate
+/// type the compiled binder was built for. The AST node is process-wide state and
+/// <see cref="Engine.PrepareScript"/> is documented as shareable across engines, so whichever engine
+/// evaluated a shared preparation first decided the target type for every engine after it, and the
+/// reflection invoke that followed rejected the delegate it was handed with
+/// <c>ArgumentException: Object of type 'Notify' cannot be converted to type 'Transform'</c>.
+/// </para>
+/// <para>
+/// This is the embedder-visible half of it: two engines, different host types, one cached
+/// <c>Prepared&lt;Script&gt;</c> — the shape the README recommends for production.
+/// </para>
+/// </remarks>
+public class HostDelegateConversionTests
+{
+    public delegate void Notify(int value);
+
+    public delegate string Transform(string value);
+
+    public sealed class NotifyHost
+    {
+        public string Call(Notify f)
+        {
+            f(1);
+            return "notify";
+        }
+    }
+
+    public sealed class TransformHost
+    {
+        public string Call(Transform f) => "transform:" + f("x");
+    }
+
+    public delegate string Feed(Payload value);
+
+    public sealed class Payload
+    {
+        public int Value => 42;
+    }
+
+    public sealed class PayloadHost
+    {
+        public string Call(Feed f) => f(new Payload());
+    }
+
+    private const string Source = "host.call(function (x) { return String(x); });";
+
+    [Fact]
+    public void TwoEnginesSharingOnePreparedScript()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var first = new Engine();
+        first.SetValue("host", new NotifyHost());
+        first.Evaluate(prepared).AsString().Should().Be("notify");
+
+        var second = new Engine();
+        second.SetValue("host", new TransformHost());
+        second.Evaluate(prepared).AsString().Should().Be("transform:x");
+    }
+
+    [Fact]
+    public void TwoEnginesSharingOnePreparedScriptInTheOtherOrder()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var first = new Engine();
+        first.SetValue("host", new TransformHost());
+        first.Evaluate(prepared).AsString().Should().Be("transform:x");
+
+        var second = new Engine();
+        second.SetValue("host", new NotifyHost());
+        second.Evaluate(prepared).AsString().Should().Be("notify");
+    }
+
+    /// <summary>
+    /// The other half of what the shared AST-node cache holds: the compiled binder marshals the host's
+    /// arguments into the realm of the engine that is running, not of whichever engine compiled it. One
+    /// delegate type is enough here — the two engines share the binder by design — so this is the case the
+    /// target-type key alone does not cover.
+    /// </summary>
+    [Fact]
+    public void ASharedBinderMarshalsIntoTheRunningEnginesRealm()
+    {
+        var prepared = Engine.PrepareScript("host.call(function (x) { return String(x instanceof Object) + ':' + x.Value; });");
+
+        var first = new Engine();
+        first.SetValue("host", new PayloadHost());
+        first.Evaluate(prepared).AsString().Should().Be("true:42");
+
+        var second = new Engine();
+        second.SetValue("host", new PayloadHost());
+        second.Evaluate(prepared).AsString().Should().Be("true:42",
+            "an object built by the first engine's realm is not an Object in the second engine's");
+    }
+
+    /// <summary>
+    /// The same preparation alternating between the two engines: each engine keeps answering for its own
+    /// host type however many times the other one has run in between.
+    /// </summary>
+    [Fact]
+    public void AlternatingEnginesOnOnePreparedScript()
+    {
+        var prepared = Engine.PrepareScript(Source);
+
+        var notify = new Engine();
+        notify.SetValue("host", new NotifyHost());
+        var transform = new Engine();
+        transform.SetValue("host", new TransformHost());
+
+        for (var i = 0; i < 5; i++)
+        {
+            notify.Evaluate(prepared).AsString().Should().Be("notify");
+            transform.Evaluate(prepared).AsString().Should().Be("transform:x");
+        }
+    }
+}

Jint.Tests.PublicInterface/OperatorOverloadResolutionCacheTests.cs

@@ -0,0 +1,184 @@
+#nullable enable
+
+using System.Diagnostics.CodeAnalysis;
+using Jint.Runtime.Interop;
+
+namespace Jint.Tests.PublicInterface;
+
+/// <summary>
+/// Which operator overload a <c>+</c> over host types selects is resolved once and remembered, and the
+/// resolution reads two things the embedder configures: <see cref="Options.InteropOptions.ValueCoercion"/>
+/// and the installed <see cref="ITypeConverter"/>. Two engines configured differently must therefore not
+/// answer one another's operator resolutions.
+/// </summary>
+/// <remarks>
+/// One host type per test — the resolution cache never evicts, so a type reused across tests would make
+/// them order-dependent on each other rather than self-contained.
+/// </remarks>
+public class OperatorOverloadResolutionCacheTests
+{
+    #region converter-decided resolution
+
+    /// <summary>
+    /// A host type whose only <c>+</c> is <c>(T, T)</c>. Evaluating <c>'s' + v</c> therefore asks whether a
+    /// string can become a <c>T</c> — a question no structural scoring rule can answer, so the installed
+    /// converter answers it and decides between an operator call and plain string concatenation.
+    /// </summary>
+    public sealed class MoneyA
+    {
+        public static string operator +(MoneyA left, MoneyA right) => "operator";
+    }
+
+    public sealed class MoneyB
+    {
+        public static string operator +(MoneyB left, MoneyB right) => "operator";
+    }
+
+    public sealed class MoneyC
+    {
+        public static string operator +(MoneyC left, MoneyC right) => "operator";
+    }
+
+    public sealed class MoneyD
+    {
+        public static string operator +(MoneyD left, MoneyD right) => "operator";
+    }
+
+    /// <summary>
+    /// Converts a string to whatever target type is asked for by handing back that type's default instance,
+    /// and defers everything else to the stock conversions. That is the shape of a host converter that
+    /// teaches the engine one of its own types — and it is what makes <c>'s' + v</c> resolve to the operator.
+    /// </summary>
+    private sealed class StringToHostTypeConverter : DefaultTypeConverter
+    {
+        public StringToHostTypeConverter(Engine engine) : base(engine)
+        {
+        }
+
+        public override bool TryConvert(object? value, Type type, IFormatProvider formatProvider, [NotNullWhen(true)] out object? converted)
+        {
+            if (value is string && (type == typeof(MoneyA) || type == typeof(MoneyB) || type == typeof(MoneyC) || type == typeof(MoneyD)))
+            {
+                converted = Activator.CreateInstance(type)!;
+                return true;
+            }
+
+            return base.TryConvert(value, type, formatProvider, out converted);
+        }
+    }
+
+    private static Engine StockEngine() => new(options => options.Interop.AllowOperatorOverloading = true);
+
+    private static Engine ConverterEngine() => new(options =>
+    {
+        options.Interop.AllowOperatorOverloading = true;
+        options.SetTypeConverter(engine => new StringToHostTypeConverter(engine));
+    });
+
+    private static string Add(Engine engine, object host)
+    {
+        engine.SetValue("v", host);
+        return engine.Evaluate("'s' + v").AsString();
+    }
+
+    [Fact]
+    public void StockEngineAloneConcatenates()
+    {
+        Add(StockEngine(), new MoneyA()).Should().NotBe("operator");
+    }
+
+    [Fact]
+    public void ConverterEngineAloneCallsTheOperator()
+    {
+        Add(ConverterEngine(), new MoneyB()).Should().Be("operator");
+    }
+
+    [Fact]
+    public void AConverterEngineDoesNotDecideForAStockEngine()
+    {
+        Add(ConverterEngine(), new MoneyC()).Should().Be("operator");
+        Add(StockEngine(), new MoneyC()).Should().NotBe("operator");
+    }
+
+    [Fact]
+    public void AStockEngineDoesNotDecideForAConverterEngine()
+    {
+        Add(StockEngine(), new MoneyD()).Should().NotBe("operator");
+        Add(ConverterEngine(), new MoneyD()).Should().Be("operator");
+    }
+
+    #endregion
+
+    #region coercion-decided resolution
+
+    /// <summary>
+    /// A host type whose only <c>+</c> takes a <see cref="string"/> on the right. Handing it an opaque host
+    /// object is a pair no structural rule recognizes, so whether the candidate survives at all is decided
+    /// by <see cref="Options.InteropOptions.ValueCoercion"/>'s string rule.
+    /// </summary>
+    public sealed class CoercedA
+    {
+        public static string operator +(CoercedA left, string right) => "operator";
+    }
+
+    public sealed class CoercedB
+    {
+        public static string operator +(CoercedB left, string right) => "operator";
+    }
+
+    public sealed class CoercedC
+    {
+        public static string operator +(CoercedC left, string right) => "operator";
+    }
+
+    public sealed class CoercedD
+    {
+        public static string operator +(CoercedD left, string right) => "operator";
+    }
+
+    /// <summary>Carries nothing the engine can convert, so only the coercion rule can bind it to a string.</summary>
+    public sealed class Opaque;
+
+    private static Engine CoercingEngine() => new(options => options.Interop.AllowOperatorOverloading = true);
+
+    private static Engine NonCoercingEngine() => new(options =>
+    {
+        options.Interop.AllowOperatorOverloading = true;
+        options.Interop.ValueCoercion = ValueCoercionType.None;
+    });
+
+    private static string AddOpaque(Engine engine, object host)
+    {
+        engine.SetValue("v", host);
+        engine.SetValue("o", new Opaque());
+        return engine.Evaluate("v + o").AsString();
+    }
+
+    [Fact]
+    public void CoercingEngineAloneCallsTheOperator()
+    {
+        AddOpaque(CoercingEngine(), new CoercedA()).Should().Be("operator");
+    }
+
+    [Fact]
+    public void NonCoercingEngineAloneConcatenates()
+    {
+        AddOpaque(NonCoercingEngine(), new CoercedB()).Should().NotBe("operator");
+    }
+
+    [Fact]
+    public void ACoercingEngineDoesNotDecideForANonCoercingOne()
+    {
+        AddOpaque(CoercingEngine(), new CoercedC()).Should().Be("operator");
+        AddOpaque(NonCoercingEngine(), new CoercedC()).Should().NotBe("operator");
+    }
+
+    [Fact]
+    public void ANonCoercingEngineDoesNotDecideForACoercingOne()
+    {
+        AddOpaque(NonCoercingEngine(), new CoercedD()).Should().NotBe("operator");
+        AddOpaque(CoercingEngine(), new CoercedD()).Should().Be("operator");
+    }
+
+    #endregion
+}