-
-
Notifications
You must be signed in to change notification settings - Fork 108
fix: handle ref struct parameters in [GenerateAssertion] source generator #4221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ator Fixes #4192 The source generator was failing when encountering methods with ref struct parameters (like DefaultInterpolatedStringHandler) because it tried to store them as class fields, which is illegal in C#. Changes: - Add IsRefStruct() and IsInterpolatedStringHandler() helper methods to detect ref-like types - Store ref struct parameters as string instead of their original type - Convert interpolated string handlers to string via .ToStringAndClear() in the extension method before passing to assertion constructor - Remove redundant .ToStringAndClear()/.ToString() calls in inlined method bodies since the field is already a string - Add TUNITGEN004 diagnostic error when methods with ref struct parameters don't use InlineMethodBody = true (required because we can't call the original method with a string when it expects a ref struct) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryFixes ref struct parameter handling in the assertion source generator by converting ref structs (like Critical IssuesNone found ✅ Suggestions1. Reflection-based detection could be more robustLocation: The var isRefLikeProperty = namedType.GetType().GetProperty("IsRefLikeType");
if (isRefLikeProperty?.GetValue(namedType) is bool isRefLike && isRefLike)While you have a fallback, consider checking the Roslyn version you're targeting. Modern Roslyn versions (4.0+) expose if (namedType.IsRefLikeType)
return true;2. Missing snapshot verification filesThe new test After the test infrastructure version mismatch is resolved and tests run successfully, ensure you commit the generated 3. Regex edge casesLocation: The regex patterns remove inlinedBody = Regex.Replace(
inlinedBody,
$@"{Regex.Escape(fieldName)}\.ToStringAndClear\(\)",
fieldName);This looks correct, but be aware that it will match even in string literals or comments. Since you're working with parsed method bodies this is likely safe, but if edge cases arise, you might need to use Roslyn's semantic model for more precise replacement. Previous Review StatusUnable to check previous comments due to GitHub token scope limitations. Verdict✅ APPROVE - No critical issues. The implementation correctly handles the ref struct constraint, adds appropriate diagnostics, and follows TUnit patterns. The suggestions are minor improvements that don't block merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes issue #4192 by adding support for ref struct parameters (like DefaultInterpolatedStringHandler) in the [GenerateAssertion] source generator. Previously, the generator would fail when encountering these types because it attempted to store them as class fields, which is illegal in C# since ref structs can only exist on the stack.
Key Changes
- Ref struct parameters are now stored as
stringfields instead of their original type - Conversion to string happens at the extension method boundary using
ToStringAndClear()for interpolated string handlers orToString()for other ref structs - The inlined method bodies are cleaned up to remove redundant conversion calls since the field already contains a string
- A new diagnostic TUNITGEN004 enforces that methods with ref struct parameters must use
InlineMethodBody = true
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs | Added ref struct detection methods, modified field/constructor/extension method generation to handle ref structs by converting to strings, added regex cleanup for inlined bodies, and added TUNITGEN004 diagnostic |
| TUnit.Assertions.SourceGenerator.Tests/TestData/RefStructParameterAssertion.cs | Added test data file with example methods using ref DefaultInterpolatedStringHandler parameters to verify the generator handles ref structs correctly |
| TUnit.Assertions.SourceGenerator.Tests/MethodAssertionGeneratorTests.cs | Added RefStructParameter() test that verifies fields are stored as strings, extension methods perform conversions, and inlined bodies don't have redundant conversion calls |
| [GenerateAssertion(ExpectationMessage = "to contain {message}", InlineMethodBody = true)] | ||
| public static bool ContainsMessage(this string value, ref DefaultInterpolatedStringHandler message) | ||
| { | ||
| var stringMessage = message.ToStringAndClear(); | ||
| return value.Contains(stringMessage); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Test with a simpler expression body | ||
| /// </summary> | ||
| [GenerateAssertion(ExpectationMessage = "to end with {suffix}", InlineMethodBody = true)] | ||
| public static bool EndsWithMessage(this string value, ref DefaultInterpolatedStringHandler suffix) | ||
| => value.EndsWith(suffix.ToStringAndClear()); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test methods use ref DefaultInterpolatedStringHandler parameters and call .ToStringAndClear() within the method bodies. However, this pattern might not accurately represent real-world usage of interpolated string handlers.
Typically, DefaultInterpolatedStringHandler parameters are used with the C# compiler's interpolated string syntax (e.g., $"string {value}"), where the compiler automatically creates and manages the handler. Manually passing a ref DefaultInterpolatedStringHandler is uncommon.
Consider adding a test case that shows how this feature would actually be used in practice, or document why users would want to create assertions with ref struct parameters. This would help clarify the intended use case and validate that the implementation supports real-world scenarios.
| $@"{Regex.Escape(fieldName)}\.ToStringAndClear\(\)", | ||
| fieldName); | ||
| // Remove .ToString() - the value is already a string | ||
| inlinedBody = Regex.Replace( | ||
| inlinedBody, | ||
| $@"{Regex.Escape(fieldName)}\.ToString\(\)", |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex replacement logic to remove .ToStringAndClear() and .ToString() calls may be too broad and could accidentally match and remove these method calls on other objects that happen to have the same field name prefix.
For example, if you have code like var otherObj_message = GetMessage(); return value.Contains(otherObj_message.ToStringAndClear()), the regex would incorrectly remove the .ToStringAndClear() call even though otherObj_message is not the ref struct field.
Consider adding word boundary anchors or being more specific in the pattern to ensure you're only replacing method calls on the exact field name, not on other variables that happen to start with the same name.
| $@"{Regex.Escape(fieldName)}\.ToStringAndClear\(\)", | |
| fieldName); | |
| // Remove .ToString() - the value is already a string | |
| inlinedBody = Regex.Replace( | |
| inlinedBody, | |
| $@"{Regex.Escape(fieldName)}\.ToString\(\)", | |
| $@"\b{Regex.Escape(fieldName)}\.ToStringAndClear\(\)", | |
| fieldName); | |
| // Remove .ToString() - the value is already a string | |
| inlinedBody = Regex.Replace( | |
| inlinedBody, | |
| $@"\b{Regex.Escape(fieldName)}\.ToString\(\)", |
| // Use reflection to access IsRefLikeType property which may not be available in all Roslyn versions | ||
| var isRefLikeProperty = namedType.GetType().GetProperty("IsRefLikeType"); | ||
| if (isRefLikeProperty?.GetValue(namedType) is bool isRefLike && isRefLike) | ||
| { | ||
| return true; | ||
| } |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses runtime reflection (namedType.GetType().GetProperty("IsRefLikeType")) to detect the IsRefLikeType property because it may not be available in all Roslyn versions. However, this approach has a performance cost since it uses reflection on every call.
Consider caching the PropertyInfo or the result of checking if the property exists. Since this method will be called for every parameter in every method being processed, the repeated reflection lookups could add up. You could cache the PropertyInfo in a static field or check once if the property is available on the current Roslyn version.
| /// <summary> | ||
| /// Checks if a type is a ref struct (ref-like type). | ||
| /// Ref structs cannot be stored as fields in classes. | ||
| /// </summary> | ||
| private static bool IsRefStruct(ITypeSymbol type) | ||
| { | ||
| if (type is not INamedTypeSymbol namedType) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Use reflection to access IsRefLikeType property which may not be available in all Roslyn versions | ||
| var isRefLikeProperty = namedType.GetType().GetProperty("IsRefLikeType"); | ||
| if (isRefLikeProperty?.GetValue(namedType) is bool isRefLike && isRefLike) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Fallback: check for common ref struct types by name | ||
| var typeName = namedType.ToDisplayString(); | ||
| if (typeName.StartsWith("System.Span<") || | ||
| typeName.StartsWith("System.ReadOnlySpan<") || | ||
| typeName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for InterpolatedStringHandlerAttribute on the type | ||
| return namedType.GetAttributes().Any(attr => | ||
| attr.AttributeClass?.ToDisplayString() == "System.Runtime.CompilerServices.InterpolatedStringHandlerAttribute"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if a type is an interpolated string handler (e.g., DefaultInterpolatedStringHandler). | ||
| /// These types need special handling as they should be converted to string. | ||
| /// </summary> | ||
| private static bool IsInterpolatedStringHandler(ITypeSymbol type) | ||
| { | ||
| if (type is not INamedTypeSymbol namedType) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Check for DefaultInterpolatedStringHandler specifically | ||
| var typeName = namedType.ToDisplayString(); | ||
| if (typeName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for InterpolatedStringHandlerAttribute on the type | ||
| return namedType.GetAttributes().Any(attr => | ||
| attr.AttributeClass?.ToDisplayString() == "System.Runtime.CompilerServices.InterpolatedStringHandlerAttribute"); | ||
| } |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both IsRefStruct and IsInterpolatedStringHandler methods duplicate the same attribute checking logic. The check for InterpolatedStringHandlerAttribute appears in both methods (lines 1094-1095 and 1117-1118).
Consider extracting this common logic into a helper method, or have IsInterpolatedStringHandler call IsRefStruct first to check if it's a ref struct, then add the additional specific checks for interpolated string handlers. This would reduce code duplication and make maintenance easier.
| // Verify that the field type is string, not the ref struct | ||
| await Assert.That(mainFile).Contains("private readonly string _message;"); | ||
| await Assert.That(mainFile).Contains("private readonly string _suffix;"); | ||
|
|
||
| // Verify that the extension method converts the ref struct to string | ||
| await Assert.That(mainFile).Contains("message.ToStringAndClear()"); | ||
| await Assert.That(mainFile).Contains("suffix.ToStringAndClear()"); | ||
|
|
||
| // Verify the constructor takes string, not the ref struct | ||
| await Assert.That(mainFile).Contains("string message)"); | ||
| await Assert.That(mainFile).Contains("string suffix)"); | ||
|
|
||
| // Verify that .ToStringAndClear() is removed in the inlined body | ||
| // (since the field is already a string) | ||
| // The inlined body should use _message directly, not _message.ToStringAndClear() | ||
| await Assert.That(mainFile).Contains("value!.Contains(_message)"); | ||
| }); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test assertions verify the generated code contains specific strings, but these are quite brittle and might break if formatting or whitespace changes. Additionally, line 234 checks for value!.Contains(_message) which assumes specific formatting.
Consider using snapshot testing (like the FileScopedClassWithInlining test on line 194) instead of string-based assertions. This would make the tests more robust and easier to maintain, and you'd be able to review the full generated output in the snapshot files.
| // Ref structs cannot be stored as class fields, so we need to inline the method body | ||
| foreach (var param in additionalParameters) | ||
| { | ||
| if (IsRefStruct(param.Type) && string.IsNullOrEmpty(methodBody)) | ||
| { | ||
| var diagnostic = Diagnostic.Create( | ||
| RefStructRequiresInliningRule, | ||
| location, | ||
| methodSymbol.Name, | ||
| param.Name, | ||
| param.Type.ToDisplayString()); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message for TUNITGEN004 states "Use InlineMethodBody = true and ensure the method has a single-expression or single-return-statement body" but the code only checks if methodBody is empty or not. It doesn't validate that when InlineMethodBody = true is specified, the method actually has a valid body that can be inlined.
A user could set InlineMethodBody = true on a method with a complex multi-statement body, pass the validation at line 275, but then get unexpected behavior because the body extraction logic (lines 247-269) would fail to extract a method body from complex methods, leaving methodBody as null. This would silently fail to inline.
Consider adding validation to ensure that when ref struct parameters are present, the method body was successfully extracted and is not null/empty even when InlineMethodBody = true is specified.
| // Ref structs cannot be stored as class fields, so we need to inline the method body | |
| foreach (var param in additionalParameters) | |
| { | |
| if (IsRefStruct(param.Type) && string.IsNullOrEmpty(methodBody)) | |
| { | |
| var diagnostic = Diagnostic.Create( | |
| RefStructRequiresInliningRule, | |
| location, | |
| methodSymbol.Name, | |
| param.Name, | |
| param.Type.ToDisplayString()); | |
| // Ref structs cannot be stored as class fields, so we need to inline the method body. | |
| // The body must be either an expression-bodied member or a single return statement with an expression. | |
| var firstRefStructParam = additionalParameters.FirstOrDefault(p => IsRefStruct(p.Type)); | |
| if (firstRefStructParam is not null) | |
| { | |
| var hasInlinableBody = | |
| methodSyntax.ExpressionBody is not null || | |
| (methodSyntax.Body is not null && | |
| methodSyntax.Body.Statements.Count == 1 && | |
| methodSyntax.Body.Statements[0] is ReturnStatementSyntax { Expression: not null }); | |
| if (!hasInlinableBody) | |
| { | |
| var diagnostic = Diagnostic.Create( | |
| RefStructRequiresInliningRule, | |
| location, | |
| methodSymbol.Name, | |
| firstRefStructParam.Name, | |
| firstRefStructParam.Type.ToDisplayString()); |
| // Use reflection to access IsRefLikeType property which may not be available in all Roslyn versions | ||
| var isRefLikeProperty = namedType.GetType().GetProperty("IsRefLikeType"); | ||
| if (isRefLikeProperty?.GetValue(namedType) is bool isRefLike && isRefLike) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Fallback: check for common ref struct types by name | ||
| var typeName = namedType.ToDisplayString(); | ||
| if (typeName.StartsWith("System.Span<") || | ||
| typeName.StartsWith("System.ReadOnlySpan<") || | ||
| typeName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for InterpolatedStringHandlerAttribute on the type |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback logic for detecting ref structs using string name comparison is incomplete. It only checks for Span<T>, ReadOnlySpan<T>, and DefaultInterpolatedStringHandler, but there are other common ref struct types in .NET that would be missed, such as:
System.ReadOnlySpan(non-generic)System.Span(non-generic)System.Runtime.InteropServices.ArraySegment<T>variantsSystem.Threading.Tasks.ValueTask<T>(not a ref struct, but worth noting)- Custom user-defined ref structs
While the InterpolatedStringHandlerAttribute check at lines 1094-1095 helps catch custom interpolated string handlers, it won't catch other custom ref structs that don't use that attribute.
Consider documenting this limitation or expanding the fallback checks. Alternatively, since you're targeting modern Roslyn versions (the project appears to be .NET 6+), you might want to verify if IsRefLikeType is always available and remove the fallback entirely.
| // Use reflection to access IsRefLikeType property which may not be available in all Roslyn versions | |
| var isRefLikeProperty = namedType.GetType().GetProperty("IsRefLikeType"); | |
| if (isRefLikeProperty?.GetValue(namedType) is bool isRefLike && isRefLike) | |
| { | |
| return true; | |
| } | |
| // Fallback: check for common ref struct types by name | |
| var typeName = namedType.ToDisplayString(); | |
| if (typeName.StartsWith("System.Span<") || | |
| typeName.StartsWith("System.ReadOnlySpan<") || | |
| typeName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler") | |
| { | |
| return true; | |
| } | |
| // Check for InterpolatedStringHandlerAttribute on the type | |
| // Prefer Roslyn's built-in ref-like detection. This correctly identifies all ref struct types, | |
| // including user-defined ones, on the modern Roslyn versions this project targets. | |
| if (namedType.IsRefLikeType) | |
| { | |
| return true; | |
| } | |
| // As a very narrow fallback (e.g., for custom interpolated string handlers), | |
| // treat types marked with InterpolatedStringHandlerAttribute as ref structs. |
| if (IsRefStruct(param.Type)) | ||
| { | ||
| // Convert ref struct to string - use ToStringAndClear for interpolated string handlers | ||
| // or ToString() for other ref structs | ||
| var conversion = IsInterpolatedStringHandler(param.Type) | ||
| ? $"{param.Name}.ToStringAndClear()" | ||
| : $"{param.Name}.ToString()"; | ||
| sb.Append($", {conversion}"); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice between .ToStringAndClear() and .ToString() for ref structs is based solely on whether the type is an interpolated string handler. However, for ref parameters of interpolated string handlers, calling .ToStringAndClear() may not be semantically correct because it mutates the original value (clearing it), which might be unexpected when using a ref parameter.
If the original method expects to receive a ref DefaultInterpolatedStringHandler and potentially use it multiple times or pass it to other methods, clearing it at the extension method boundary could break the expected behavior.
Consider using .ToString() instead of .ToStringAndClear() for ref parameters, even for interpolated string handlers, to avoid unexpected mutations. Alternatively, document this behavior clearly so users understand that ref struct parameters passed to generated assertions will be consumed/cleared.
| foreach (var param in data.AdditionalParameters) | ||
| { | ||
| if (IsRefStruct(param.Type)) | ||
| { | ||
| var fieldName = $"_{param.Name}"; | ||
| // Remove .ToStringAndClear() - the value is already a string | ||
| inlinedBody = Regex.Replace( | ||
| inlinedBody, | ||
| $@"{Regex.Escape(fieldName)}\.ToStringAndClear\(\)", | ||
| fieldName); | ||
| // Remove .ToString() - the value is already a string | ||
| inlinedBody = Regex.Replace( | ||
| inlinedBody, | ||
| $@"{Regex.Escape(fieldName)}\.ToString\(\)", | ||
| fieldName); | ||
| } | ||
| } |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Summary
Fixes #4192
The source generator was failing when encountering methods with
ref structparameters (likeDefaultInterpolatedStringHandler) because it tried to store them as class fields, which is illegal in C#.Changes
IsRefStruct()andIsInterpolatedStringHandler()to detect ref-like types using theIsRefLikeTypeproperty and fallback pattern matchingstringtype instead of the original type.ToStringAndClear()before passing to the assertion constructor; other ref structs use.ToString().ToStringAndClear()/.ToString()calls in inlined method bodies since the field is already a stringInlineMethodBody = true(required because we can't call the original method with a string when it expects a ref struct)Example
For a method like:
The generator now produces:
private readonly string _message;(instead of storing the handler)message.ToStringAndClear()before passing to constructorvalue!.Contains(_message)(with.ToStringAndClear()removed since it's already a string)Test plan
RefStructParameterAssertion.cswith interpolated string handler parameter examplesRefStructParameter()to verify the generated code🤖 Generated with Claude Code