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

Provide metadata about symbols with DefinitionLink #415

Merged
merged 5 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@rcjsuen
Copy link
Contributor

rcjsuen commented Sep 12, 2018

I've updated textDocument/definition, textDocument/implementation, and textDocument/typeDefinition, to potentially return a DefinitionLink. This new interface includes metadata about the ranges of both the originating source symbol and the target symbol. This will help provide more context to the user while navigating between different symbols in an editor.

For example, instead of simply highlighting a word, the editor can now understand that the source symbol's range extends beyond standard word boundaries. This will allow it to highlight/underline stuff like an entire java.util.List in a Java import statement.

This implements Microsoft/language-server-protocol#524.

Provide metadata about symbols with DefinitionLink
Enhanced textDocument/definition, textDocument/implementation, and
textDocument/typeDefinition to potentially return a DefinitionLink.
This new interface includes metadata about the ranges of both the
originating source symbol and the target symbol. This will help
provide more context to the user while navigating between different
symbols in an editor.

Signed-off-by: Remy Suen <remy.suen@gmail.com>

@rcjsuen rcjsuen force-pushed the rcjsuen:definitionLink branch from 88044e4 to 74d1577 Sep 12, 2018

@@ -1656,7 +1696,7 @@ export interface SignatureHelp {
* For most programming languages there is only one location at which a symbol is
* defined. If no definition can be found `null` is returned.
*/
export type Definition = Location | Location[] | null;
export type Definition = Location | Location[] | DefinitionLink | DefinitionLink[] | null;

This comment has been minimized.

@dbaeumer

dbaeumer Sep 13, 2018

Member

The deeper question that we need to decide is whether we want to deprecate Location for definitions and ask for providing a DefinitionLink instead. If so we shouldn't or them into the Defintion, but rather list them in the results.

Location is semantically equal to DefinitionLink if originSelectionRange and targetSelectionRange are not provided.

What is your opinion?

This comment has been minimized.

@rcjsuen

rcjsuen Sep 13, 2018

Contributor

@dbaeumer Your suggestion seems reasonable to me. We can add a deprecation notice to Definition then to try to help discourage people from using it.

It is too bad that we cannot simply extend DefinitionLink from Location as the attribute names don't match (and range in Location as-is is a little ambiguous compared to the more specific names in DefinitionLink).

This comment has been minimized.

@KamasamaK

KamasamaK Sep 13, 2018

It's not clear why, but in the VS Code API, @jrieken thought it would not be a good idea to "mix DefinitionLink into Definition" as is being done here. See Microsoft/vscode#54424

This comment has been minimized.

@dbaeumer

dbaeumer Sep 14, 2018

Member

@rcjsuen then lets keep them different. I talked to @jrieken about why he kept them different but there was no deeper semantic meaning behind it.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Sep 13, 2018

Other than the above comment as always a good PR.

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Sep 13, 2018

@dbaeumer I just noticed textDocument/references while going over the specification.

Should I update that one too to return Location[] | DefinitionLink[] | null?

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Sep 14, 2018

@rcjsuen let me think about the references request. Spontaneously I would say yes. But we would need a different name then since a references request returning DefinitionLink might be confusing.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Sep 24, 2018

@rcjsuen haven't forgotten you. Was busy doing other thing. Will look into this this week.

@dbaeumer dbaeumer added this to the September 2018 milestone Sep 24, 2018

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Sep 24, 2018

@dbaeumer No rush. We all have our own obligations and timelines. Microsoft/language-server-protocol#524 has also been quiet, so... shrugs

This might get delayed either way as I'll be on/off during the first half of October.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Sep 28, 2018

@rcjsuen we should return DefinitionLink from a references result as well. I checked with @mjbvz and @jrieken and they agree that this would even make sense in the vscode API.

However I dislike the name DefinitionLink if we return it from references results as well. Ideas are: LocationLink, SymbolLocation Has anyone else an idea?

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Sep 28, 2018

@dbaeumer I would say SymbolLink or LocationLink. I don't like SymbolLocation because there's an "origin" and a "target". The word "link" makes more sense for this relationship in my opinion.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Oct 2, 2018

@rcjsuen almost all LSP data types do have an URI and non are called links except DocumentLink which is in most cases used to represent links. For me the DefinitionLink as definied in the API does the following:

originSelectionRange: this expands the position passsed into the request into a range in case the editor wants to give visual feedback which might be a link / underline or soemthing else
targetUri: the target URI which is the same then Location#uri
targetSelectionRange: the target range that should be selected (same as Location#range)
targetRange: the whole range of the symbol. Did not exist on Location.

Now thinking about it again this doesn't make sense for reference results since there is no origin and no target. There might be two ranges if we want to support this (match range indicating what should be selected and a reveal range).

So may be we stick with DefinitionLink. However I am not sure which of these properties should be optional.

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Oct 4, 2018

@dbaeumer Wouldn't the origin of a textDocument/references request be the position of where the cursor is when the user invoked the request? The user is interested in what other places in the opened workspace is the symbol at the current location used, no? It seems to me that the origin is the symbol at the current cursor position and the target(s) would be the "other locations".

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Nov 20, 2018

@rcjsuen sorry for the long silence. I had a discussion with @jrieken about this lately and we decided to name this LocationLink for now. We also introduced a DeclarationProvider. This and the DefinitionProvider now return LocationLink

Regarding your comment: I am not sure I understand this. Isn't this the whole purpose of the reference result to list all locations where a symbol is referenced. And the position where the cursor is is the input param to the request. If we want to expand this to a valid range then I think this need to go onto the reference result which then might look like this

interface ReferenceResult  {
   range: Range;
   locations: Location[];
}

Does that make sense ?

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Nov 20, 2018

Hi, @dbaeumer. I presume DeclarationProvider was introduced to add support for Microsoft/language-server-protocol#605?

What you suggested with ReferenceResult makes sense to me. So we want to change textDocument/references from returning Location[] to Location[] | { range: Range, locations: Location[] } then, correct?

If so, since we are no longer reusing the new type for textDocument/references, perhaps that should be addressed in a separate PR?

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Nov 20, 2018

//---- Goto Definition -------------------------------------
/**
* A request to resolve the definition location of a symbol at a given text
* document position. The request's parameter is of type [TextDocumentPosition]
* (#TextDocumentPosition) the response is of type [Definition](#Definition) or a
* Thenable that resolves to such.
*/
export namespace DefinitionRequest {
export const type = new RequestType<TextDocumentPositionParams, Definition | null, void, TextDocumentRegistrationOptions>('textDocument/definition');
}

@dbaeumer What should we return here if we are deprecating Definition?

  1. Definition | LocationLink[] | null
  2. Definition | LocationLink | LocationLink[] | null
  3. Something else?

rcjsuen added some commits Nov 20, 2018

Do not reference new API in Definition
Signed-off-by: Remy Suen <remy.suen@gmail.com>
Rename DefinitionLink to LocationLink
Signed-off-by: Remy Suen <remy.suen@gmail.com>
Deprecate Definition in favour of LocationLink
Signed-off-by: Remy Suen <remy.suen@gmail.com>
@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Nov 20, 2018

I presume DeclarationProvider was introduced to add support for Microsoft/language-server-protocol#605?

Yes.

What you suggested with ReferenceResult makes sense to me. So we want to change textDocument/references from returning Location[] to Location[] | { range: Range, locations: Location[] } then, correct?

Yes and we should address this in another PR if this is necessary at all.

What should we return here if we are deprecating Definition?

After thinking about this I would actually not deprecate Location and therefore not Definition. I would simply introduce a LinkLocation and add a DefinitionLink in the form

type DefinitionLink = LinkLocation | LinkLocation[] | null;

and the change the return value of the request to Definition | DefinitionLink. We then mimic the same for Declaration.

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Nov 20, 2018

@dbaeumer Is the name DefinitionLink okay? I think we may want to reuse this type again for Microsoft/language-server-protocol#605 or am I mistaken?

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Nov 20, 2018

@rcjsuen I would do the same there. Have a type alias for DeclarationLink. In general I am tending more for having unique types even if they are aliases. Makes it clearer and easier to evolve.

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Nov 20, 2018

Thank you, @dbaeumer. I understand. It is the same as what you mentioned in #346 (comment).

I will do some poking at this pull request then...

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@dbaeumer dbaeumer merged commit 0cc3b8a into Microsoft:master Dec 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

1 similar comment
@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

4 similar comments
@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

@rcjsuen rcjsuen deleted the rcjsuen:definitionLink branch Dec 6, 2018

@rcjsuen

This comment has been minimized.

Copy link
Contributor

rcjsuen commented Dec 6, 2018

@dbaeumer Thank you for merging this in, Dirk. Sorry that I've been busy with work and couldn't get the chance to finish things off. :(

I don't have a strong feeling about the deprecation of Definition. Perhaps an alternative would be to simply suggest the use of LocationLink if additional contextual information can be provided/may be helpful?

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 6, 2018

I merged this in since I am in the process of adding goto declaration which will reuse LocationLink

I am still not sure if we should deprecate Definition or not just keep it. @rcjsuen strong feelings about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment