-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix IDE0028: Suppress diagnostic in CollectionBuilder methods to prevent infinite recursion #81004
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
Fix IDE0028: Suppress diagnostic in CollectionBuilder methods to prevent infinite recursion #81004
Conversation
- Added virtual method ShouldSuppressDiagnostic to base analyzer - Implemented CollectionBuilder detection in C# analyzer - Detects when we're inside a CollectionBuilder method to avoid infinite recursion - Added tests to verify the fix works correctly Co-authored-by: CyrusNajmabadi <[email protected]>
Use OriginalDefinition to check attributes on generic types and compare builder types correctly Co-authored-by: CyrusNajmabadi <[email protected]>
- Use IsCollectionBuilderAttribute() helper method instead of manual namespace checking - Use NewKeyword.SpanStart for more precise position when getting enclosing symbol - Add null check for AttributeClass before calling extension method Co-authored-by: CyrusNajmabadi <[email protected]>
Explain that CollectionBuilderAttribute has exactly 2 constructor parameters Co-authored-by: CyrusNajmabadi <[email protected]>
| var builderTypeArg = collectionBuilderAttribute.ConstructorArguments[0]; | ||
| var methodNameArg = collectionBuilderAttribute.ConstructorArguments[1]; | ||
|
|
||
| if (builderTypeArg.Kind != TypedConstantKind.Type || | ||
| builderTypeArg.Value is not INamedTypeSymbol builderType || | ||
| methodNameArg.Kind != TypedConstantKind.Primitive || | ||
| methodNameArg.Value is not string methodName) | ||
| { | ||
| return false; | ||
| } |
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.
@copilot replace this section with a pattern match.
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.
Refactored to use a list pattern match in commit f63c546. The code now validates both constructor arguments and extracts the values in a single expression.
| ObjectCreationExpressionSyntax objCreation => objCreation.NewKeyword.SpanStart, | ||
| ImplicitObjectCreationExpressionSyntax implicitObjCreation => implicitObjCreation.NewKeyword.SpanStart, | ||
| _ => objectCreationExpression.SpanStart | ||
| }; |
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.
@copilot this is all unnecessary. you can just use objectCreationExpression.SpanStart for all cases.
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.
Simplified in commit 93efc8b. Now using objectCreationExpression.SpanStart directly.
| ReferenceAssemblies = ReferenceAssemblies.Net.Net80, | ||
| }.RunAsync(); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/74268")] |
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.
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.
Replace separate extraction and validation with a single pattern match expression Co-authored-by: CyrusNajmabadi <[email protected]>
- Use objectCreationExpression.SpanStart directly instead of switch statement - Update WorkItem references from issue #74268 to #70099 Co-authored-by: CyrusNajmabadi <[email protected]>
...ore/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
| namespace System.Runtime.CompilerServices | ||
| { | ||
| [AttributeUsage(AttributeTargets.All, Inherited = false, AllowMultiple = false)] | ||
| public sealed class CollectionBuilderAttribute : Attribute |
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.
is the definition necessary? e.g. it isn't included in the standard testworkspace references?
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.
technically not in the refeences. but we have a string constant for it. updating.
…UseCollectionInitializerDiagnosticAnalyzer.cs Co-authored-by: David Barbet <[email protected]>
Fix for IDE0028 with CollectionBuilder Methods ✅
This PR fixes issue #70099 where IDE0028 (Collection initialization can be simplified) incorrectly suggests using collection expressions inside CollectionBuilder methods, which would cause infinite recursion.
Problem
When implementing a CollectionBuilder method, the analyzer would suggest converting:
To:
This causes infinite recursion because the collection expression
[.. items]calls the same CollectionBuilder method.Solution
Added detection logic to suppress the diagnostic when:
Implementation
ShouldSuppressDiagnosticmethodIsCollectionBuilderAttribute()helper for type checkingOriginalDefinitioncomparison for generic typesFiles Changed
src/Analyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerDiagnosticAnalyzer.cssrc/Analyzers/CSharp/Analyzers/UseCollectionInitializer/CSharpUseCollectionInitializerDiagnosticAnalyzer.cssrc/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests_CollectionExpression.csCommits
All changes follow Roslyn coding conventions and include comprehensive tests.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.