Skip to content

[patch] Skip conversion methods that cannot produce the type, so a working Parse is used - #114

Merged
matt-edmondson merged 1 commit into
mainfrom
fix/reject-unusable-conversion-methods
Oct 7, 2026
Merged

matt-edmondson merged 1 commit into
mainfrom
fix/reject-unusable-conversion-methods

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #104

What changed

IsUsableConversionMethod accepted two kinds of method that can never work, and because FromString is tried before Parse, either one hid a usable Parse(string):

  1. A method returning a different closed type. The return type only had to share T's generic type definition, so Box<string> FromString(string) was accepted for Box<int>. Every read then threw InvalidCastException.
  2. An open generic method. It was accepted without being closed and was only closed later, in the converter's static initializer, with MakeGenericMethod(typeof(T)). When T broke a constraint, that threw TypeInitializationException and broke RoundTripStringJsonConverter<T> for the rest of the process.

FindStringConversionMethod now does the following:

  • It closes each generic candidate over the type while searching, in the new CloseOverType helper, and drops it if closing throws. That happens when a constraint is violated or the method has more than one type parameter.
  • IsUsableConversionMethod now requires the return type to be exactly the type.
  • FindStringConversionMethod returns the closed method, so the converter uses it directly and CreateStringConversionMethod is gone.
  • The catch (AmbiguousMatchException) block is removed. It could not be reached, and it carried a second copy of the loose return-type check. A stale comment in EdgeCaseTests that referred to it is updated.

The existing TestGenericClass<int>.FromString<TSelf> case still resolves, because closing it over the type yields a method that returns exactly that type.

Tests

Two tests in ConversionMethodPriorityTests cover the two shapes from the issue:

  • BoxWithMismatchedFromString<int>: FromString returns the <string> closure
  • CodeWithConstrainedGenericFromString: FromString<TEncoding> has a struct, ITestEncoding constraint

Both now deserialize through Parse. With the library change reverted, both tests fail, with InvalidCastException and TypeInitializationException respectively. With the change, the full suite passes (121/121, net10.0), and the library builds for all its target frameworks.

This PR is independent of #113 (stack-trace preservation). The two edit different parts of RoundTripStringJsonConverter.cs.

🤖 Generated with Claude Code

https://claude.ai/code/session_013JBDCsjuRez5zdzBcBJ7Y7


Generated by Claude Code

…rking Parse is used

IsUsableConversionMethod accepted a method whose return type only shared
T's generic type definition (Box<string>.FromString for Box<int>), and
open generic methods that were closed over T only later, in the
converter's static initializer. Either one won over a usable Parse(string)
and then failed on every read: an InvalidCastException, or a
TypeInitializationException that broke the converter for the rest of the
process.

Generic candidates are now closed over the type while searching, and
dropped if that fails; the return type must be exactly the type. The
unreachable AmbiguousMatchException fallback, which carried a second copy
of the loose return-type check, is removed.

Fixes #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013JBDCsjuRez5zdzBcBJ7Y7

Copy link
Copy Markdown
Contributor Author

CI is blocked by runner availability, not by this change. In run 37363469055, ci / Classify repository (ubuntu-latest) waited 15 minutes for a runner and was cancelled before any step ran. That happened on the re-run (attempt 2). On the first attempt the job was ci / .NET / Discover Test Projects, and it was cancelled the same way. Either way, the .NET build and test jobs never ran.

#113 here and ktsu-dev/Sorting#65 show the same cancellation at the same time, so it looks like an org-wide Actions capacity or limit issue. Locally the full suite passes (121/121) on this branch. Once runners are available, CI needs to be re-run.


Generated by Claude Code

@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants