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

RoundTripStringJsonConverter.Test/ConversionMethodPriorityTests.cs

@@ -110,4 +110,59 @@ public void Factory_Should_Detect_Convertible_Types_With_Multiple_Methods()
 		Assert.IsTrue(factory.CanConvert(typeof(ClassWithParseAndCreate)));
 		Assert.IsTrue(factory.CanConvert(typeof(ClassWithCreateAndConvert)));
 	}
+
+	/// <summary>
+	/// Generic class whose FromString returns a different closed type, so only Parse can produce it.
+	/// </summary>
+	public sealed class BoxWithMismatchedFromString<TValue>(string raw)
+	{
+		public string Raw { get; } = raw;
+
+		public static BoxWithMismatchedFromString<string> FromString(string value) => new($"FromString:{value}");
+
+		public static BoxWithMismatchedFromString<TValue> Parse(string value) => new($"Parse:{value}");
+
+		public override string ToString() => Raw;
+	}
+
+	public interface ITestEncoding;
+
+	public struct HexEncoding : ITestEncoding;
+
+	/// <summary>
+	/// Class whose FromString is generic with a constraint the class itself cannot satisfy, so only Parse can produce it.
+	/// </summary>
+	public sealed class CodeWithConstrainedGenericFromString(string value)
+	{
+		public string Value { get; } = value;
+
+		[System.Diagnostics.CodeAnalysis.SuppressMessage("Major Code Smell", "S2326:Unused type parameters should be removed", Justification = "Testing a constrained generic overload")]
+		public static CodeWithConstrainedGenericFromString FromString<TEncoding>(string value) where TEncoding : struct, ITestEncoding => new($"FromString:{value}");
+
+		public static CodeWithConstrainedGenericFromString Parse(string value) => new($"Parse:{value}");
+
+		public override string ToString() => Value;
+	}
+
+	[TestMethod]
+	public void FromString_Returning_A_Different_Closed_Generic_Type_Should_Fall_Back_To_Parse()
+	{
+		JsonSerializerOptions options = GetOptions();
+
+		BoxWithMismatchedFromString<int>? result = JsonSerializer.Deserialize<BoxWithMismatchedFromString<int>>("\"test\"", options);
+
+		Assert.IsNotNull(result);
+		Assert.AreEqual("Parse:test", result.Raw, "Parse should be used because FromString returns BoxWithMismatchedFromString<string>");
+	}
+
+	[TestMethod]
+	public void Generic_FromString_That_Cannot_Be_Closed_Over_The_Type_Should_Fall_Back_To_Parse()
+	{
+		JsonSerializerOptions options = GetOptions();
+
+		CodeWithConstrainedGenericFromString? result = JsonSerializer.Deserialize<CodeWithConstrainedGenericFromString>("\"test\"", options);
+
+		Assert.IsNotNull(result);
+		Assert.AreEqual("Parse:test", result.Value, "Parse should be used because FromString<TEncoding> cannot be closed over the type");
+	}
 }

RoundTripStringJsonConverter.Test/EdgeCaseTests.cs

@@ -234,7 +234,7 @@ public void Should_Handle_Ambiguous_FromString_By_Falling_Back()
 		JsonSerializerOptions options = GetOptions();
 		string json = "\"abc\"";
 
-		// With ambiguity present, factory's catch(AmbiguousMatchException) path should still resolve a valid method
+		// With overloads present, the factory should still resolve a valid method
 		TypeWithAmbiguousFromString? result = JsonSerializer.Deserialize<TypeWithAmbiguousFromString>(json, options);
 		Assert.IsNotNull(result);
 		// Either FromString or Parse may be selected depending on reflection path; both produce non-null; ensure it's one of expected shapes

RoundTripStringJsonConverter/RoundTripStringJsonConverter.cs

