-
Notifications
You must be signed in to change notification settings - Fork 826
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
Support Type Hierarchy #1231
Support Type Hierarchy #1231
Changes from 6 commits
4c847a4
a2be815
21db26e
d4cc98e
2aa99a1
03ee936
8cab387
90090af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1927,6 +1927,13 @@ export interface TextDocumentClientCapabilities { | |
* @since 3.16.0 | ||
*/ | ||
moniker?: MonikerClientCapabilities; | ||
|
||
/** | ||
* Capabilities specific to the various type hierarchy requests. | ||
* | ||
* @since 3.17.0 | ||
*/ | ||
typeHierarchy?: TypeHierarchyClientCapabilities; | ||
} | ||
``` | ||
|
||
|
@@ -2419,6 +2426,14 @@ interface ServerCapabilities { | |
} | ||
} | ||
|
||
/** | ||
* The server provides type hierarchy support. | ||
* | ||
* @since 3.17.0 | ||
*/ | ||
typeHierarchyProvider?: TypeHierarchyOptions | ||
| TypeHierarchyRegistrationOptions; | ||
|
||
/** | ||
* Experimental server capabilities. | ||
*/ | ||
|
@@ -8173,6 +8188,172 @@ export interface Moniker { | |
} | ||
``` | ||
|
||
#### <a href="#textDocument_prepareTypeHierarchy" name="textDocument_prepareTypeHierarchy" class="anchor">Prepare Type Hierarchy Request (:leftwards_arrow_with_hook:)</a> | ||
|
||
> *Since version 3.17.0* | ||
|
||
The type hierarchy request is sent from the client to the server to return a type hierarchy for the language element of given text document positions. Will return `null` if the server couldn't infer a valid type from the position. The type hierarchy requests are executed in two steps: | ||
|
||
1. first a type hierarchy item is prepared for the given text document position. | ||
1. for a type hierarchy item the supertype or subtype type hierarchy items are resolved. | ||
|
||
_Client Capability_: | ||
|
||
* property name (optional): `textDocument.typeHierarchy` | ||
* property type: `TypeHierarchyClientCapabilities` defined as follows: | ||
|
||
```typescript | ||
interface TypeHierarchyClientCapabilities { | ||
/** | ||
* Whether implementation supports dynamic registration. If this is set to | ||
* `true` the client supports the new `(TextDocumentRegistrationOptions & | ||
* StaticRegistrationOptions)` return value for the corresponding server | ||
* capability as well. | ||
*/ | ||
dynamicRegistration?: boolean; | ||
} | ||
``` | ||
|
||
_Server Capability_: | ||
|
||
* property name (optional): `typeHierarchyProvider` | ||
* property type: `TypeHierarchyOptions | TypeHierarchyRegistrationOptions` where `TypeHierarchyOptions` is defined as follows: | ||
|
||
```typescript | ||
export interface TypeHierarchyOptions extends WorkDoneProgressOptions { | ||
/** | ||
* The server supports for providing an inheritance tree. | ||
*/ | ||
inheritanceTreeSuppport?: boolean; | ||
} | ||
``` | ||
|
||
_Registration Options_: `TypeHierarchyRegistrationOptions` defined as follows: | ||
|
||
```typescript | ||
export interface TypeHierarchyRegistrationOptions extends | ||
TextDocumentRegistrationOptions, TypeHierarchyOptions, | ||
StaticRegistrationOptions { | ||
} | ||
``` | ||
|
||
_Request_: | ||
|
||
* method: 'textDocument/prepareTypeHierarchy' | ||
* params: `TypeHierarchyPrepareParams` defined as follows: | ||
|
||
```typescript | ||
export interface TypeHierarchyPrepareParams extends TextDocumentPositionParams, | ||
WorkDoneProgressParams { | ||
} | ||
``` | ||
|
||
_Response_: | ||
|
||
* result: `TypeHierarchyItem[] | null` defined as follows: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should we allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Thanks for the comment, I'll add a description in the next commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
```typescript | ||
export interface TypeHierarchyItem { | ||
/** | ||
* The name of this item. | ||
*/ | ||
name: string; | ||
|
||
/** | ||
* The kind of this item. | ||
*/ | ||
kind: SymbolKind; | ||
|
||
/** | ||
* Tags for this item. | ||
*/ | ||
tags?: SymbolTag[]; | ||
|
||
/** | ||
* More detail for this item, e.g. the signature of a function. | ||
*/ | ||
detail?: string; | ||
|
||
/** | ||
* The resource identifier of this item. | ||
*/ | ||
uri: DocumentUri; | ||
|
||
/** | ||
* The range enclosing this symbol not including leading/trailing whitespace | ||
* but everything else, e.g. comments and code. | ||
*/ | ||
range: Range; | ||
|
||
/** | ||
* The range that should be selected and revealed when this symbol is being | ||
* picked, e.g. the name of a function. Must be contained by the | ||
* [`range`](#TypeHierarchyItem.range). | ||
*/ | ||
selectionRange: Range; | ||
|
||
/** | ||
* A data entry field that is preserved between a type hierarchy prepare and | ||
* supertypes or subtypes requests. It could also be used to identify the | ||
* type hierarchy in the server, helping improve the performance on | ||
* resolving supertypes and subtypes. | ||
*/ | ||
data?: unknown; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense for the prepare response to be an object (versus an array), and have a single data field? I guess I'm not sure to what extent the full call needs data versus each individual item (where if you really just wanted the same data, you'd have to dupe it on each of them). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, in clangd, what we put in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we need a data field in each individual item would depend on server. If server could hold or cache the whole type hierarchy and use it to find a type in coming requests, one data field is OK, but if server wants to use |
||
} | ||
``` | ||
|
||
* error: code and message set in case an exception happens during the 'textDocument/prepareTypeHierarchy' request | ||
|
||
#### <a href="#typeHierarchy_supertypes" name="typeHierarchy_supertypes" class="anchor">Type Hierarchy Supertypes(:leftwards_arrow_with_hook:)</a> | ||
|
||
> *Since version 3.17.0* | ||
|
||
The request is sent from the client to the server to resolve the supertypes for a given type hierarchy item. Will return `null` if the server couldn't infer a valid type from `item` in the params. The request doesn't define its own client and server capabilities. It is only issued if a server registers for the [`textDocument/prepareTypeHierarchy` request](#textDocument_prepareTypeHierarchy). | ||
|
||
_Request_: | ||
|
||
* method: 'typeHierarchy/supertypes' | ||
* params: `TypeHierarchySupertypesParams` defined as follows: | ||
|
||
```typescript | ||
export interface TypeHierarchySupertypesParams extends | ||
WorkDoneProgressParams, PartialResultParams { | ||
/** | ||
* If this is set to `true`, The request only asks for super class. | ||
*/ | ||
classOnly?: boolean; | ||
item: TypeHierarchyItem; | ||
} | ||
``` | ||
_Response_: | ||
|
||
* result: `TypeHierarchyItem[] | null` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does |
||
* partial result: `TypeHierarchyItem[]` | ||
* error: code and message set in case an exception happens during the 'typeHierarchy/supertypes' request | ||
|
||
#### <a href="#typeHierarchy_subtypes" name="typeHierarchy_subtypes" class="anchor">Type Hierarchy Subtypes(:leftwards_arrow_with_hook:)</a> | ||
|
||
> *Since version 3.17.0* | ||
|
||
The request is sent from the client to the server to resolve the subtypes for a given type hierarchy item. Will return `null` if the server couldn't infer a valid type from `item` in the params. The request doesn't define its own client and server capabilities. It is only issued if a server registers for the [`textDocument/prepareTypeHierarchy` request](#textDocument_prepareTypeHierarchy). | ||
|
||
_Request_: | ||
|
||
* method: 'typeHierarchy/subtypes' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not obvious why there are so many messages here, nor why the prepare and resolve steps are required. For clients (and presumably many servers), it would be simpler to return the whole tree in a single message. With this current API proposal there will be a lot of chatter for an ostensibly simple dataset. Could we perhaps incorporate the ability for the server to return the full type hierarchy in response to just a simple It's also unclear to me when a client should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just as you mentioned, calculating the full type hierarchy is extremely expensive for a given type with many subtypes. We currently have an implementation in Java language server, it may cost about 25 seconds to calculate the direct subtypes of For the second question, we have two common views for all languages, they are |
||
* params: `TypeHierarchySubtypesParams` defined as follows: | ||
|
||
```typescript | ||
export interface TypeHierarchySubtypesParams extends | ||
WorkDoneProgressParams, PartialResultParams { | ||
item: TypeHierarchyItem; | ||
} | ||
``` | ||
_Response_: | ||
|
||
* result: `TypeHierarchyItem[] | null` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, what's |
||
* partial result: `TypeHierarchyItem[]` | ||
* error: code and message set in case an exception happens during the 'typeHierarchy/subtypes' request | ||
|
||
##### Notes | ||
|
||
Server implementations of this method should ensure that the moniker calculation matches to those used in the corresponding LSIF implementation to ensure symbols can be associated correctly across IDE sessions and LSIF indexes. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced that we should add
inheritanceTreeSuppport
in the first version since I don't see clients to support this rendering. IMO it makes things simply harder to understand. Or do I miss something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologize for not mentioning the client behaviors about
inheritanceTreeSupport
in the protocol. I'd like to describe it in detail here:When using
inheritanceTreeSupport
, the client has the same mechanism of handling response data as the other two requests. The only difference is how to setclassOnly
inTypeHierarchySupertypesParams
.inheritanceTreeSupport
or it'sfalse
, theclassOnly
will always beundefined
since the server doesn't support inheritance tree. Besides, Inheritance Tree button will hide in the client.inheritanceTreeSupport
totrue
, the client could show Inheritance Tree button.typeHierarchy/supertypes
requests withclassOnly
in their params set totrue
. The responses include all classes in the inheritance tree of the given item, so that the client can render this tree reversally. The remaining requests work normally.classOnly
will always befalse
and everything works normally.typeHierarchy/supertypes
request with itsclassOnly
istrue
, the response should contain class only, otherwise it contains all types.If there is no
![type2](https://user-images.githubusercontent.com/45906942/118770745-466b6700-b8b4-11eb-8e20-fcad056611d6.jpg)
inheritanceTreeSupport
in the protocol, since we'd like to support inheritance tree view for it's the most important type hierarchy view for Java users (might also for single inheritance language users), we have to independently implement the view in java extension instead. We might make it as a command and useworkspace.executeCommand
to trigger this view. In server, we'll have the similar logics to handle the supertype requests which return only class. Another concern is that if we can put the Inheritance Tree button properly. See the following screenshot:Currently we could put them together since we contribute
ms-vscode.references-view
, if the right two buttons are supported in LSP and implemented in client already, can we still find a way to put them together?What do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was incorrect about what I said. I see the rendering but I am still not convinced for the modes in the API. Explaining this is simply too complicated. Why can a client not render that tree using the existing API by filtering interfaces. And what do we expect that happens if a client calls the API with
classOnly
for a item that represents a interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A client can render that tree by filtering interfaces indeed. It's a good tradeoff to remove
classOnly
and put all the logics in client implementation to keep the protocol clear. If we removeclassOnly
and keepinheritanceTreeSupport
in the first version, should we define the behaviors of client? If we removeinheritanceTreeSupport
from the first version, my concern is whether we can still keep the inheritance tree view (from java extension) and the other two views button (from VSCode) together.Currently we expect calling the API with
classOnly
for an interface will return an empty array since never a class can be the supertype of an interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not understanding why
inheritanceTreeSupport
can influence how a client can renderer a class hierarchy. Isn't this something that is under the sole control of the client and a server should never influence this?Another problem I see with
classOnly
is that it is specific for a certain use case. It might cause problems with other languages. I could for example imagine a special view for language that have multiple class inheritance. Another view could be special for interface to see the implemented protocols.If we think that that performance and payload size is a problem then we should add a filter property to the params which would allows to filter hierarchy items on the server side. IMO
classOnly
is simply to specific for a specific use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About inheritanceTreeSupport
After reading through above comments, I think current naming is causing confusion. Let me help to clarify according to my understanding:
ServerCapabilities.inheritanceTreeSupport
proposed by @CsCherrYY indicates whether "the language only supports single inheritance through class". E.g. it's true for Java because multiple inheritance is supported through interfaces. And it's false for C++. (@CsCherrYY correct me if I'm wrong)"how a client can renderer a class hierarchy" mentioned by @dbaeumer should be related to a Client capability if needed. For example
inheritanceTreeSupport
,inheritanceGraphSupport
(for multiple-inheritance languages?) etc. (@dbaeumer correct me if I'm wrong)About classOnly
Agree that
classOnly
is merely a specific filter property, is there any other filtering requirement (e.g. interfaceOnly), how would a generic filter property look like?I'm testing this proposal on Java language server, so far I don't see any performance/payload size problems as we expand the hierarchy level by level, and I'm fine to filter items on either server/client side.
Here I only have a stupid question as I know little about client implementation details: No matter what the filtering property eventually is, if I want to support showing class view (e.g. using tree items in vscode's reference view) for Java, how does client know when to filter the items or not? Does it provide multiple entries (e.g."show supertypes", "show subtypes", "show class view", "show interface only") and send requests with different params corresponding to the entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter properties would be properties that are available on a TypeHierarchyItem. I would say typically the kind property.
Regarding the UI: it could either be a different command or some sort of filter buttons in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's possible to keep the Class Hierarchy view available for the Java users, I'm very glad to remove
inheritanceTreeSupport
in the first version since it's indeed a little complicated. Here is another approach to support Class Hierarchy: The Java extension can contribute another button in reference-view and use a separated command to show class hierarchy. But there is still a concern about the UI implementation.We want to make the separated class hierarchy button align with the two existing type hierarchy buttons (supertype hierarchy and subtype hierarchy). In the current reference-view implementation, the call hierarchy uses context value to control whether to show the button. Since the call hierarchy has only two views, it works well. But for type hierarchy, can we make the context value extendable to support possible other views? Then we can use other conditions like
enablement
to show if this button is disabled, like the current Java implementation.@dbaeumer Could you give any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Another concern is about the default view. Currently the default type hierarchy view for java users is Class Hierarchy view, which is most frequently used by java developers. However, the implementation of type hierarchy in vscode-references-view would be based on the protocol, supporting subtype hierarchy view and supertype hierarchy view only. Could it be possible to change the default view of type hierarchy in another extension? e.g. Java extension.