Skip to content

Fix DefaultTypeConverter.Convert bypassing subclass TryConvert overrides#2498

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:private-tryconvert-fi
Jun 7, 2026
Merged

Fix DefaultTypeConverter.Convert bypassing subclass TryConvert overrides#2498
lahma merged 1 commit into
sebastienros:mainfrom
lahma:private-tryconvert-fi

Conversation

@lahma

@lahma lahma commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Fixes #2495

Problem

DefaultTypeConverter.Convert called the private TryConvert overload (the one with propagateException/problemMessage parameters). C# binds that overload at compile time, so subclasses overriding the public virtual TryConvert were silently bypassed on every Convert-driven interop path: DelegateWrapper, MethodDescriptor, ReflectionAccessor, ObjectWrapper.Specialized, and nested element conversions inside the converter itself. Overriding TryConvert did work for the probing callers (MethodInfoFunction, ObjectWrapper, IndexerAccessor), making the trap especially confusing — the override appeared half-working.

Fix

Convert now dispatches through the public virtual TryConvert first, so subclass overrides are honored. Only when that returns false does it fall back to the internal pipeline to produce the detailed error message ("No valid constructors found for type …", FormatException text, etc.) and to honor exception propagation semantics (Options.Interop.ExceptionHandler — raw CLR exception still propagates to the host by default, JS error with the original message under CatchClrExceptions()).

The private overload is renamed to TryConvertInternal so the silent overload binding cannot recur, and both methods now have XML docs describing the dispatch contract (override TryConvert, call base.TryConvert as fallback — never Convert, which would recurse).

No public API changes; behavior for the non-subclassed converter is unchanged on success paths, and failure paths produce identical messages/exceptions (the pipeline just runs once more to surface them).

Tests

New DefaultTypeConverterTests covering the issue repro (Action<Point> invoked with a plain JS object through an override-only converter), nested element conversions (Point[], List<Point>), direct Convert calls, detailed-error fidelity under CatchClrExceptions(), and raw exception propagation under default options.

🤖 Generated with Claude Code

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 sebastienros#2495

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lahma
lahma enabled auto-merge (squash) June 7, 2026 06:59
@lahma
lahma merged commit b77fb4f into sebastienros:main Jun 7, 2026
7 of 8 checks passed
@lahma
lahma deleted the private-tryconvert-fi branch June 7, 2026 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DefaultTypeConverter.Convert calls private TryConvert overload, silently ignoring subclass override

1 participant