Skip to content

Support Type Hierarchy #1231

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

Closed
wants to merge 8 commits into from
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 176 additions & 0 deletions _specifications/specification-3-17.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
```

Expand Down Expand Up @@ -2419,6 +2426,14 @@ interface ServerCapabilities {
}
}

/**
* The server provides type hierarchy support.
*
* @since 3.17.0
*/
typeHierarchyProvider?: boolean | TypeHierarchyOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a simple boolean necessary/sufficient here since each server capability can be defined under TypeHierarchyOptions?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, we don't need boolean here in this version. We are considering whether to remove the capability inheritanceTreeSuppport to keep the type hierarchy general. I'll update this PR then.

Copy link
Member

Choose a reason for hiding this comment

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

I find it was referring call hierarchy.

callHierarchyProvider?: boolean | CallHierarchyOptions | CallHierarchyRegistrationOptions;

which is equivalent to:

boolean // simplest 
OR
{
  workDoneProgress?: boolean;  // from WorkDoneProgressOptions
} 
OR
{
  workDoneProgress?: boolean;  // from WorkDoneProgressOptions
  id?: string;  // from StaticRegistrationOptions 
  documentSelector: DocumentSelector | null;  // from TextDocumentRegistrationOptions 
}

specifying whether server has capabitility to support work done progress / unregistration / customized scope of the feature.

Is a simple boolean necessary/sufficient here since each server capability can be defined under TypeHierarchyOptions?

@KamasamaK I think boolean is insufficient for advanced features mentioned above. For me, it's somehow necessary . Because on one hand, when I implement it in server side I might not care about any advanced feature, then I can simply return a true; on the other hand it's consistent with other capabilities like call hierarchy.

We are considering whether to remove the capability inheritanceTreeSuppport to keep the type hierarchy general.

@CsCherrYY I'm voting +1 for removing inheritanceTreeSuppport, otherwise for languages supporting multiple inheritance (e.g. C++), you may also need something like inheritanceGraphSuppport?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't care about implementing or can't implement advanced features, you can return false for their capabilities. For example, codeLensProvider only has a type of CodeLensOptions with one optional native capability, so that's already established for LSP. That is the reason I say it is probably unnecessary. But if the extra capability is going to be removed, that might change things.

Perhaps @dbaeumer can weigh in on the necessity of a simple boolean for new operations when *Options exists, and whether them having no or only optional native properties makes a difference in that determination.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for pointing out CodeLensOptions. It looks:
{ resolveProvider: false } means server doesn't have a resolve provider.
{ resolveProvider: true } means server also support to resolve code lens.
So now I have a question, if server doesn't support code lens at all, what should it return? (if boolean is not allowed)

Copy link
Contributor

Choose a reason for hiding this comment

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

Excluding codeLensProvider would indicate that the server does not support it.

A missing property should be interpreted as an absence of the capability. If a missing property normally defines sub properties, all missing sub properties should be interpreted as an absence of the corresponding capability.

| TypeHierarchyRegistrationOptions;

/**
* Experimental server capabilities.
*/
Expand Down Expand Up @@ -8173,6 +8188,167 @@ 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.

In the first step, the `textDocument/prepareTypeHierarchy` request could have a unique, constant and optional `transactionId`. The following `typeHierarchy/supertypes` and `typeHierarchy/subtypes` requests in the second step could have the same `transactionId` in their params, which could be used to help indicate some cached data in the server.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of any existing LSP API that uses this "prepare to get ID then pass ID back" method; this seems pretty specific to a particular LS implementation. The closest I'm aware of is completion (where it's arbitrary data), but that's because the user may not even visit a particular completion (so you want to defer that work to later in hopes it's never done). This new API appears to always call prepare, then one of the two other calls.

Why not just have the two calls and cache the information internally (say, per snapshot or similar?). This would match other similarly-expensive calls like go-to-references or go-to-implementations which are simple requests. If we were to add this to Pylance, we wouldn't need this transaction ID, as the info is there in the analysis (or will be lazily produced from the current state)

I can also see this pattern being problematic for LSIF (where you expect basically pure functions for read-only requests), but I guess the ID is optional.

Copy link
Member

Choose a reason for hiding this comment

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

this "prepare to get ID then pass ID back" method

As far as I understand of this proposal, client is reponsible to generate this ID itself, and passes it to server in request textDocument/prepareTypeHierarchy. I assume the purpose of this "prepare" request is to get corresponding TypeHierarchyItem of the cursor position where you send the request, instead of getting ID.

As mentioned above in https://github.com/microsoft/language-server-protocol/pull/1231/files#r612267620 (though not explicitly described in spec) if it's not a valid type (e.g. triggered on some keywords), response of "prepare" request can be null, and client probably should not send further requests then.

Copy link
Member

Choose a reason for hiding this comment

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

I can sort of see the argument of needing to re-query at various points in the same hierarchy, but I can see troubles where because the client is defining this ID, it doesn't have enough information to determine when the result has actually changed (e.g., the server has changed the analysis and now this request cycle is invalid; like on an external file watcher change), and therefore requires something like how semantic tokenization has notifications for "the data has changed".

One thing that I think is similar to this right now is signature help retriggering; as the user moves the cursor, the previous signature info is passed back to the server, which can then use that to further edit the response, or, choose to ignore it and give the data fresh.

I guess I'll need to reread this with fresh eyes in the morning.

Copy link
Member

Choose a reason for hiding this comment

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

Here is an alternative approach: the prepare request returns a TypeHierarchyItem so the data field of the item can be used to identify the type hierarchy. If the type hierarchy changes I would suggest that the server errors this on the next sub or super type request and lets the client decided what to do (e.g. refresh the hierarchy, close the hierarchy, ...)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I find that to be a bit more understandable and close to completion resolution / signature help callbacks (assuming this new data field is something returned by the server at the prepare call, then passed back by the client, versus how this spec currently has the client come up with an ID).

Copy link
Author

@CsCherrYY CsCherrYY Apr 29, 2021

Choose a reason for hiding this comment

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

Here is an alternative approach: the prepare request returns a TypeHierarchyItem so the data field of the item can be used to identify the type hierarchy.

So under this approach, in my understanding, the server will be responsible for managing the "transaction" itself. It can save something to identify it in data field at the response of prepare call, as @jakebailey said. The client always keep it unchanged in the TypeHierarchyItem and send it back with the next sub or super type requests, is that right?

And if we apply this and remove transactionId in params, should we add some description explicitly to show that the data field could be used to carry the type hierarchy related information in the server? I think it may help the designers of language servers if the server can use this mechanism to improve the performance.

Copy link
Member

Choose a reason for hiding this comment

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

@CsCherrYY yes, that is the idea and consistent with other situations. For example the completion item has a data field as well which the clients needs to preserve and sent back on a resolve call so that the server can related the completion item to some previous request.

A comment in the data field that describes the use case of a transaction ID is for sure something we should do.


_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: `boolean | TypeHierarchyOptions | TypeHierarchyRegistrationOptions` where `TypeHierarchyOptions` is defined as follows:

```typescript
export interface TypeHierarchyOptions extends WorkDoneProgressOptions {
}
```

_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 {
transactionId?: integer | string;
}
```

