Consistently map reparsed JSDoc nodes in GetReparsedNodeForNode
#2586
Conversation
ahejlsberg
requested review from
DanielRosenwasser,
Copilot and
jakebailey
and removed request for
Copilot
DanielRosenwasser
left a comment
DanielRosenwasser
left a comment
There was a problem hiding this comment.
Looks like there are some nice improvements on find-all-references/go-to-definition, so that's great.
Can you add both examples (the original repro from the issue, and the one you are trying to fix with the parent cache) to the test suite?
Would you say that generally all exposed checker methods should guard on nodes and try to get the reparsed node?
internal/ast/ast.go
Outdated
| key := TokenCacheKey{Loc: loc, Parent: parent} | ||
| if token, ok := node.tokenCache[key]; ok { |
There was a problem hiding this comment.
Do we know precisely what was happening that requires caching on the parent?
There was a problem hiding this comment.
Yes, I'm realizing that the completions logic is asking for tokens in both JSDoc comments and their reparsed clones. Such nodes have the same location but different parents. I'm changing the logic to only ask for tokens in the original JSDoc nodes. That will then allow me to get rid of the parent in the cache key.
| // === /foo.js === | ||
| // /** | ||
| // * @overload | ||
| // * @param {number} [|x|] |
There was a problem hiding this comment.
Not sure if this is a regression - what changed here?
|
@copilot try again |
|
@DanielRosenwasser I've opened a new pull request, #2588, to work on those changes. Once the pull request is ready, I'll request review from you. |
With this PR we track reparsed clones of JSDoc nodes such that
ast.GetReparsedNodeForNodecan consistently map from JSDoc nodes to their reparsed clones (which are the ones that are assigned symbols by the binder). The PR changes thechecker.GetSymbolAtLocationandchecker.TryGetThisTypeAtExfunctions to useast.GetReparsedNodeFromNode.Fixes #2490.