@@ -103,49 +103,22 @@
 	/// <returns>The method info if found, null otherwise.</returns>
 	private static MethodInfo? FindStringConversionMethod(Type type)
 	{
+		MethodInfo[] publicStaticMethods = type.GetMethods(BindingFlags.Static | BindingFlags.Public | BindingFlags.FlattenHierarchy);
 		foreach (string methodName in SupportedMethodNames)
 		{
-			try
-			{
-				MethodInfo[] publicStaticMethods = type.GetMethods(BindingFlags.Static | BindingFlags.Public | BindingFlags.FlattenHierarchy);
-
-				// Get all methods with the specified name and binding flags
-				MethodInfo[] methods = [.. publicStaticMethods
-					.Where(m => m.Name == methodName)];
-
-				// Prefer an exact single-string overload. Otherwise take the first overload whose extra
-				// parameters can all be supplied, so GetMethods() ordering can't pick one we can't call.
-				MethodInfo? match = methods.FirstOrDefault(m => m.GetParameters().Length == 1 && IsUsableConversionMethod(m, type))
-					?? methods.FirstOrDefault(m => IsUsableConversionMethod(m, type));
-				if (match is not null)
-				{
-					return match;
-				}
-			}
-			catch (AmbiguousMatchException)
+			// Close any generic candidates over the type, dropping those that can't be closed or can't be called
+			MethodInfo[] methods = [.. publicStaticMethods
+				.Where(m => m.Name == methodName)
+				.Select(m => CloseOverType(m, type))
+				.OfType<MethodInfo>()
+				.Where(m => IsUsableConversionMethod(m, type))];
+
+			// Prefer an exact single-string overload. Otherwise take the first overload whose extra
+			// parameters can all be supplied, so GetMethods() ordering can't pick one we can't call.
+			MethodInfo? match = methods.FirstOrDefault(m => m.GetParameters().Length == 1) ?? methods.FirstOrDefault();
+			if (match is not null)
 			{
-				// If there's an ambiguous match, try to find the specific overload we want
-				try
-				{
-					MethodInfo? method = type.GetMethod(methodName, BindingFlags.Static | BindingFlags.Public, null, [typeof(string)], null);
-					if (method is not null &&
-						(method.ReturnType == type ||
-						 (method.ReturnType.IsGenericType && type.IsGenericType &&
-						  method.ReturnType.GetGenericTypeDefinition() == type.GetGenericTypeDefinition())))
-					{
-						return method;
-					}
-				}
-				catch (ArgumentException)
-				{
-					// Continue to next method name if this one fails
-					continue;
-				}
-				catch (AmbiguousMatchException)
-				{
-					// Continue to next method name if this one fails
-					continue;
-				}
+				return match;
 			}
 		}
 		return null;
@@ -164,9 +137,31 @@
 		return parameters.Length > 0 &&
 			parameters[0].ParameterType == typeof(string) &&
 			parameters.Skip(1).All(p => p.ParameterType == typeof(IFormatProvider) || p.IsOptional) &&
-			(method.ReturnType == type ||
-			 (method.ReturnType.IsGenericType && type.IsGenericType &&
-			  method.ReturnType.GetGenericTypeDefinition() == type.GetGenericTypeDefinition()));
+			method.ReturnType == type;
+	}
+
+	/// <summary>
+	/// Closes a generic method definition over the specified type, so that it can be checked and invoked.
+	/// </summary>
+	/// <param name="method">The candidate method.</param>
+	/// <param name="type">The type to convert to.</param>
+	/// <returns>The method itself if it is not generic, the closed method, or null if it cannot be closed over the type.</returns>
+	private static MethodInfo? CloseOverType(MethodInfo method, Type type)
+	{
+		if (!method.IsGenericMethodDefinition)
+		{
+			return method;
+		}
+
+		try
+		{
+			return method.MakeGenericMethod(type);
+		}
+		catch (ArgumentException)
+		{
+			// The method has more than one type parameter, or the type violates a constraint
+			return null;
+		}
 	}
 
 	/// <summary>
@@ -176,7 +171,7 @@
 	/// <param name="method">The conversion method.</param>
 	/// <param name="value">The string to convert.</param>
 	/// <returns>The arguments to invoke the method with.</returns>
 	private static object?[] BuildArguments(MethodInfo method, string value)
 	{
 		ParameterInfo[] parameters = method.GetParameters();
 		object?[] arguments = new object?[parameters.Length];
@@ -218,8 +213,8 @@
 	/// <returns>A JSON converter for the specified type.</returns>
 	public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
 	{
 		Ensure.NotNull(typeToConvert, nameof(typeToConvert));
 		Ensure.NotNull(options, nameof(options));
 		Type converterType = typeof(RoundTripStringJsonConverter<>).MakeGenericType(typeToConvert);
 		return (JsonConverter)Activator.CreateInstance(converterType, BindingFlags.Instance | BindingFlags.Public, binder: null, args: null, culture: null)!;
 	}
@@ -232,7 +227,7 @@
 	[SuppressMessage("Microsoft.Performance", "CA1812:AvoidUninstantiatedInternalClasses", Justification = "Instantiated via reflection in CreateConverter using Activator.CreateInstance")]
 	private sealed class RoundTripStringJsonConverter<T> : JsonConverter<T>
 	{
-		private static readonly MethodInfo? StringConversionMethod = CreateStringConversionMethod();
+		private static readonly MethodInfo? StringConversionMethod = FindStringConversionMethod(typeof(T));
 
 		/// <summary>
 		/// Whether the conversion method is handed the invariant culture by <see cref="BuildArguments"/>,
@@ -241,16 +236,6 @@
 		private static readonly bool ReadsWithInvariantCulture =
 			StringConversionMethod?.GetParameters().Any(p => p.ParameterType == typeof(IFormatProvider)) ?? false;
 
-		private static MethodInfo? CreateStringConversionMethod()
-		{
-			MethodInfo? method = FindStringConversionMethod(typeof(T));
-			if (method is not null && method.ContainsGenericParameters)
-			{
-				method = method.MakeGenericMethod(typeof(T));
-			}
-			return method;
-		}
-
 		/// <summary>
 		/// Formats a value in the culture its conversion method reads it back with: the invariant culture
 		/// when the method takes an <see cref="IFormatProvider"/>, otherwise whatever <c>ToString()</c> uses.
@@ -278,7 +263,7 @@
 		/// <returns>The converted value.</returns>
 		public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 		{
 			Ensure.NotNull(typeToConvert, nameof(typeToConvert));
 
 			if (reader.TokenType == JsonTokenType.Null)
 			{
@@ -291,7 +276,7 @@
 			}
 
 			string? stringValue = reader.GetString();
 			Ensure.NotNull(stringValue, nameof(stringValue));
 
 			try
 			{
@@ -314,7 +299,7 @@
 		/// <returns>The converted value.</returns>
 		public override T ReadAsPropertyName(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 		{
 			Ensure.NotNull(typeToConvert, nameof(typeToConvert));
 
 			string? stringValue = reader.GetString();
 			Ensure.NotNull(stringValue, nameof(stringValue));