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

Add proposed textDocument/callHierarchy extension #420

Merged
merged 1 commit into from
Apr 5, 2019

Conversation

AlexTugarev
Copy link
Contributor

@AlexTugarev AlexTugarev commented Oct 5, 2018

This PR proposes a textDocument/callHierarchy request sent from the client to the server to request the call hierarchy for a symbol at the given text document position.

A prototype implementation for TypeScript LS is implemented in typescript-language-server/typescript-language-server#98.

Related LSP issue: microsoft/language-server-protocol#468

protocol/src/protocol.calls.proposed.ts Outdated Show resolved Hide resolved
protocol/src/protocol.calls.proposed.ts Outdated Show resolved Hide resolved
types/src/main.ts Outdated Show resolved Hide resolved
@dbaeumer
Copy link
Member

A call hierarchy is IMO very similar to a type hierarchy. I can be expanded in two ways and we should be able to resolve the hierarchy lazy. So I would mimic what I have suggested for the TypeHierarchy here: #346 (comment)

@AlexTugarev
Copy link
Contributor Author

A call hierarchy is IMO very similar to a type hierarchy. I can be expanded in two ways and we should be able to resolve the hierarchy lazy. So I would mimic what I have suggested for the TypeHierarchy here: #346 (comment)

@dbaeumer, thanks for the feedback! So, you suggest to have two separate request types for the lazy loading? In the beginning to send CallHierarchyParams extends TextDocumentPositionParams and then continue to resolve by sending ResolveCallHierarchyItemParams. I can update my proposal quite easily to meet this.

@dbaeumer
Copy link
Member

Yes. That is the idea.

@AlexTugarev
Copy link
Contributor Author

@dbaeumer, I have updated the proposal. Please have a look!

AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Jan 22, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Jan 23, 2019
@AlexTugarev AlexTugarev changed the title Add proposed textDocument/calls extension Add proposed textDocument/callHierarchy extension Jan 23, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Jan 23, 2019
@AlexTugarev
Copy link
Contributor Author

