-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add isDefinition to references #9148
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
Clients can now easily tell if the reference is to a definition or a usage.
@@ -304,6 +304,11 @@ declare namespace ts.server.protocol { | |||
* True if reference is a write location, false otherwise. | |||
*/ | |||
isWriteAccess: boolean; | |||
|
|||
/** | |||
* True if reference is a definition, false otherwise. |
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.
undefined
otherwise?
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.
sorry looking at the code below, it is always set, so why make it optional?
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.
I thought it was necessary for API additions. I'll make it required if that's not the case.
i would add a test for a binding pattern, import/export, string literal names property, computed property, and numeric named property. |
For the deprecated getOccurrencesAtPosition, isDefinition is always false.
@@ -6740,7 +6741,7 @@ namespace ts { | |||
fileName: node.getSourceFile().fileName, | |||
textSpan: createTextSpanFromBounds(start, end), | |||
isWriteAccess: isWriteAccess(node), | |||
isDefinition: isDeclarationName(node) | |||
isDefinition: isDeclarationName(node) || node.parent.kind === SyntaxKind.ComputedPropertyName |
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.
why not push this into isDeclarationName
?
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.
it's not true all the time -- isDeclarationName additionally checks that node.parent.name === node
which needs to be generally true for something to be a valid declaration. Computed property names don't meet this requirement because often we can't calculate the name at compile time. In this code path, however, we know that the computed property name already, so I added the special case here only .
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.
I tried pushing it further, but computed property's symbols started disappearing from our symbol baselines. I did discover isLiteralCompuatedPropertyDeclarationName
, so I switched to that instead of the direct check of SyntaxKind.
👍 |
This allows clients to distinguish definitions from usages. I used this to change tide to behave more like Resharper: tide-references now jumps directly to a single usage if only one usage and one definition exist. For example,
If you get all references at /1/, you'll jump straight to the
return
line. If you get all references at /2/, you'll get the normal buffer listing references. Because of the deduping logic I wrote for tide, /3/ will behave the same as /1/ since all references are on the same line. But that's not necessarily the right thing.@Andy-MS I believe you were interested in this for Sublime too. @zhengbli, can you look at this and say whether it's the right approach?