Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Include declarationSpan as relevant declaration span when defintion or other places are declaration name #31587

Merged
merged 39 commits into from
Jun 18, 2019
Merged

Include declarationSpan as relevant declaration span when defintion or other places are declaration name #31587

merged 39 commits into from
Jun 18, 2019

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 24, 2019 •
edited

Fixes #30849

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@sheetalkamat sheetalkamat changed the title Include definitionSpan as relevant declaration span when defintion or other places are declaration name Include declarationSpan as relevant declaration span when defintion or other places are declaration name May 24, 2019
@mjbvz mjbvz mentioned this pull request May 28, 2019
Closed
@sheetalkamat
More tests
6a961b5
@sheetalkamat
Rename declarationRange to declarationRangeIndex
8948fe4
@sheetalkamat
More test fixes
f37ae23
@sheetalkamat
Use import export sepcifier as declaration for the property name of i…
bbfbf8f 
…mport export as well
@sheetalkamat
More tests
2fc1143
@sheetalkamat
Export assignment identifier use ExportAssigment as declaration
15ce996
@sheetalkamat
More tests
bbd2d00
@sheetalkamat
For property name of binding element use binding element as preview node
6c04a0d
@sheetalkamat
More tests
bcf7752
@sheetalkamat
Show property assignment for special property assignments in js files
9703b3d
@sheetalkamat
More test cases
b61813e
@sheetalkamat
Use for-of declaration list + expression as span for preview
dfb613c
@sheetalkamat
More tests
c0537d9
@sheetalkamat
Handle destructuring assignments
0fee3b0
@sheetalkamat
More tests
01bbc4d
@sheetalkamat
Handle computed property names
e41533a
@sheetalkamat
More Tests
34624a0
@sheetalkamat
More tests
424f2c9
@sheetalkamat
Handle when declarationSpan from declarationNode is undefined
eaeeb06
@sheetalkamat
More tests
cc1cb54
@sheetalkamat
Set declaration span only if its not same as own span
004488c
@sheetalkamat
More tests
35c0499
@sheetalkamat sheetalkamat marked this pull request as ready for review June 7, 2019 16:36
@weswigham
Copy link
Member

weswigham commented Jun 10, 2019 •
edited

Rather than all this reverse engineering to find the containing declaration in findAllReferences.ts, shouldn't we just amend createDefinitionInfoFromName in goToDefinition.ts and definitionToReferencedSymbolDefinitionInfo and getReferencedSymbolsForModule in findAllReferences.ts to push the declaration forward? We already explicitly collapse down to just the name if available in these places - collapsing down to the name only to walk back up and find the body again later seems silly.

@sheetalkamat
Copy link
Member Author

@weswigham it already directly gets the declaration from the declaration of node in definitionToReferencedSymbolDefinitionInfo and in createDefinitionInfoFromName. nodeEntry just turns out to be common place to calculate other declarations for the references added as they come from various places which are mostly through checking symbol we are searching === symbol at location, rather than looking at the declaration

@weswigham
Copy link
Member

weswigham commented Jun 12, 2019 •
edited

Ah, OK - getDeclarationForDeclarationSpan is also mapping declarations like variable declarations back into their containing variable statements (which is not a declaration) - I guess I just find the name misleading then, since what we're really searching for isn't the declaration (we usually already have that) but instead some containing broader context node. In the original issue @mjbvz also called it a definitionSpan or contextSpan - I think the later is better (even if it is conceptually vague), since it doesn't conflate the span with the declarations or definitions in use elsewhere in the service API.

weswigham
weswigham reviewed Jun 12, 2019
View reviewed changes
src/services/services.ts Outdated Show resolved Hide resolved
@sheetalkamat
Copy link
Member Author

My preference has been contextSpan so will change to that. Thanks.

@sheetalkamat sheetalkamat requested a review from mjbvz June 14, 2019 15:49
@sheetalkamat
Copy link
Member Author

Ping...

mjbvz added a commit to microsoft/vscode that referenced this pull request Jun 17, 2019
@mjbvz
Provide full TS symbol range when previewing definitions in VSCode
459939b 
Fixes #72017

Has two fixes:

- Hooks up the JS/TS extension to consume the full symbol range provided by microsoft/TypeScript#31587

- Makes the go the definition mouse implementation use the locationLink to compute the preview range. If a`targetSelectionRange` is provided, this means we use the normal `range` to get the preview range
@mjbvz mjbvz mentioned this pull request Jun 17, 2019
Merged
@mjbvz
Copy link
Contributor

mjbvz commented Jun 17, 2019

Thanks @sheetalkamat! I've pushed a PR to VS Code to make use of this new property for definitions: microsoft/vscode#75659 You will be able to test this in VS Code insiders once that pr is merged

The new API looks good to me, just make sure protocol.d.ts gets updated too at some point

@sheetalkamat sheetalkamat merged commit 7ed3896 into master Jun 18, 2019
@weswigham weswigham deleted the definitionSpan branch June 18, 2019 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham weswigham approved these changes

@mjbvz mjbvz mjbvz approved these changes

@sandersn sandersn Awaiting requested review from sandersn

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

@rbuckton rbuckton Awaiting requested review from rbuckton

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include both symbol span and whole definition span for tsserver definitions
5 participants
@sheetalkamat @typescript-bot @weswigham @mjbvz @andrewbranch