Improve property symbol parenting#450
Open
nicolas-guichard wants to merge 13 commits intosourcegraph:mainfrom
Open
Improve property symbol parenting#450nicolas-guichard wants to merge 13 commits intosourcegraph:mainfrom
nicolas-guichard wants to merge 13 commits intosourcegraph:mainfrom
Conversation
This especially brings Typescript 5.9.3 and support for es2024.
When a symbol appears at line start, it used to have an extra carret: ``` symbol //^^^^^ symbol ``` Now it doesn't anymore: ``` symbol //^^^^ symbol ``` If the symbol is too short, it gets a north-west arrow: ``` s //↖ symbol ```
Currently, assigning to the prototype property of an object creates and references many local symbols which don't allow navigation across multiple files.
Currently, this appears as many local symbols, or even worse the `exports` in `module.exports` is marked as a reference to the whole file.
When we visit a function definition, we actual visit the identifier and pass that to getTSSymbolAtLocation and visitSymbolOccurrence. In the constructor case however, we were passing the whole constructor node and callees would handle constructors as a special case. Instead, visit now gets the constructor keyword out of the constructor node and pass that to other functions, which don't need to special-case it anyumore.
This big ternary operator was hard to read IMO.
This avoids spurious local symbols, especially when assigning to a property of a function.
The comment was wrong: it says blocks are local symbols that reached this fallback but they are actually filtered out earlier to return empty symbols. This greatly reduces the number of local symbols for things that aren't symbols at all.
This will allow extending getParent to fix a couple of cases were the parent node in the AST isn't the semantic parent.
The parent of a property in a property definition isn't the parent in
the AST.
Take this example:
```js
obj.property = {a: 3}
```
Previously `property`'s parent would be the `obj.property`
`PropertyAccessExpression` node and `a`'s parent would be the
`BinaryExpression` node.
Neither of them really are symbols. Instead `property`'s parent should
be `obj` ie the `expression` of the `PropertyAccessExpression` node
and `a`'s parent should be `obj.property` ie the `left` part of the
`BinaryExpression` node.
When we export a class or function expression (as opposed to class or function definition), the exported symbol should be used as the canonical definition.
nicolas-guichard
force-pushed
the
property-parenting
branch
from
e904d7c to
2567d83
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In
FileIndexer'sscipSymbol, we used to try and get a symbol parent from its AST nodes's parent, which is wrong in many cases. This improves the situation for property definitions and assignments so thatobj.property = {a: 1}correctly reports definitions forobj.property.andobj.property.a.instead of local symbols.Along the way this attempts at reducing the number of spurious local symbols.
Conflicts/depends on #449
Fixes #409
Test plan
yarn testpasses