-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #if NET6_0_OR_GREATER | ||
| using System.Runtime.CompilerServices; | ||
| using TUnit.Assertions.Attributes; | ||
|
|
||
| namespace TUnit.Assertions.Tests.TestData; | ||
|
|
||
| /// <summary> | ||
| /// Test case: Method with ref struct parameter (DefaultInterpolatedStringHandler) | ||
| /// The generator should convert the ref struct to string before storing it | ||
| /// </summary> | ||
| public static class RefStructParameterAssertions | ||
| { | ||
| /// <summary> | ||
| /// Test that interpolated string handlers are properly converted to strings | ||
| /// </summary> | ||
| [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()); | ||
|
Comment on lines
+16
to
+28
|
||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public sealed class MethodAssertionGenerator : IIncrementalGenerator | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static readonly DiagnosticDescriptor MethodMustBeStaticRule = new DiagnosticDescriptor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: "TUNITGEN001", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 24 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: "Method must be static", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageFormat: "Method '{0}' decorated with [GenerateAssertion] must be static", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| category: "TUnit.Assertions.SourceGenerator", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -30,7 +30,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Methods decorated with [GenerateAssertion] must be static to be used in generated assertions."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static readonly DiagnosticDescriptor MethodMustHaveParametersRule = new DiagnosticDescriptor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: "TUNITGEN002", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 33 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: "Method must have at least one parameter", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageFormat: "Method '{0}' decorated with [GenerateAssertion] must have at least one parameter (the value to assert)", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| category: "TUnit.Assertions.SourceGenerator", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -39,14 +39,23 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Methods decorated with [GenerateAssertion] must have at least one parameter representing the value being asserted."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static readonly DiagnosticDescriptor UnsupportedReturnTypeRule = new DiagnosticDescriptor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: "TUNITGEN003", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 42 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: "Unsupported return type", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageFormat: "Method '{0}' decorated with [GenerateAssertion] has unsupported return type '{1}'. Supported types are: bool, AssertionResult, Task<bool>, Task<AssertionResult>", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 44 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| category: "TUnit.Assertions.SourceGenerator", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultSeverity: DiagnosticSeverity.Error, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isEnabledByDefault: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Methods decorated with [GenerateAssertion] must return bool, AssertionResult, Task<bool>, or Task<AssertionResult>."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static readonly DiagnosticDescriptor RefStructRequiresInliningRule = new DiagnosticDescriptor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: "TUNITGEN004", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 51 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: "Ref struct parameter requires method body inlining", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageFormat: "Method '{0}' has ref struct parameter '{1}' of type '{2}'. Use InlineMethodBody = true and ensure the method has a single-expression or single-return-statement body", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 53 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| category: "TUnit.Assertions.SourceGenerator", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultSeverity: DiagnosticSeverity.Error, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isEnabledByDefault: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Methods with ref struct parameters (like DefaultInterpolatedStringHandler) require InlineMethodBody = true because ref structs cannot be stored as class fields. The method must have a simple body that can be inlined."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void Initialize(IncrementalGeneratorInitializationContext context) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Find all methods decorated with [GenerateAssertion] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -259,6 +268,22 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Validate that methods with ref struct parameters have inlined method bodies | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+272
to
+282
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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()); |
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\(\)", |
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(...)'.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (fr-FR)
Déréférencement d'une éventuelle référence null.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (pl-PL)
Wyłuskanie odwołania, które może mieć wartość null.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (de-DE)
Dereferenzierung eines möglichen Nullverweises.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (ubuntu-latest)
Dereference of a possibly null reference.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (ubuntu-latest)
Dereference of a possibly null reference.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (macos-latest)
Dereference of a possibly null reference.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (macos-latest)
Dereference of a possibly null reference.
Check warning on line 785 in TUnit.Assertions.SourceGenerator/Generators/MethodAssertionGenerator.cs
GitHub Actions / modularpipeline (windows-latest)
Dereference of a possibly null reference.
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.
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.
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. |
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.
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
FileScopedClassWithInliningtest 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.