Add a parse method wrapper and caching to fix AoT compilation#1103
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new HasParseMethod/Parse API on ITypeInspector (and corresponding plumbing in the inspector wrappers, composite inspector, cache inspector, and static source generator) to support string-to-type parsing in a way that is intended to be more AoT/trimming-friendly.
Changes:
- Adds
HasParseMethod(Type)andParse(string, Type)toITypeInspectorandTypeInspectorSkeleton, and threads them through the existing type inspector wrappers. - Implements reflection-based parse discovery/invocation in
ReflectionTypeInspectorand addsHasParseMethodcaching inCachedTypeInspector. - Updates the static source generator to emit
HasParseMethod/Parseimplementations, and adjusts the Core7 AoT compile test to use an underscored naming convention.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| YamlDotNet/Serialization/ITypeInspector.cs | Adds new parse-related APIs and documentation. |
| YamlDotNet/Serialization/TypeInspectors/TypeInspectorSkeleton.cs | Makes parse-related APIs abstract on the skeleton base. |
| YamlDotNet/Serialization/TypeInspectors/ReflectionTypeInspector.cs | Adds reflection-based HasParseMethod/Parse implementation. |
| YamlDotNet/Serialization/TypeInspectors/CachedTypeInspector.cs | Adds caching for HasParseMethod. |
| YamlDotNet/Serialization/TypeInspectors/CompositeTypeInspector.cs | Aggregates parse support across multiple inspectors. |
| YamlDotNet/Serialization/TypeInspectors/NamingConventionTypeInspector.cs | Delegates new APIs to wrapped inspector. |
| YamlDotNet/Serialization/TypeInspectors/ReadableAndWritablePropertiesTypeInspector.cs | Delegates new APIs to wrapped inspector. |
| YamlDotNet/Serialization/YamlAttributesTypeInspector.cs | Delegates new APIs to wrapped inspector. |
| YamlDotNet.Analyzers.StaticGenerator/StaticTypeInspectorFile.cs | Generates parse support in the static inspector source output. |
| YamlDotNet.Core7AoTCompileTest/Program.cs | Updates AoT compile test builder/sample YAML for underscored naming. |
| YamlDotNet/Serialization/Utilities/ReflectionTypeConverter.cs | Removes an extraneous whitespace-only line. |
Comments suppressed due to low confidence (4)
YamlDotNet/Serialization/TypeInspectors/ReflectionTypeInspector.cs:86
- Parse invokes the reflected Parse method via MethodInfo.Invoke but does not unwrap TargetInvocationException like other conversion paths do (e.g., TypeConverter.ChangeType). This changes the exception surface for consumers by wrapping parse failures. Consider catching TargetInvocationException and rethrowing the InnerException when present for consistency.
public override object? Parse(string value, Type expectedType)
{
var method = expectedType.GetMethod("Parse", [typeof(string)]);
if (method == null)
{
throw new InvalidOperationException($"Type '{expectedType.FullName}' does not have a static Parse method.");
}
return method.Invoke(null, new object[] { value });
}
YamlDotNet/Serialization/TypeInspectors/ReflectionTypeInspector.cs:83
- The error message uses expectedType.FullName, which can be null for some Type instances (e.g., generic parameters). Using expectedType.ToString() (or a fallback when FullName is null) would make the exception message reliably informative.
if (method == null)
{
throw new InvalidOperationException($"Type '{expectedType.FullName}' does not have a static Parse method.");
}
YamlDotNet/Serialization/TypeInspectors/CompositeTypeInspector.cs:102
- CompositeTypeInspector.Parse throws ArgumentOutOfRangeException when no inspector supports the type, but this isn't a range/argument validation problem; it's an unsupported operation/configuration issue. Consider throwing InvalidOperationException or NotSupportedException (and include a stable type name fallback if FullName is null).
if (parsableInspector == null)
{
throw new ArgumentOutOfRangeException(nameof(expectedType), $"No inspector with a Parse method for type {expectedType.FullName} was found.");
}
YamlDotNet/Serialization/ITypeInspector.cs:79
- The XML doc comment for Parse has malformed
formatting ("///
Converts...") which can degrade generated documentation. Align it with the surrounding style by putting the summary text on its own line between
and
.
/// <summary> Converts a string to an instance of the specified type using a static Parse method.
/// </summary>
/// <param name="value">The string value to convert.</param>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #1098
This is a breaking change in the TypeInspectorSkeleton class and adds 2 methods to ITypeInspector. Quick fix to resolve those breaking changes in your own custom TypeInspector is to return
falseon theHasParseMethodmethod and returnnullor throw an exception on theParsemethod.