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.

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

Respect the client capability "textDocument.definition.linkSupport." #1875

Conversation

andlrc
Copy link
Contributor

@andlrc andlrc commented Mar 9, 2023

If the client doesn't explicitly tells the server that they have this capability then a Location should be send instead of LocationLink as a reply to a "Goto Definition Request".

See the relevant documentation:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_definition

Closes #1874

@google-cla
Copy link

google-cla bot commented Mar 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@andlrc andlrc force-pushed the respect-the-specification-in-regards-to-textDocument.declaration.linkSupport branch from 26aad5a to ac5d422 Compare March 9, 2023 01:14
integration/lsp/test_utils.ts Outdated Show resolved Hide resolved
@@ -882,6 +892,14 @@ export class Session {
if (!definitions) {
return null;
}

const clientSupportsLocationLinks =
this.clientCapabilities.textDocument?.definition?.linkSupport ?? false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.clientCapabilities.textDocument?.definition?.linkSupport ?? false;
this.clientCapabilities.textDocument?.typeDefinition?.linkSupport ?? false;

nit: I think technically this should be typeDefinition though I doubt these would ever truly be different in the client capabilities. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_typeDefinition

integration/lsp/test_utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@atscott atscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than 1 technicality in onTypeDefinition. Thanks for the fix!

@andlrc andlrc force-pushed the respect-the-specification-in-regards-to-textDocument.declaration.linkSupport branch from ac5d422 to 21c5954 Compare March 9, 2023 07:30
@angular-robot angular-robot bot requested a review from atscott March 9, 2023 07:31
@andlrc andlrc changed the title Respect the client capability "textDocument.declaration.linkSupport." Respect the client capability "textDocument.definition.linkSupport." Mar 9, 2023
@andlrc
Copy link
Contributor Author

andlrc commented Mar 9, 2023

Looks good other than 1 technicality in onTypeDefinition. Thanks for the fix!

Thanks for taking the time to guide me and review the code.

The main concern that I could have with this PR is that we need to ensure that the language server client running inside vscode (or any other consumer) announces their client capabilities correctly, as in including linkSupport in the object, or gracefully handles both Location and LocationLink.

I'm not sure how this can be tested in vscode, or if the automatic tests already covers this.

@atscott
Copy link
Collaborator

atscott commented Mar 9, 2023

The main concern that I could have with this PR is that we need to ensure that the language server client running inside vscode (or any other consumer) announces their client capabilities correctly, as in including linkSupport in the object, or gracefully handles both Location and LocationLink.

I checked out this PR and double checked that it's working in VSCode and that the VSCode client correctly sends its capabilities.

I'm not sure how this can be tested in vscode, or if the automatic tests already covers this.

Unfortunately the integration tests which would actually launch the VSCode client with a local build have been disabled due to flakiness. Some day I hope to resurrect them but at the moment, tests which need to exercise end-to-end behavior like interactions between client and server are manual.

…on,typeDefinition}.linkSupport."

If the client doesn't explicitly tells the server that they have this
capability then a `Location` should be send instead of `LocationLink` as
a reply to a "Goto Declaration Request".

See the relevant documentation:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_declaration
@atscott atscott force-pushed the respect-the-specification-in-regards-to-textDocument.declaration.linkSupport branch from 21c5954 to 5578c0f Compare March 9, 2023 16:52
@atscott atscott added target: patch This PR is targeted for the next patch release action: merge Ready to merge labels Mar 9, 2023
@atscott atscott merged commit 8751840 into angular:main Mar 9, 2023
atscott pushed a commit that referenced this pull request Mar 9, 2023
…on,typeDefinition}.linkSupport." (#1875)

If the client doesn't explicitly tells the server that they have this
capability then a `Location` should be send instead of `LocationLink` as
a reply to a "Goto Declaration Request".

See the relevant documentation:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_declaration

(cherry picked from commit 8751840)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge Ready to merge target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always sending LocationLink and not respecting textDocument.definition.linkSupport
2 participants