Fix DefaultTypeConverter.Convert bypassing subclass TryConvert overrides

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<string, object?> 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<Point>(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<Point>? receivedList = null;
+        engine.SetValue("processArray", new Action<Point[]>(p => receivedArray = p));
+        engine.SetValue("processList", new Action<List<Point>>(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<string, object?> dict = new ExpandoObject();
+        dict["x"] = 1;
+        dict["y"] = 2;
+
+        var point = Assert.IsType<Point>(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<JavaScriptException>(() => 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<int>(_ => { }));
+
+        Assert.Throws<FormatException>(() => engine.Execute("callInt('not a number')"));
+    }
+}

Jint/Runtime/Interop/DefaultTypeConverter.cs

@@ -54,31 +54,48 @@ public DefaultTypeConverter(Engine engine)
         _engine = engine;
     }
 
+    /// <summary>
+    /// Converts value to the given type, throwing if the conversion cannot be done.
+    /// Dispatches through the virtual <see cref="TryConvert"/> 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 (<see cref="Options.InteropOptions.ExceptionHandler"/>).
+    /// </summary>
     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;
     }
 
+    /// <summary>
+    /// Converts value to the given type, returning false if the conversion cannot be done.
+    /// This is the extension point for custom conversions: both <see cref="Convert"/> and the engine's
+    /// interop paths dispatch through it. Overrides should call <c>base.TryConvert</c> as the fallback;
+    /// do not call <see cref="Convert"/> from an override as that would cause infinite recursion.
+    /// </summary>
     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<IFunction, Func<object, Delegate>> _targetBinderDelegateCache = new();
     private static readonly ConditionalWeakTable<object, Delegate> _boundTargetDelegateCache = new();
 
-    private bool TryConvert(
+    private bool TryConvertInternal(
         object? value,
         [DynamicallyAccessedMembers(InteropHelper.DefaultDynamicallyAccessedMemberTypes)] Type type,
         IFormatProvider formatProvider,