From e851aaaf549aee1dce748d27f398f8f58cace1f9 Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Sun, 7 Jun 2026 09:56:00 +0300 Subject: [PATCH] Fix DefaultTypeConverter.Convert bypassing subclass TryConvert overrides Convert called the private TryConvert overload directly, which C# binds at compile time, so subclasses overriding the public virtual TryConvert were silently ignored on all Convert-driven interop paths (DelegateWrapper, MethodDescriptor, ReflectionAccessor, nested element conversions). Convert now dispatches through the public virtual TryConvert first and only falls back to the internal pipeline to produce the detailed error message and to honor exception propagation semantics (Options.Interop.ExceptionHandler). The private overload is renamed to TryConvertInternal so the silent overload binding cannot recur. Fixes #2495 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Runtime/DefaultTypeConverterTests.cs | 105 ++++++++++++++++++ Jint/Runtime/Interop/DefaultTypeConverter.cs | 23 +++- 2 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 Jint.Tests/Runtime/DefaultTypeConverterTests.cs diff --git a/Jint.Tests/Runtime/DefaultTypeConverterTests.cs b/Jint.Tests/Runtime/DefaultTypeConverterTests.cs new file mode 100644 index 000000000..a2800eeeb --- /dev/null +++ b/Jint.Tests/Runtime/DefaultTypeConverterTests.cs @@ -0,0 +1,105 @@ +#nullable enable + +using System.Diagnostics.CodeAnalysis; +using System.Dynamic; +using System.Globalization; +using Jint.Runtime; +using Jint.Runtime.Interop; +using Jint.Tests.Runtime.Domain; + +namespace Jint.Tests.Runtime; + +public class DefaultTypeConverterTests +{ + private record Point(int X, int Y); + + // Mirrors the scenario from https://github.com/sebastienros/jint/issues/2495 - overriding only + // TryConvert should be enough to intercept conversions that are triggered via Convert. + private sealed class PointTypeConverter(Engine engine) : DefaultTypeConverter(engine) + { + public override bool TryConvert(object? value, Type type, IFormatProvider formatProvider, [NotNullWhen(true)] out object? converted) + { + if (type == typeof(Point) && value is IDictionary d) + { + converted = new Point( + X: System.Convert.ToInt32(d["x"], formatProvider), + Y: System.Convert.ToInt32(d["y"], formatProvider)); + return true; + } + + return base.TryConvert(value, type, formatProvider, out converted); + } + } + + private static Engine CreateEngine() => new(options => options.SetTypeConverter(e => new PointTypeConverter(e))); + + [Fact] + public void ShouldUseOverriddenTryConvertWhenConvertingDelegateArguments() + { + var engine = CreateEngine(); + + Point? received = null; + engine.SetValue("process", new Action(p => received = p)); + + engine.Execute("process({ x: 10, y: 20 });"); + + Assert.Equal(new Point(10, 20), received); + } + + [Fact] + public void ShouldUseOverriddenTryConvertForNestedElementConversions() + { + var engine = CreateEngine(); + + Point[]? receivedArray = null; + List? receivedList = null; + engine.SetValue("processArray", new Action(p => receivedArray = p)); + engine.SetValue("processList", new Action>(p => receivedList = p)); + + engine.Execute("processArray([{ x: 1, y: 2 }, { x: 3, y: 4 }]);"); + engine.Execute("processList([{ x: 5, y: 6 }]);"); + + Assert.Equal(new[] { new Point(1, 2), new Point(3, 4) }, receivedArray); + Assert.Equal(new[] { new Point(5, 6) }, receivedList!); + } + + [Fact] + public void ShouldUseOverriddenTryConvertWhenConvertCalledDirectly() + { + var converter = new PointTypeConverter(new Engine()); + + IDictionary dict = new ExpandoObject(); + dict["x"] = 1; + dict["y"] = 2; + + var point = Assert.IsType(converter.Convert(dict, typeof(Point), CultureInfo.InvariantCulture)); + Assert.Equal(new Point(1, 2), point); + + // built-in conversions still work + Assert.Equal(42, converter.Convert("42", typeof(int), CultureInfo.InvariantCulture)); + } + + [Fact] + public void ShouldReportDetailedErrorWhenOverriddenTryConvertCannotConvert() + { + var engine = new Engine(options => options + .SetTypeConverter(e => new PointTypeConverter(e)) + .CatchClrExceptions()); + + engine.SetValue("a", new Person()); + + var ex = Assert.Throws(() => engine.Execute("a.age = 'not a number'")); + Assert.Contains("input string", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains(" was not in a correct format", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void ShouldPropagateConversionExceptionWhenClrExceptionsAreNotCaught() + { + var engine = CreateEngine(); + + engine.SetValue("callInt", new Action(_ => { })); + + Assert.Throws(() => engine.Execute("callInt('not a number')")); + } +} diff --git a/Jint/Runtime/Interop/DefaultTypeConverter.cs b/Jint/Runtime/Interop/DefaultTypeConverter.cs index 6d6aed2eb..91376f75a 100644 --- a/Jint/Runtime/Interop/DefaultTypeConverter.cs +++ b/Jint/Runtime/Interop/DefaultTypeConverter.cs @@ -54,31 +54,48 @@ public DefaultTypeConverter(Engine engine) _engine = engine; } + /// + /// Converts value to the given type, throwing if the conversion cannot be done. + /// Dispatches through the virtual first so that subclass overrides are honored, + /// then falls back to the built-in conversion pipeline to produce a detailed error message and to honor + /// exception propagation semantics (). + /// public virtual object? Convert( object? value, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields)] Type type, IFormatProvider formatProvider) { - if (!TryConvert(value, type, formatProvider, propagateException: true, out var converted, out var problemMessage)) + if (TryConvert(value, type, formatProvider, out var converted)) + { + return converted; + } + + if (!TryConvertInternal(value, type, formatProvider, propagateException: true, out converted, out var problemMessage)) { Throw.Error(_engine, problemMessage ?? $"Unable to convert {value} to type {type}"); } return converted; } + /// + /// Converts value to the given type, returning false if the conversion cannot be done. + /// This is the extension point for custom conversions: both and the engine's + /// interop paths dispatch through it. Overrides should call base.TryConvert as the fallback; + /// do not call from an override as that would cause infinite recursion. + /// public virtual bool TryConvert( object? value, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields)] Type type, IFormatProvider formatProvider, [NotNullWhen(true)] out object? converted) { - return TryConvert(value, type, formatProvider, propagateException: false, out converted, out _); + return TryConvertInternal(value, type, formatProvider, propagateException: false, out converted, out _); } private static readonly ConditionalWeakTable> _targetBinderDelegateCache = new(); private static readonly ConditionalWeakTable _boundTargetDelegateCache = new(); - private bool TryConvert( + private bool TryConvertInternal( object? value, [DynamicallyAccessedMembers(InteropHelper.DefaultDynamicallyAccessedMemberTypes)] Type type, IFormatProvider formatProvider,