- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.2k
Fix rename conflict detection for local functions and lambdas #80821
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: CyrusNajmabadi <[email protected]>
…ction tests Co-authored-by: CyrusNajmabadi <[email protected]>
| @copilot the following tests are failing: ConflictingLocalWithSimpleLambdaParameter | 
| // Create a new scope for local functions since C# allows variables | ||
| // in the outer scope to have the same name as variables in the local function. | ||
| var parameterTokens = node.ParameterList.Parameters.Select(p => p.Identifier); | ||
| _tracker.AddIdentifiers(parameterTokens); | 
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 this actually doing the right thing? or is it now adding these identifiers as things that could potentially conflict?
Also, this seems to only be addressing parameters. not necessarily locals.
i would also expect to see changes to how lambdas are processed as they should also support the new c# semantics.
Note: Perhaps ti's ok to be adding/removing the identifiers as you have done. But when a conflict is found, we might want to see if it crosses a lambda boundary, and if so, not treat it as a conflict.
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.
You're right - the approach now visits the lambda/local function bodies to detect internal conflicts while not tracking their parameters to avoid false cross-scope conflicts. Updated in commit fec8a4e.
The key changes:
- Lambda/local function parameters are not added to the tracker (prevents cross-scope conflicts)
- Bodies are still visited (detects legitimate intra-scope conflicts)
- This handles both parameters and locals inside the functions correctly
This addresses your concerns about detecting conflicts within the lambda/local function scope while allowing the new C# shadowing semantics.
Changed the VisitSimpleLambdaExpression, VisitParenthesizedLambdaExpression, and VisitLocalFunctionStatement methods to not track identifiers from lambda parameters and local function parameters. This prevents false conflict detection when an outer variable has the same name as a lambda parameter or local function parameter, which C# now allows. Co-authored-by: CyrusNajmabadi <[email protected]>
| 
 Fixed in commit 8a5f19c. The issue was that the lambda and local function visit methods were still tracking identifiers in the same scope as outer variables. The fix is to not track lambda parameters or local function parameters at all, since C# now allows shadowing. | 
Updated the visitor methods to still visit the bodies of lambdas and local functions to properly detect conflicts within those scopes, while not tracking their parameters to avoid false cross-scope conflict detection. This addresses the concern that we need to detect conflicts between locals inside the lambda/local function, not just avoid cross-boundary conflicts. Co-authored-by: CyrusNajmabadi <[email protected]>
Summary
C# now allows variables in outer scopes to have the same name as variables in nested local functions and lambda expressions. This PR removes the incorrect conflict detection that was treating these valid renamings as conflicts.
Changes Made
LocalConflictVisitor.csVisitLocalFunctionStatementto not track parameters but still visit bodiesVisitSimpleLambdaExpressionto not track parameters but still visit bodiesVisitParenthesizedLambdaExpressionto not track parameters but still visit bodiesTechnical Details
The root cause was that
LocalConflictVisitorwas tracking lambda parameters and local function parameters in the same scope as outer variables, causing false conflicts to be reported. The fix is to:This ensures we detect legitimate conflicts within the lambda/local function scope while allowing shadowing of outer variables.
Test Results
The fix resolves the issue where renaming
ttot1incorrectly reported a conflict with a local function's variablet1.Original prompt
Fixes #60728
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.