fix(2932): handle parameter-property ref to method with same name#2934
fix(2932): handle parameter-property ref to method with same name#2934a-tarasyuk wants to merge 11 commits intomicrosoft:mainfrom
Conversation
DanielRosenwasser
left a comment
DanielRosenwasser
left a comment
There was a problem hiding this comment.
I was trying to debug this but I just didn't understand why Strada doesn't have exactly the same issues. Maybe it's related to changes we have in #693 ?
I don't like not fully understanding that part, but either way, it seems like this fix is at least closer (it's possible that you need to restore the logic to pick the "other" symbol).
internal/fourslash/tests/findAllRefsParameterPropertyDeclaration_test.go
Outdated
Show resolved
Hide resolved
internal/fourslash/tests/findAllRefsParameterPropertyDeclaration_test.go
Outdated
Show resolved
Hide resolved
internal/fourslash/tests/findAllRefsParameterPropertyDeclaration_test.go
Outdated
Show resolved
Hide resolved
| if res := fromRoot(paramProp1); res != nil { | ||
| return res, entryKindNode | ||
| } | ||
| if res := fromRoot(paramProp2); res != nil { | ||
| return res, entryKindNode | ||
| } |
There was a problem hiding this comment.
Isn't this different from the original logic of picking the "other" symbol based on the original root symbol?
There was a problem hiding this comment.
@DanielRosenwasser, thanks for the additinal test cases. Strada uses PropertyExcludes = None, it was changed
typescript-go/internal/ast/symbolflags.go
Line 59 in 0a7d128
The symbol contains two declarations, and valueDeclaration points to the method declaration rather than the parameter. As a result, this condition is always false in the crash scenario.
In tsgo, however, valueDeclaration points to the parameter declaration, so the condition is evaluated and proceeds. To correctly handle parameter-property/member name collisions so FAR returns both related declarations.
There was a problem hiding this comment.
Right, but the old logic used to be "pick the property symbol if we're starting with a parameter symbol, otherwise pick the parameter symbol".
I guess the test case shows that this works the same as before - but I don't understand how.
There was a problem hiding this comment.
Okay, I think I understand- let me know if it also makes sense to you.
There are two calls to forEachRelatedSymbol. In the first, populateSearchSymbolSet basically goes through all related symbols and adds them to an initial search set. The second pass is getRelatedSymbol where previously-seen symbols are added but thrown away.
So you're technically doing more work now, but you're still indirectly achieving the "one or the other" logic.
There was a problem hiding this comment.
It would be nice to still boil this down to a single call though.
internal/fourslash/tests/findAllRefsParameterPropertyWithConflictingMember_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Fixes a language-service crash in find all references when a constructor parameter property shares a name with an earlier non-property class member (issue #2932), and adds a regression fourslash test + baseline to lock in the behavior.
Changes:
- Adjust
refState.forEachRelatedSymbolto tolerate parameter-property “pair” symbols where the class-side symbol is a non-property class member (e.g. method/accessor), avoiding the panic. - Add a new fourslash test covering the conflicting-member scenario (method + parameter property, and accessor + parameter property).
- Add the corresponding reference baseline output for
findAllReferences.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/ls/findallreferences.go |
Changes parameter-property related-symbol handling to avoid panicking when the “property” symbol is actually another class member kind. |
internal/fourslash/tests/findAllRefsParameterPropertyWithConflictingMember_test.go |
New regression test exercising find-all-refs across method/accessor + parameter-property name conflicts. |
testdata/baselines/reference/fourslash/findAllReferences/findAllRefsParameterPropertyWithConflictingMember.baseline.jsonc |
New golden baseline for the added fourslash test. |
|
It seems with the latest commits, we're no longer testing find-all-refs on usages as well. |
|
@DanielRosenwasser, I updated test to use ranges. Did you mean this, or were you referring to adding the additional cases? |
|
@a-tarasyuk you don't have to close the PR, I just wanted to spend a bit longer understanding it |
Fixes #2932