I have updated the PR again and added a separate callHierarchy/resolve request (instead of having an either type for the textDocument/callHierarchy request's parameter.)

AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Jan 23, 2019
@dbaeumer
Copy link
Member

@AlexTugarev the big change that happend (see comment #420 (comment) and #420 (comment)) is that we moved from a vertex based result to an edge based result. The cost is that the result contains duplicate information (e.g. the [Locaction[], Symbol] discussion). This is fine for me since the edge based model brings quite some advantages.

@AlexTugarev
Copy link
Contributor Author

@dbaeumer, thanks! This sounds good. I'll update this proposal shortly.

@aeschli
Copy link
Contributor

aeschli commented Mar 29, 2019

@tsmaeder I allowed Range in provideCalls so that you can pass in a user selection or the range of a CallHierarchySymbol.

@AlexTugarev
Copy link
Contributor Author

pass in a user selection or the range of a CallHierarchySymbol

@aeschli, which part would be responsible to check if there is only a single reference (call) or a definition in that pos Range?
and what is the downside of using selectionRange.start, besides that it might break when using a user selection?

@aeschli
Copy link
Contributor

aeschli commented Mar 29, 2019

@AlexTugarev When the symbol at the position (or range) resolves in multiple definitions, the provideCalls result will contain calls to/from all of the definitions. The UI can decide to group the results under multiple root nodes (one for each definition).

Position is probably also good enough. The definition provider and the reference provider also just take a position. I added it for convenience, and future considerations: There might be cases where a range would allow the user to be more specific on what to look for. E.g. in a reference like List one could say that selecting List would resolve to the non-parameterized List, while List<String> the list of strings.
But given that the definition provider and the reference provider don't support such capabilities, we are probably good to go with just Position.

@dbaeumer
Copy link
Member

dbaeumer commented Apr 4, 2019

@AlexTugarev we plan to ship a new stable version of VS Code end of this week. So if you have something that corresponds to the new API I could create a new next version with the call hierarchy support as proposed so that people can try it out.

@AlexTugarev
Copy link
Contributor Author

@dbaeumer, that sounds good. I'll update it this PR tonight.

@dbaeumer
Copy link
Member

dbaeumer commented Apr 5, 2019

@AlexTugarev ping me when the changes are available

Adds the `textDocument/callHierarchy` request sent from the client to the server to request the call hierarchy for a symbol at the given text document position.

LSP issue: language-server-protocol#468

Signed-off-by: Alex Tugarev <[email protected]>
@AlexTugarev
Copy link
Contributor Author

@dbaeumer, please have a look.

I'm not sure about the client/server part regarding the proposed API. I cannot find your example how to create it.

@dbaeumer
Copy link
Member

dbaeumer commented Apr 5, 2019

On the server part we usually register a request handler as long as the protocol is proposed.

I will merge the PR in and will then prepare a new next version with the selection range proposal.

@dbaeumer dbaeumer merged commit 54f6caa into microsoft:master Apr 5, 2019
@AlexTugarev
Copy link
Contributor Author

Thanks @dbaeumer!

@MaskRay
Copy link

MaskRay commented Jul 21, 2019

I think the proposal has several deficiency that require further improvement. Comments inlined below:

export interface CallHierarchyCall {
  // If there are multiple `CallHierarchyCall` elements with the same `from`, will the `from: CallHierarchySymbol` be duplicated multiple times?
  range: Range;
  from: CallHierarchySymbol;
  to: CallHierarchySymbol;
}
result: CallHierarchyCall[] | null

export interface CallHierarchySymbol {
  // There is no id field. `name` or `detail` may not identify a specific target that can be suitable for further requests (e.g. node expansion).
  name: string;
  detail?: string;
  kind: SymbolKind;
  uri: string;
  range: Range;
  selectionRange: Range;
}

In ccls (https://github.com/MaskRay/ccls/blob/master/src/messages/ccls_call.cc), we use:

struct Out_cclsCall {
  Usr usr;  // language server specific
  std::string id;
  std::string_view name;
  Location location;
  CallType callType = CallType::Direct;
  int numChildren;
  // Empty if the |levels| limit is reached.
  std::vector<Out_cclsCall> children;
};

The method $ccls/call was designed so that the result can either be a hierarchy or a flattened list (like textDocument/references).

@dbaeumer
Copy link
Member

dbaeumer commented Aug 6, 2019

@MaskRay can you please open a GitHub issue to discuss this.

@HighCommander4
Copy link

Does this being merged mean the extension is now an official part of LSP? Or are there additional steps required for that to happen?

@rcjsuen
Copy link
Contributor

rcjsuen commented Sep 3, 2019

@HighCommander4 I wouldn't consider anything final until microsoft/language-server-protocol#468 has been closed. I think people look to the website for the final say in general.

@HighCommander4
Copy link

HighCommander4 commented Sep 3, 2019

I wouldn't consider anything final until microsoft/language-server-protocol#468 has been closed.

Thanks. Do you know what is needed for that to happen? Is it just a matter of someone writing a PR?

Another question: is the proposed spec (the protocol.callHierarchy.proposed.md file from the commit in this PR) hosted in rendered form somewhere, for implementers / early adopters to look at?

@rcjsuen
Copy link
Contributor

rcjsuen commented Sep 9, 2019

@HighCommander4

  1. From what I understand, yes, it's just a matter of someone writing up the PR. But note that at the end of the day it is Microsoft's call regarding when to merge it and make it an official new LSP release (be it 3.x or 4.0). Note that I am not expecting the next release to be 4.0 but that's just me and I don't speak for anyone else in the LSP community.
  2. You can find that file rendered online in GitHub but I do not believe it is viewable through any "non-GitHub website" otherwise.

@KamasamaK
Copy link

As a consequence of it not being finalized yet, it's important to understand that it's still subject to change. There continue to be discussions on how best to finalize this API, but since this PR is merged this is probably not the best place. Maybe create a new issue.

Also, while there is nothing to actually dictate this, it is generally the case that the LSP API is closely aligned with the VS Code API, so you can see microsoft/vscode#70231 for how the API may change as well.

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

Successfully merging this pull request may close these issues.