-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix duplicate parameter names in introduce parameter refactoring #81404
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
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| } | ||
| """); | ||
|
|
||
| [Fact] |
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 add WorkItem("https://github.com/dotnet/roslyn/issues/81400") for all tests you've added.
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.
Done in ae24b17.
| else | ||
| { | ||
| baseName = _semanticFacts.GenerateNameForExpression(semanticModel, _expression, capitalize: false, cancellationToken); | ||
| } |
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 change this to a conditional expression like so:
var baseName = ShouldRemove(...)
? varDeclName
: _semanticFacts.GenerateNameForExpression(...);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.
Done in cace554.
| _containerMethod, | ||
| _containerMethod, | ||
| baseName, | ||
| existingParameterNames, |
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 do you need to pass existingParameterNames along? i think GenerateUniqueName figures it out from the locations you pass in. Also, use GenerateUniqueLocalName. I don't think you need the container if you have the _expression as that's the location we can use to determine the names of things in scope.
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.
Done in cace554. Changed to use GenerateUniqueLocalName with _expression as the location and container: null, following the pattern used in IntroduceVariable. This automatically determines conflicts without needing to pass existing parameter names.
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Fix "Introduce parameter" duplicate parameter names
This PR fixes an issue where the "Introduce parameter" refactoring could generate duplicate or ambiguous parameter names when the generated name conflicts with an existing parameter.
Problem:
When using the "Introduce parameter" refactoring on an expression like
v + 1in a method that already has a parameter namedv, the refactoring would generate a duplicate parameterv, leading to compiler errors:Solution:
Modified
GetNewParameterNameAsyncinIntroduceParameterDocumentRewriter.csto useISemanticFactsService.GenerateUniqueLocalNameto ensure the generated parameter name doesn't conflict with existing parameters. The method now:GenerateUniqueLocalNamewhich automatically determines conflicts from the expression locationTesting:
Added comprehensive tests covering all three code action options:
Status:
IntroduceParameterDocumentRewriter.csGetNewParameterNameAsyncmethod that generates parameter namesGetNewParameterNameAsyncto useISemanticFactsService.GenerateUniqueLocalNameFixes #81400
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.