_Response_:

* result: `TypeHierarchyItem[] | null` defined as follows:

Choose a reason for hiding this comment

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

Why should we allow null here ? What's the client expected to do with a null? It's better to define this to either return an empty array, or an error. Alternatively explicitly define what the meaning of null is (as opposed to an empty array or an error return).

Copy link
Author

Choose a reason for hiding this comment

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

In the textDocument/prepareTypeHierarchy request, if the element in the location of the params is not a valid type (that depends on the server implementation, whether to infer a valid type), it will return null to indicate that there is no result. For those types have no subtypes in typeHierarchy/subtypes, the server will return an empty array.

Thanks for the comment, I'll add a description in the next commit.

Copy link
Member

Choose a reason for hiding this comment

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

null is for consistency since lots of LSP requests that return and array can return null to indicate no elements


```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.
*/
data?: unknown;
Copy link
Member

Choose a reason for hiding this comment

The 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).

Choose a reason for hiding this comment

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

FWIW, in clangd, what we put in the data field is a hash of the internal universal identifier of the symbol represented by the TypeHierarchyItem, so it being per-item makes sense.

Copy link
Author

Choose a reason for hiding this comment

The 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 data field to indicate the specific type, we'd better to keep a data field in each item. In protocol, I suggest keeping this field in each item to support these two usages both.

}
```

* 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 {
item: TypeHierarchyItem;
transactionId?: integer | string;
}
```
_Response_:

* result: `TypeHierarchyItem[] | null`

Choose a reason for hiding this comment

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

what does null return mean?

* 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'

Choose a reason for hiding this comment

The 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 /typeHierarchy message as the first instance? Do we know that this is extremely expensive to calculate in many servers (enough to justify the complexity of this back-and-forth prepare/request/request/request API?

It's also unclear to me when a client should use subtypes and super types requests vs the inheritanceTree request - unless I'm missing something, they are implementing the same thing in a different way? What's the rationale for this?

Copy link
Author

Choose a reason for hiding this comment

The 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 java.io.Serializable (1877 subtypes in the example project in total). We want the user to know what server is doing - after prepare request, the client can get the base type itself so that it can be shown. So the user can see the based type with some progress icons at least, with partial result mechanism (if implemented), the client can show the results gradually.

For the second question, we have two common views for all languages, they are subtypes and supertypes. For single inheritance languages, since a given class could only have one superclass, usually they have another view to show all the classes in the inheritance tree. That can be performed in many supertypes requests and a subtypes request as well, so we're still in discussing whether to isolate this request.

* params: `TypeHierarchySubtypesParams` defined as follows:

```typescript
export interface TypeHierarchySubtypesParams extends
WorkDoneProgressParams, PartialResultParams {
item: TypeHierarchyItem;
transactionId?: integer | string;
}
```
_Response_:

* result: `TypeHierarchyItem[] | null`

Choose a reason for hiding this comment

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

again, what's null for? we need to be explicit about what the valid responses are and what they mean, so that client and servers work together in harmony.

* 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.
Expand Down