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

Prototype definition and span API #40045

Closed
wants to merge 1 commit into from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Dec 11, 2017

Problem
See #10037

Proposal
Add a new DefinitionAndSpan class to the VSCode API. This bundles up a location and the span of the defining symbol. Allow definition providers to return either a Location or a DefinitionAndSpan

**Problem**
See microsoft#10037

**Proposal**
Add a new `DefinitionAndSpan` class to the VSCode API. This bundles up a location and the span of the defining symbol
@mjbvz mjbvz self-assigned this Dec 11, 2017
@mjbvz mjbvz requested a review from jrieken December 11, 2017 20:35
@@ -1896,6 +1896,13 @@ declare module 'vscode' {
resolveCodeLens?(codeLens: CodeLens, token: CancellationToken): ProviderResult<CodeLens>;
}

export class DefinitionAndSpan {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may want to make this use a single definition instead:

 class DefinitionAndSpan {
 		span: Location;
 		definition: Location;
}

My original thinking was to bundle the concept of a definition into a class, i.e. this text range corresponds to these X definitions. However I'm not sure this makes sense since we later flatten out the definition list in the UI anyways

@@ -1896,6 +1896,13 @@ declare module 'vscode' {
resolveCodeLens?(codeLens: CodeLens, token: CancellationToken): ProviderResult<CodeLens>;
}

export class DefinitionAndSpan {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I'm thinking of renaming this type. Something like DefinitionAndDefiningSpan or BoundedDefinition perhaps

@@ -50,19 +66,19 @@ function getDefinitions<T>(
}


export function getDefinitionsAtPosition(model: IReadOnlyModel, position: Position): TPromise<Location[]> {
export function getDefinitionsAtPosition(model: IReadOnlyModel, position: Position): TPromise<DefinitionAndSpan[]> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change currently breaks the vscode.executeDefinitionProvider command. It previously returned Location[] but will now return a DefinitionAndSpan[]

@jrieken
Copy link
Member

jrieken commented Dec 13, 2017

Not a fan... It's seems overly specific for that one bug. It is under-specified with respect to what the span or source is. Is it just the symbol-name or the range of the symbol definition? The latter would be useful for the preview-declaration-in-hover feature where we face a similar challenge as described in #10037.

If this is just about fixing #10037 we should look into something simpler, e.g. not just checking the first at the position but also the token and take the token if it's truly (w/o whitespace) larger than the word. Or use some the logical-selection-logic (F1 > Expand Select).

Wrt new API I'd favour something more AST'ish like a new provider that can answer questions like 'What's the range of the symbol/token at this position?', 'What's the range of the container at this position?' etc. That could solve this issue and improve the preview-logic (and maybe even the shrink/expand selection feature)

@mjbvz
Copy link
Collaborator Author

mjbvz commented Dec 13, 2017

@jrieken Yes the intent here is that we return the symbol-name range for the hover. The main use case I have is for strings in TS, either import paths or string literal types. Another example without strings would be markdown link definitions, such as some ref:

[a][some ref]

[some ref]: http://example.com

I'd be interested to see if any other extension authors have also run into this limitation

My first though was to use the tokens but that seems specific to #10037. The expand select logic would actually do the right thing for both of my examples but @bpasero's comments on #10037 suggests this could also break a lot of existing API users if we aren't careful

Can you explain the other APIs where this additional information would be helpful. The new provider option seems promising but I'm not sure if it makes sense to decouple the defining symbol span from the definitions in this way (for example, are there cases where a language would want to return different ranges for a hover vs a definition?). Maybe a new method on DefinitionProvider?

mjbvz added a commit that referenced this pull request Mar 8, 2018
Supersedes #40045

**Problem**
See #10037

**Proposal**
Adds a new `resolveDefiniotionContext` to `DefinitionProvider`. This allows a definition provider to return the defining symbol span.
@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 8, 2018

Superseded by #45249

@mjbvz mjbvz closed this Mar 8, 2018
@mjbvz mjbvz mentioned this pull request Apr 16, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants