diff --git a/Jint.Tests.PublicInterface/HostArrayElementOverloadTests.cs b/Jint.Tests.PublicInterface/HostArrayElementOverloadTests.cs new file mode 100644 index 0000000000..b8f382fcec --- /dev/null +++ b/Jint.Tests.PublicInterface/HostArrayElementOverloadTests.cs @@ -0,0 +1,303 @@ +#nullable enable + +using System.Globalization; +using Jint.Native; +using Jint.Runtime; + +namespace Jint.Tests.PublicInterface; + +/// +/// Overload candidates that differ only in the element type of an array or collection parameter are chosen +/// by that element type, and an element the conversion cannot perform declines instead of throwing. +/// +/// +/// +/// A params call bundles its trailing arguments into one JavaScript array before scoring, so scoring +/// saw a single argument against CDecimal[], CInteger[] and CLong[] alike and answered a +/// blanket "is an array, wants an array" for every one of them. Every candidate tied, the converter probe +/// below that rule — the rule that would have declined CInteger -> CDecimal — was never reached, and +/// declaration order decided. Where the first-declared candidate happened to be convertible the wrong +/// overload answered silently; where it was not, the conversion's left +/// through the embedder's Evaluate, because the composite branches of TryConvert converted their +/// parts through the throwing Convert and so escaped the candidate loop that exists to move on. +/// +/// +/// #3754, reported in +/// discussion #3746; the +/// same shape as #3407 and +/// #3577, one level down — a score that +/// claims a binding the conversion cannot perform. +/// +/// +public class HostArrayElementOverloadTests +{ + // Three unrelated host classes, none of them IConvertible and none declaring a conversion to any other: + // exactly the shape of the report, where nothing but the element type tells the overloads apart. + + public sealed class CDecimal + { + public CDecimal(decimal value) => Value = value; + + public decimal Value { get; } + + public string Kind => nameof(CDecimal); + } + + public sealed class CInteger + { + public CInteger(int value) => Value = value; + + public int Value { get; } + + public string Kind => nameof(CInteger); + } + + public sealed class CLong + { + public CLong(long value) => Value = value; + + public long Value { get; } + + public string Kind => nameof(CLong); + } + + public sealed class MathHost + { + public CDecimal Add(params CDecimal[] args) + { + decimal sum = 0; + foreach (var arg in args) + { + sum += arg.Value; + } + + return new CDecimal(sum); + } + + public CInteger Add(params CInteger[] args) + { + var sum = 0; + foreach (var arg in args) + { + sum += arg.Value; + } + + return new CInteger(sum); + } + + public CLong Add(params CLong[] args) + { + long sum = 0; + foreach (var arg in args) + { + sum += arg.Value; + } + + return new CLong(sum); + } + } + + /// Both elements convert to both parameter types; only the score can tell the two apart. + public sealed class StringDeclaredFirst + { + public string Join(params string[] values) => string.Concat(values); + + public string Join(params int[] values) + { + var sum = 0; + foreach (var value in values) + { + sum += value; + } + + return sum.ToString(CultureInfo.InvariantCulture); + } + } + + /// The same pair in the opposite declaration order. + public sealed class IntDeclaredFirst + { + public string Join(params int[] values) + { + var sum = 0; + foreach (var value in values) + { + sum += value; + } + + return sum.ToString(CultureInfo.InvariantCulture); + } + + public string Join(params string[] values) => string.Concat(values); + } + + public sealed class CollectionHost + { + public string Sum(List values) => "CDecimal:" + values.Count; + + public string Sum(List values) + { + var sum = 0; + foreach (var value in values) + { + sum += value.Value; + } + + return "CInteger:" + sum.ToString(CultureInfo.InvariantCulture); + } + } + + public sealed class CatchAllHost + { + public string Take(object value) => "object"; + + public string Take(params object[] values) => "params:" + values.Length; + } + + public sealed class JsValueParamsHost + { + public string Take(char value) => "char:" + value; + + public string Take(params JsValue[] values) => "params:" + values.Length; + } + + public sealed class Bag + { + public Bag(params CDecimal[] items) => Kind = "CDecimal:" + items.Length; + + public Bag(params CInteger[] items) => Kind = "CInteger:" + items.Length; + + public string Kind { get; } + } + + private static Engine NewEngine() + { + var engine = new Engine(); + engine.SetValue("math", new MathHost()); + engine.SetValue("stringFirst", new StringDeclaredFirst()); + engine.SetValue("intFirst", new IntDeclaredFirst()); + engine.SetValue("collections", new CollectionHost()); + engine.SetValue("catchAll", new CatchAllHost()); + engine.SetValue("jsValues", new JsValueParamsHost()); + engine.SetValue("Bag", typeof(Bag)); + engine.SetValue("a", new CInteger(1)); + engine.SetValue("b", new CInteger(2)); + return engine; + } + + // ---- method lane --------------------------------------------------------------------------------- + + [Test] + public void AParamsOverloadIsChosenByTheElementTypeTheArgumentsActuallyAre() + { + // The report verbatim. CDecimal is declared first and nothing converts a CInteger to it, so the call + // used to die inside Convert.ChangeType with "Object must implement IConvertible" - a CLR exception + // out of Evaluate that neither a script catch nor a host catch (JavaScriptException) could see. + var engine = NewEngine(); + + engine.Evaluate("math.Add(a, b).Kind").AsString().Should().Be("CInteger"); + engine.Evaluate("math.Add(a, b).Value").Should().Be(3); + } + + [Test] + public void AConvertibleButWrongEarlierOverloadNoLongerWinsSilently() + { + // The half that never threw: a double element converts to string through Convert.ChangeType, so the + // first-declared params string[] answered the concatenation "12" for Join(1, 2). The element type is + // an exact match for int[] and a forced conversion for string[], so int[] is now the better score. + NewEngine().Evaluate("stringFirst.Join(1, 2)").AsString().Should().Be("3"); + } + + [Test] + public void DeclarationOrderDoesNotDecide() + { + var engine = NewEngine(); + + engine.Evaluate("stringFirst.Join(1, 2)").AsString() + .Should().Be(engine.Evaluate("intFirst.Join(1, 2)").AsString()); + } + + [Test] + public void AnArgumentNoOverloadAcceptsIsACatchableResolutionFailure() + { + // Nothing converts a string to any of the three element types, so every candidate declines and the + // call ends in resolution rather than in whatever the first candidate's conversion threw. + var engine = NewEngine(); + + Invoking(() => engine.Evaluate("math.Add('text')")) + .Should().Throw() + .WithMessage("No public methods with the specified arguments were found."); + + engine.Evaluate("try { math.Add('text') } catch (e) { e instanceof TypeError }") + .AsBoolean().Should().BeTrue(); + } + + [Test] + public void AGenericCollectionParameterTakesTheSamePath() + { + // List and the other single-argument generic collection types are scored by the same branch, and + // read their element type off the generic argument rather than off Type.GetElementType(). + NewEngine().Evaluate("collections.Sum([a, b])").AsString().Should().Be("CInteger:3"); + } + + [Test] + public void AnExplicitJavaScriptArrayArgumentTakesTheSamePath() + { + // A single array argument is passed through to the params parameter unwrapped, so it reaches scoring + // as the very same JsArray the bundling would have built. + var engine = NewEngine(); + + engine.Evaluate("math.Add([a, b]).Kind").AsString().Should().Be("CInteger"); + engine.Evaluate("math.Add([a, b]).Value").Should().Be(3); + } + + [Test] + public void AnEmptyArrayKeepsTodaysAnswer() + { + // Deliberately unchanged: with no element to read, the candidates are genuinely indistinguishable and + // the base score is all there is. Pinned so that a future element rule cannot quietly change it. + NewEngine().Evaluate("math.Add().Kind").AsString().Should().Be("CDecimal"); + } + + // ---- carve-out guards ---------------------------------------------------------------------------- + + [Test] + public void AnObjectElementTypeKeepsItsScoreBesideAScalarObjectOverload() + { + // params object[] is what every host writes for "anything"; scoring its elements would rate each of + // them the catch-all 5 and hand the call to the scalar object overload instead. + NewEngine().Evaluate("catchAll.Take(1)").AsString().Should().Be("params:1"); + } + + [Test] + public void AJsValueElementTypeKeepsItsScoreBesideAConvertibleScalarOverload() + { + // params JsValue[] is the other "anything" signature. A JsString is an is-a match for JsValue rather + // than an exact one, so scoring the elements would add 1 and tie this with the char overload beside + // it - and a tie is decided by the candidate order, which puts params last. + NewEngine().Evaluate("jsValues.Take('x')").AsString().Should().Be("params:1"); + } + + // ---- constructor lane ---------------------------------------------------------------------------- + + [Test] + public void AConstructorParamsOverloadIsChosenByTheElementTypeToo() + { + // Constructor resolution has no retry - TypeReference calls the first match and stops - so the wrong + // selection was not merely a preference here, it was the whole answer. + NewEngine().Evaluate("new Bag(a, b).Kind").AsString().Should().Be("CInteger:2"); + } + + [Test] + public void AConstructorArgumentNoOverloadAcceptsIsACatchableResolutionFailure() + { + var engine = NewEngine(); + + Invoking(() => engine.Evaluate("new Bag('text')")) + .Should().Throw() + .WithMessage("Could not resolve a constructor for the specified arguments."); + + engine.Evaluate("try { new Bag('text') } catch (e) { e instanceof TypeError }") + .AsBoolean().Should().BeTrue(); + } +} diff --git a/Jint.Tests/Runtime/Interop/CompositeConversionDeclineTests.cs b/Jint.Tests/Runtime/Interop/CompositeConversionDeclineTests.cs new file mode 100644 index 0000000000..8db22930f9 --- /dev/null +++ b/Jint.Tests/Runtime/Interop/CompositeConversionDeclineTests.cs @@ -0,0 +1,145 @@ +#nullable enable + +using System.Collections.ObjectModel; +using System.Globalization; +using Jint.Runtime.Interop; + +namespace Jint.Tests.Runtime.Interop; + +/// +/// declines a composite whose parts cannot be converted, and +/// still throws for the very same input. +/// +/// +/// The composite branches — List<T>, Collection<T>, T[], a target dictionary's +/// values and the members of a POCO built from a dictionary — converted their parts through the public, +/// throwing Convert whatever their own frame had been asked, so a Try method documented as +/// returning false threw a CLR exception instead. That is what defeated the candidate loop in +/// MethodInfoFunction.Call, which asks the converter per candidate and is meant to move on when one +/// declines (#3754). +/// +public class CompositeConversionDeclineTests +{ + public sealed class Widget + { + public string Name { get; set; } = "widget"; + } + + public sealed class Gadget + { + public string Name { get; set; } = "gadget"; + } + + public sealed class Holder + { + public Gadget? Part { get; set; } + } + + private static DefaultTypeConverter NewConverter() => new DefaultTypeConverter(new Engine()); + + private static void ShouldDeclineRatherThanThrow(object value, Type type) + { + var converter = NewConverter(); + var declined = false; + object? converted = null; + + Invoking(() => declined = !converter.TryConvert(value, type, CultureInfo.InvariantCulture, out converted)) + .Should().NotThrow(); + + declined.Should().BeTrue(); + converted.Should().BeNull(); + } + + private static void ConvertShouldStillThrow(object value, Type type) + { + var converter = NewConverter(); + + Invoking(() => converter.Convert(value, type, CultureInfo.InvariantCulture)) + .Should().Throw(); + } + + private static object[] ArrayWithUnconvertibleElement() => [new Widget()]; + + private static Dictionary DictionaryWithUnconvertibleValue() => new() { ["part"] = new Widget() }; + + private static Dictionary DictionaryForPoco() => new() { ["Part"] = new Widget() }; + + [Test] + public void AnArrayWithAnUnconvertibleElementDeclines() + { + ShouldDeclineRatherThanThrow(ArrayWithUnconvertibleElement(), typeof(Gadget[])); + } + + [Test] + public void AListWithAnUnconvertibleItemDeclines() + { + ShouldDeclineRatherThanThrow(ArrayWithUnconvertibleElement(), typeof(List)); + } + + [Test] + public void ACollectionWithAnUnconvertibleItemDeclines() + { + ShouldDeclineRatherThanThrow(ArrayWithUnconvertibleElement(), typeof(Collection)); + } + + [Test] + public void ADictionaryWithAnUnconvertibleValueDeclines() + { + ShouldDeclineRatherThanThrow(DictionaryWithUnconvertibleValue(), typeof(Dictionary)); + } + + [Test] + public void APocoWithAnUnconvertibleMemberDeclines() + { + ShouldDeclineRatherThanThrow(DictionaryForPoco(), typeof(Holder)); + } + + [Test] + public void ConvertStillThrowsForAnArray() + { + ConvertShouldStillThrow(ArrayWithUnconvertibleElement(), typeof(Gadget[])); + } + + [Test] + public void ConvertStillThrowsForAList() + { + ConvertShouldStillThrow(ArrayWithUnconvertibleElement(), typeof(List)); + } + + [Test] + public void ConvertStillThrowsForACollection() + { + ConvertShouldStillThrow(ArrayWithUnconvertibleElement(), typeof(Collection)); + } + + [Test] + public void ConvertStillThrowsForADictionary() + { + ConvertShouldStillThrow(DictionaryWithUnconvertibleValue(), typeof(Dictionary)); + } + + [Test] + public void ConvertStillThrowsForAPoco() + { + ConvertShouldStillThrow(DictionaryForPoco(), typeof(Holder)); + } + + [Test] + public void AConvertibleCompositeStillConverts() + { + // The control: nothing about a composite whose parts do convert changes. + var converter = NewConverter(); + + converter.TryConvert(new object[] { 1d, 2d }, typeof(int[]), CultureInfo.InvariantCulture, out var array) + .Should().BeTrue(); + ((int[]) array!).Should().Equal(1, 2); + + converter.TryConvert(new object[] { 1d, 2d }, typeof(List), CultureInfo.InvariantCulture, out var list) + .Should().BeTrue(); + ((List) list!).Should().Equal(1, 2); + + converter.TryConvert(new Dictionary { ["Part"] = "1" }, typeof(Dictionary), CultureInfo.InvariantCulture, out var dictionary) + .Should().BeTrue(); + ((Dictionary) dictionary!)["Part"].Should().Be(1); + } +} diff --git a/Jint/Runtime/Interop/AGENTS.md b/Jint/Runtime/Interop/AGENTS.md index 313128886c..d3c70ad2b7 100644 --- a/Jint/Runtime/Interop/AGENTS.md +++ b/Jint/Runtime/Interop/AGENTS.md @@ -74,7 +74,7 @@ rest of the list is split across the files indexed from the repository-root [`AG - **Deriving from `DefaultTypeConverter` used to disarm three lanes, and nothing said so.** The gate was `converter.GetType() == typeof(DefaultTypeConverter)` — an exact type test — so the elegant and obvious thing to write, a subclass that adjusts one conversion and inherits the rest, was indistinguishable from a converter that replaces everything. It is now the declaration that decides, so a subclass declaring what it changes keeps every lane for everything else. Two things follow. The **compiled method-invoker lane** is gated on both converter kinds at once: the object-converter filter answers for the return value and the type-converter filter for the parameter types, and either can decline it. And a `ClrTypeConverter` installed *after* construction (through `Engine.TypeConverter`) is honoured, which a value captured once into `InteropResolutionProfile` was not. - **A wrapper's prototype comes from the engine's *running* realm, so building it is realm-scoped work.** `JsValue.FromObject` reaches `ObjectInstance`'s base constructor, `TypeReference.CreateTypeReference` reaches `TypeReferencePrototype`, `DelegateWrapper` reads it outright — all three take `engine.Realm.Intrinsics`, which is `ExecutionContext.Realm` and therefore whichever realm happens to be current at the call. That is right for `Engine.SetValue`, where the realm being written *is* the current one, and wrong for anything projecting into a second realm: `ShadowRealm.SetValue` converted against the principal realm and installed on the shadow realm's global, so a host object handed to a realm was not `instanceof Object` inside it ([#3325](https://github.com/sebastienros/jint/issues/3325)). Jint has exactly one realm-scoped construction path — push that realm's `ExecutionContext` for the duration (`ShadowRealm.EnterRealm`, and `ShadowRealmImportValue` before it) — and no lighter one; `Engine._realmInConstruction` is not it, being neither nestable nor exception-safe. Three host-facing base constructors deliberately do **not** follow the running realm: `ClrFunction(Engine, …)`, `HostFunction` and `Constructor(Engine, string)` pin `engine._originalIntrinsics`, because a function a host builds against an `Engine` belongs to the realm the surrounding script can reach whatever was running when it was built ([#2893](https://github.com/sebastienros/jint/pull/2893)). The two rules do not conflict — one is about a realm named by the caller, the other about a realm that merely happened to be current — but a member built through those constructors *inside* a wrapper (its `toJSON`, its `Symbol.dispose`) still lands in the principal realm, which realm-scoping the wrapper does not change. - **Array-like is not indexable, and `TypeDescriptor.IsArrayLike` is the weaker of the two.** It means the target has a `Count`, nothing more: `ICollection`, `ICollection` and `IReadOnlyCollection` are count-and-copy contracts with no index in them, so `Queue`, `Stack`, `LinkedList`, `SortedSet` and `HashSet` are all array-like with no element at index 0. Any lane that *reads by index* must gate on `ObjectWrapper.HasIndexedElements` instead — `ArrayOperations.For` gated on array-likeness and a bare `ICollection` and handed `IndexWrappedOperations` a target it then hard-cast to `IList`, which is [#3302](https://github.com/sebastienros/jint/issues/3302): a raw `InvalidCastException` out of `Evaluate` for every `Array.prototype` generic over a `Queue`. Falling through to `ObjectOperations` is not a consolation prize — it asks the *object*, so a host collection that really does have an integer indexer still gets its elements through the reflected accessor, and only a genuinely index-less one reads `undefined` per index, which is what an array-like with no index properties means. The same rule governs `ObjectWrapper.HasOriginalIterator`: a wrapper's `Symbol.iterator` is never the array iterator (it enumerates the CLR target), so the index-reading fast path it enables for array destructuring may only stand in for `GetIterator` where index reads reproduce what enumeration yields — which is exactly `HasIndexedElements`. -- **Overload scoring's last rule is the converter's own answer, and it has to be.** `InteropHelper.CalculateMethodParameterScore` rates an argument against a parameter with a dozen structural rules, and everything they did not recognize scored a blanket 100 — "will rarely succeed". `FindBestMatch` discards only a *negative* score, so 100 is a match, and a candidate the argument can never bind to is the *best* one whenever it is the only one. That is survivable where the caller retries — `MethodInfoFunction.TryCall` asks `converter.TryConvert` per candidate and moves on when it declines — and not where the caller takes the first match and stops, which `JintBinaryExpression.TryOperatorOverloading` and `TypeReference`'s constructor selection both do: `'s' + v` selected `op_Addition(T, T)` and died converting the string instead of concatenating, for any host type whose only `+` is `(T, T)` ([#3407](https://github.com/sebastienros/jint/issues/3407)). The last rule now asks the installed `ClrTypeConverter` — the very one `MethodDescriptor.Call` will use — so the score cannot claim a conversion the call then fails to perform, and only what it *confirms* keeps the 100. Three things follow. **What the 100 was protecting is three shapes no structural rule can see**: a JS function to a delegate parameter, an enum parameter given a number outside its defined members, and a conversion operator declared on the **target** type — the operator scan above it reads the *argument's* type only, while `DefaultTypeConverter.TryCastWithOperators` reads both. Each is pinned in `Jint.Tests.PublicInterface/HostOverloadScoringTests.cs`, and deleting the probe fails exactly those three and nothing else in the suite. A parameter type still carrying **open** type parameters (`T`, `Func`) is the one thing the converter cannot be asked — the closed type does not exist until `MethodInfoFunction.ResolveMethod` builds it, and handing an open one over is an `ArgumentException` rather than an answer — so those keep the undecided 100. And the probe *performs* the conversion rather than predicting it, so a user-defined `op_Implicit` on a candidate that is then selected runs twice; the `CanChangeType` rule above it already converts speculatively, so that is this function's established cost rather than a new one, but it is why nothing here may assume a conversion happens once. +- **Overload scoring's last rule is the converter's own answer, and it has to be.** `InteropHelper.CalculateMethodParameterScore` rates an argument against a parameter with a dozen structural rules, and everything they did not recognize scored a blanket 100 — "will rarely succeed". `FindBestMatch` discards only a *negative* score, so 100 is a match, and a candidate the argument can never bind to is the *best* one whenever it is the only one. That is survivable where the caller retries — `MethodInfoFunction.TryCall` asks `converter.TryConvert` per candidate and moves on when it declines — and not where the caller takes the first match and stops, which `JintBinaryExpression.TryOperatorOverloading` and `TypeReference`'s constructor selection both do: `'s' + v` selected `op_Addition(T, T)` and died converting the string instead of concatenating, for any host type whose only `+` is `(T, T)` ([#3407](https://github.com/sebastienros/jint/issues/3407)). The last rule now asks the installed `ClrTypeConverter` — the very one `MethodDescriptor.Call` will use — so the score cannot claim a conversion the call then fails to perform, and only what it *confirms* keeps the 100. Three things follow. **What the 100 was protecting is three shapes no structural rule can see**: a JS function to a delegate parameter, an enum parameter given a number outside its defined members, and a conversion operator declared on the **target** type — the operator scan above it reads the *argument's* type only, while `DefaultTypeConverter.TryCastWithOperators` reads both. Each is pinned in `Jint.Tests.PublicInterface/HostOverloadScoringTests.cs`, and deleting the probe fails exactly those three and nothing else in the suite. A parameter type still carrying **open** type parameters (`T`, `Func`) is the one thing the converter cannot be asked — the closed type does not exist until `MethodInfoFunction.ResolveMethod` builds it, and handing an open one over is an `ArgumentException` rather than an answer — so those keep the undecided 100. And the probe *performs* the conversion rather than predicting it, so a user-defined `op_Implicit` on a candidate that is then selected runs twice; the `CanChangeType` rule above it already converts speculatively, so that is this function's established cost rather than a new one, but it is why nothing here may assume a conversion happens once. **A JavaScript array short-circuited *above* that rule**, so none of it ever reached an array or collection parameter: "is an array, wants an array" answered a flat 2 for every candidate, so they tied and declaration order decided - while the composite branches of `TryConvert` converted their parts through the throwing `Convert` and so escaped the very candidate loop that makes a 100 survivable ([#3754](https://github.com/sebastienros/jint/issues/3754)). The elements are scored now, worst-of-eight, with `object` and `JsValue` element types carved out because rating theirs would rank a `params` catch-all below the scalar overload beside it. Pinned in `Jint.Tests.PublicInterface/HostArrayElementOverloadTests.cs` and `Jint.Tests/Runtime/Interop/CompositeConversionDeclineTests.cs`. - **A numeric score states its bound in what the conversion accepts, not in what the type is called.** The same rule one level up, and it cost the same two lanes. `CalculateMethodParameterScore` rated an integral number a **perfect** 0 against an `int` parameter and a 2 against a `long` one at *any* magnitude, while `float`, `short`, `ushort`, `byte` and `sbyte` all checked the range - so `3000000000` was a perfect match for `int`, and a perfect score also ends `FindBestMatch` with a one-element candidate set. On the two lanes that take the first match and stop the `OverflowException` left through the embedder's `Evaluate`; on the one that retries it became `No public methods with the specified arguments were found.`, the `object` overload beside it never scored ([#3577](https://github.com/sebastienros/jint/issues/3577)). `int` and `long` gate through `FitsInt32`/`FitsInt64`, the very predicates `TryConvertNumberFast` converts by, and the narrow checks read the double rather than a `(int)` cast of it that truncated before they read it - and differently on .NET and .NET Framework. Pinned in `Jint.Tests.PublicInterface/HostNumericRangeOverloadTests.cs`. - **An index-shaped key on a wrapped bounded collection is the wrapper's to answer, in every lane.** The reflected indexer parses an index out of whatever key it is handed and takes it to the collection, so an out-of-range `x[3] = 9` was the CLR's own `ArgumentOutOfRangeException` out of `Evaluate` — invisible to a script `try`/`catch` and to a host `catch (JavaScriptException)` alike. `ArrayLikeWrapper` owns those keys now ([#3384](https://github.com/sebastienros/jint/issues/3384)), *including* the descriptor lane ([#3423](https://github.com/sebastienros/jint/issues/3423)): `Get`, `Set`, `HasProperty`, `Delete`, `GetOwnProperty`, `ProbeOwnProperty`, `DefineOwnProperty` and both key enumerations all answer from `Length`, and a lane added later that does not is a lane where `in` and `hasOwnProperty` contradict each other — which they may not, `OrdinaryHasProperty` being defined in terms of `[[GetOwnProperty]]`. A **plain** `ObjectWrapper` over a bounded target has no view and reads the target's own count instead ([#3422](https://github.com/sebastienros/jint/issues/3422)); that check is deliberately conditioned on two facts at once, because the same lane serves `Dictionary`, where `d[99] = "x"` is a legitimate add, and a string-keyed indexer on a collection, where `x["3"]` names a key rather than a position. `ObjectWrapper.ClassifyElementKey` is the one definition of "index-shaped key" and lives on the base class for that reason; a second copy is the thing that would drift. Owning the key also means owning the **containment** decision the reflected lane used to make for free: a view resolves no member per access, so `TypeResolver.MemberFilter` has to be asked about the indexer the lanes stand for, once per (resolver, type), and `ArrayLikeWrapper._elementsExposed` is that answer ([#3558](https://github.com/sebastienros/jint/issues/3558)). It is asked **before** `CanWrite`/`IsFixedSize`, because containment decides whether there is a property at all and writability only what may be done to one — a fixed-size array whose indexer is hidden must report "no such property", not the `TypeError` naming its bounds. `HasIndexedElements` carries it, so a hidden element lane routes every `Array.prototype` generic to `ObjectOperations` exactly as a `Queue` is routed. What the filter does *not* speak for, and must not be extended to without a decision: `length` (produced from `Count`, filtered separately), iteration (`GetEnumerator`), and `ArrayConversionMode.Copy`, which converts a `T[]` before any member is touched. diff --git a/Jint/Runtime/Interop/DefaultTypeConverter.cs b/Jint/Runtime/Interop/DefaultTypeConverter.cs index dfd2b75328..59e30288a6 100644 --- a/Jint/Runtime/Interop/DefaultTypeConverter.cs +++ b/Jint/Runtime/Interop/DefaultTypeConverter.cs @@ -102,6 +102,53 @@ public override bool TryConvert( return TryConvertInternal(value, type, formatProvider, propagateException: false, out converted, out _); } + /// + /// Converts one part of a composite - an array element, a collection item, a target dictionary's value, + /// a member of a POCO built from a dictionary - under the same + /// contract as the frame that is assembling the composite. + /// + /// + /// Every one of those sites reached for the public, throwing whatever its own frame + /// had been asked, so a part that could not be converted escaped as a CLR + /// exception rather than as the that method documents - and an exception is not + /// something MethodInfoFunction.Call can move on from: it tries candidates in score order and + /// declines its way to the next one, so a throwing conversion ends the call rather than the candidate + /// (#3754). The body mirrors + /// so that behaviour under is unchanged: the + /// virtual first, so a subclass override still answers for the parts, and only + /// then the internal pipeline that produces the detailed message and honours + /// . + /// + private bool TryConvertPart( + object? value, + [DynamicallyAccessedMembers(InteropHelper.DefaultDynamicallyAccessedMemberTypes)] Type type, + IFormatProvider formatProvider, + bool propagateException, + out object? converted, + out string? problemMessage) + { + problemMessage = null; + + if (TryConvert(value, type, formatProvider, out converted)) + { + return true; + } + + if (!propagateException) + { + converted = null; + problemMessage = $"Unable to convert a value of type '{value?.GetType()}' to '{type}'"; + return false; + } + + if (!TryConvertInternal(value, type, formatProvider, propagateException: true, out converted, out problemMessage)) + { + Throw.Error(_engine, problemMessage ?? $"Unable to convert {value} to type {type}"); + } + + return true; + } + private static readonly ConditionalWeakTable>> _targetBinderDelegateCache = new(); private static readonly ConditionalWeakTable> _boundTargetDelegateCache = new(); private static readonly ConditionalWeakTable _hostCallbackDelegates = new(); @@ -224,7 +271,12 @@ private bool TryConvertInternal( var targetList = (IList) Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType))!; foreach (var item in sourceArray) { - targetList.Add(item is null ? null : Convert(item, elementType, formatProvider)); + if (!TryConvertPart(item, elementType, formatProvider, propagateException, out var convertedItem, out problemMessage)) + { + return false; + } + + targetList.Add(convertedItem); } converted = targetList; return true; @@ -236,7 +288,12 @@ private bool TryConvertInternal( var innerList = (IList) Activator.CreateInstance(innerListType)!; foreach (var item in sourceArray) { - innerList.Add(item is null ? null : Convert(item, elementType, formatProvider)); + if (!TryConvertPart(item, elementType, formatProvider, propagateException, out var convertedItem, out problemMessage)) + { + return false; + } + + innerList.Add(convertedItem); } converted = Activator.CreateInstance(type, innerList)!; return true; @@ -343,7 +400,10 @@ private bool TryConvertInternal( var itemsConverted = new object?[source.Length]; for (var i = 0; i < source.Length; i++) { - itemsConverted[i] = Convert(source[i], targetElementType, formatProvider); + if (!TryConvertPart(source[i], targetElementType, formatProvider, propagateException, out itemsConverted[i], out problemMessage)) + { + return false; + } } var result = Array.CreateInstance(targetElementType, source.Length); itemsConverted.CopyTo(result, 0); @@ -423,7 +483,12 @@ private bool TryConvertInternal( { if (typeDescriptor.TryGetDictionaryValue(value, key, out var sourceVal)) { - targetDict[key] = Convert(sourceVal, targetValueType, formatProvider); + if (!TryConvertPart(sourceVal, targetValueType, formatProvider, propagateException, out var convertedValue, out problemMessage)) + { + return false; + } + + targetDict[key] = convertedValue; } } } @@ -436,28 +501,48 @@ private bool TryConvertInternal( // each of them only to discard it. Same public instance-and-static set. foreach (var member in type.GetProperties()) { - CopyDictionaryEntryToMember(this, typeDescriptor, value, obj, member, formatProvider); + if (!CopyDictionaryEntryToMember(this, typeDescriptor, value, obj, member, formatProvider, propagateException, out problemMessage)) + { + return false; + } } foreach (var member in type.GetFields()) { - CopyDictionaryEntryToMember(this, typeDescriptor, value, obj, member, formatProvider); + if (!CopyDictionaryEntryToMember(this, typeDescriptor, value, obj, member, formatProvider, propagateException, out problemMessage)) + { + return false; + } } - static void CopyDictionaryEntryToMember( + // propagateException is threaded in as a parameter because this is a static local function, + // and it has to be threaded in at all for the same reason the other four composite sites take + // it: a member the dictionary supplies but the target cannot hold is a decline of the whole + // conversion, not an exception out of a Try method. + static bool CopyDictionaryEntryToMember( DefaultTypeConverter converter, TypeDescriptor typeDescriptor, object value, object target, MemberInfo member, - IFormatProvider formatProvider) + IFormatProvider formatProvider, + bool propagateException, + out string? problemMessage) { + problemMessage = null; + if (typeDescriptor.TryGetDictionaryValue(value, member.Name, out var val) || typeDescriptor.TryGetDictionaryValue(value, member.Name.UpperToLowerCamelCase(), out val)) { - var output = converter.Convert(val, member.GetDefinedType(), formatProvider); + if (!converter.TryConvertPart(val, member.GetDefinedType(), formatProvider, propagateException, out var output, out problemMessage)) + { + return false; + } + member.SetValue(target, output); } + + return true; } } diff --git a/Jint/Runtime/Interop/InteropHelper.cs b/Jint/Runtime/Interop/InteropHelper.cs index 0d1b39e069..63023d1cb2 100644 --- a/Jint/Runtime/Interop/InteropHelper.cs +++ b/Jint/Runtime/Interop/InteropHelper.cs @@ -160,9 +160,16 @@ internal static AssignableResult IsAssignableToGenericType( /// Determines how well parameter type matches target method's type. /// private static int CalculateMethodParameterScore(Engine engine, ParameterInfo parameter, JsValue parameterValue) - { - var paramType = parameter.ParameterType; + => CalculateParameterTypeScore(engine, parameter.ParameterType, parameter.IsOptional, parameterValue); + /// + /// The same question asked of a bare , so that a value with no + /// of its own - an element of a JavaScript array being rated against an array or collection parameter - + /// is scored by the very rules its parameter would be. Only the parameter's type and its optionality were + /// ever read, and an element is never optional. + /// + private static int CalculateParameterTypeScore(Engine engine, Type paramType, bool isOptional, JsValue parameterValue) + { // Special case: if parameter expects a JsValue-derived type (e.g., TypeReference), // check if the argument is of that exact type before calling ToObject(). // This is important because ToObject() unwraps TypeReference to System.Type, @@ -191,7 +198,7 @@ private static int CalculateMethodParameterScore(Engine engine, ParameterInfo pa if (objectValue is null) { - if (!parameter.IsOptional && !TypeIsNullable(paramType)) + if (!isOptional && !TypeIsNullable(paramType)) { // this is bad return -1; @@ -289,10 +296,9 @@ parameterValue is JsNumber jsNumber return 1; } - if (parameterValue.IsSpecArray() && (paramType.IsArray || IsGenericCollectionType(paramType))) + if (parameterValue.IsSpecArray() && TryGetCollectionElementType(paramType, out var elementType)) { - // we have potential, TODO if we'd know JS array's internal type we could have exact match - return 2; + return CalculateArrayParameterScore(engine, elementType, parameterValue); } // not sure the best point to start generic type tests @@ -350,6 +356,91 @@ parameterValue is JsNumber jsNumber return -1; } + /// What an array or collection parameter is worth before its elements are read. + private const int ArrayParameterBaseScore = 2; + + /// How many of a JavaScript array's elements are rated against the parameter's element type. + private const int MaxScoredArrayElements = 8; + + /// + /// Rates a JavaScript array against an array or generic-collection parameter by the elements it actually + /// holds, so that overloads differing only in element type are told apart rather than tied. + /// + /// + /// + /// The score is the base plus the worst element, never their sum, so an array parameter's + /// contribution stays the same magnitude as a scalar's and a long array cannot outweigh the parameters + /// beside it. Exact elements therefore still answer 2, which is what every array parameter answered + /// before. It also never answers 0: a perfect score ends outright + /// with a one-element candidate set, and an array argument has never been a perfect match. + /// + /// + /// Only the first elements are read, and the cap is safe in both + /// directions. A tail that would have scored worse only reorders the candidates - the conversion still + /// validates every element, and since the composite branches of DefaultTypeConverter.TryConvert + /// now decline instead of throwing, a wrong order costs a decline and a move to the next candidate rather + /// than a failed call. A tail that would have scored better cannot promote a candidate past a rival the + /// prefix already ruled out, because a -1 is returned on the first element that cannot bind at all. + /// + /// + /// That -1 is the converter's own answer, reached through the same ladder + /// MethodDescriptor.Call will use to perform the conversion - the + /// #3407 invariant applied one level + /// down, so the score cannot claim an element conversion the call then fails to perform. The recursion + /// terminates because each level strips one array rank off the parameter type, which is finite. + /// + /// + private static int CalculateArrayParameterScore(Engine engine, Type elementType, JsValue parameterValue) + { + // CARVE-OUT: an element type any JsValue is already assignable to - object, JsValue itself, or a base + // of it - answers for every element there could be, so reading them buys nothing and costs the + // candidate its place. params object[] and params JsValue[] are what a host writes for "anything", + // and rating their elements (5 for the catch-all object, 1 for an is-a JsValue) would hand the call + // to whatever scalar overload sits beside them. + if (elementType.IsAssignableFrom(typeof(JsValue))) + { + return ArrayParameterBaseScore; + } + + // A Proxy reports itself as a spec array; reading its elements here would run its traps - user + // JavaScript - during overload scoring, so it keeps the base score the way it always had. + if (parameterValue is not Jint.Native.Array.ArrayInstance array) + { + return ArrayParameterBaseScore; + } + + var length = array.GetLength(); + if (length == 0) + { + // nothing to read: the candidates are genuinely indistinguishable and the base score is all there is + return ArrayParameterBaseScore; + } + + var scoredElements = length < MaxScoredArrayElements ? length : (uint) MaxScoredArrayElements; + var worstElementScore = 0; + for (uint i = 0; i < scoredElements; i++) + { + if (!array.TryGetValue(i, out var element)) + { + element = JsValue.Undefined; + } + + var elementScore = CalculateParameterTypeScore(engine, elementType, isOptional: false, element); + if (elementScore < 0) + { + // an element this parameter cannot hold makes the whole array unbindable to it + return -1; + } + + if (elementScore > worstElementScore) + { + worstElementScore = elementScore; + } + } + + return ArrayParameterBaseScore + worstElementScore; + } + /// /// Method's match score tells how far away it's from ideal candidate. 0 = ideal, bigger the the number, /// the farther away the candidate is from ideal match. Negative signals impossible match. @@ -421,6 +512,28 @@ internal static bool IsGenericCollectionType(Type type) && GenericCollectionTypeDefinitions.Contains(type.GetGenericTypeDefinition()); } + /// + /// The element type an array or single-argument generic collection parameter holds, which is what + /// rates a JavaScript array's elements against. + /// + private static bool TryGetCollectionElementType(Type type, [NotNullWhen(true)] out Type? elementType) + { + if (type.IsArray) + { + elementType = type.GetElementType(); + return elementType is not null; + } + + if (IsGenericCollectionType(type)) + { + elementType = type.GetGenericArguments()[0]; + return true; + } + + elementType = null; + return false; + } + internal static bool TypeIsNullable(Type type) { return !type.IsValueType || Nullable.GetUnderlyingType(type) != null; diff --git a/docs/v5-migration.md b/docs/v5-migration.md index 7bc3776067..f0648ff0c7 100644 --- a/docs/v5-migration.md +++ b/docs/v5-migration.md @@ -5256,6 +5256,76 @@ that accessor's value — in a console line, in a `console.table` cell, in `%o`/ sets `this.name` in its constructor, because that is an own data property. A host that wants the accessor's value calls it itself and logs the string. +### 4.124 An array parameter is chosen by the elements it holds, and a failing element declines ([#3754](https://github.com/sebastienros/jint/issues/3754)) + +A `params` call bundles its trailing arguments into one JavaScript array before overload resolution runs, so +candidates differing only in the element type of an array or collection parameter were rated by a single rule +— "is an array, wants an array" — that answered the same number for every one of them. The element type was +never consulted, the converter probe below that rule (§4.33) was never reached, and declaration order decided. + +```csharp +public sealed class MathHost +{ + public CDecimal Add(params CDecimal[] args) => …; + public CInteger Add(params CInteger[] args) => …; + public CLong Add(params CLong[] args) => …; +} + +public sealed class Host +{ + public string Join(params string[] values) => string.Concat(values); + public string Join(params int[] values) => Sum(values).ToString(); +} +``` + +```js +// 5.0 5.x +math.Add(a, b); // System.InvalidCastException the CInteger overload +h.Join(1, 2); // "12" — the string overload "3" — the int overload +math.Add('text'); // System.InvalidCastException TypeError: No public methods… +``` + +Up to the first eight elements are now rated against the parameter's element type, and the parameter scores +the base it always did plus its **worst** element, so an exact-typed array answers exactly what it answered +before and a long array cannot outweigh the parameters beside it. An element no conversion can produce makes +the whole candidate unbindable, which is the same rule §4.33 applies one level up: the last word belongs to +the converter that will actually perform the conversion. An **empty** array is deliberately unchanged — there +is no element to read, the candidates are genuinely indistinguishable, and today's declaration-order answer +stands. + +Two element types are carved out and keep the flat score: `object` and `JsValue` — that is, anything a +`JsValue` is already assignable to. `params object[]` and `params JsValue[]` are what a host writes for +"anything", and rating their elements would rank them below whatever scalar overload sits beside them. + +The second half is in the converter. `DefaultTypeConverter.TryConvert` converted the *parts* of a composite — +a `List` or `Collection` item, a `T[]` element, a target dictionary's value, a member of a POCO built +from a dictionary — through the public, throwing `Convert`, whatever its own frame had been asked. So a method +documented as returning `false` threw a CLR exception, which is not something the candidate loop in +`MethodInfoFunction.Call` can move on from: it tries candidates in score order and declines its way to the +next, and an exception ended the call instead of the candidate. Those five sites now honour the flag their +frame was called with. + +**What could break.** Three things, all of them narrow. + +A host API overloaded on the element type of an array or collection parameter now selects by that element +type, so a call that previously reached the first-declared overload — and either answered from it or died +converting to it — reaches the one the arguments actually fit. There is no switch that restores the old +selection. + +`DefaultTypeConverter.TryConvert` returns `false` for a composite with an unconvertible part where it used to +throw. `Convert` is unchanged and still throws the very same exception for the very same input, so code that +wanted the exception asks for it by name. A subclass overriding `TryConvert` is now also consulted for the +*parts* of a composite, which it was not before — it was consulted only for the composite as a whole, the +parts going through the base `Convert`. + +An overloaded call whose arguments no candidate accepts now raises the ordinary catchable interop resolution +error rather than whatever the first candidate's conversion threw, and the CLR exception's detail is no longer +in the message. The argument and candidate types are, once the host asks: + +```csharp +var engine = new Engine(options => options.Interop.ExposeDetailedResolutionErrors = true); +``` + ## 5. New in v5 Everything in the table below is opt-in: nothing in it is installed unless the host asks for it, so