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

[WIP] Proposal for type hierarchy #346

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
77 changes: 77 additions & 0 deletions protocol/protocol.typeHierarchy.proposed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@

#### Type Hierarchy
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this proposal is using the term "type" everywhere and not "symbol"? For example in TypeScript, those are two very different concepts

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 those are very different concepts on most languages.


Many language support inheritance and LSP provides retrieving the hierarchy information with the following requests.


The `textDocument/subTypes` request is sent from the client to the server to resolve resolve the subtypes of a symbol at a given text document position. Returns the direct subtypes in no particular order.

_Client Capabilities_:

```ts
TextDocumentClientCapabilities {
/**
* Capabilities specific to the `textDocument/subTypes`
* and `textDocument/superTypes
*/
typeHierarchy?: {
/**
* Whether implementation supports dynamic registration.
*/
dynamicRegistration?: boolean;
};
```

_Server Capabilities_:
```ts
ServerCapabilities{
/**
* The server provides type hierarchy information
*/
typeHierarchyProvider?: boolean
}
```

```ts
/**
* Represents hierarchical information about programming constructs like variables, classes,
* interfaces etc.
*/
export interface SymbolNode extends SymbolInformation {

Choose a reason for hiding this comment

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

As of LSP 3.10.0, we can use the DocumentSymbol as is, right? It has the children property.

Choose a reason for hiding this comment

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

We need one structural change, and we could simplify this PR:

The DocumentSymbol should be

export class DocumentSymbol {
	name: string;
	detail?: string;
	kind: SymbolKind;
	deprecated?: boolean;
	range: Range | Location;
	selectionRange: Range | Location;
	children?: DocumentSymbol[];
}

instead of the current state:

export class DocumentSymbol {
	name: string;
	detail?: string;
	kind: SymbolKind;
	deprecated?: boolean;
	range: Range;
	selectionRange: Range;
	children?: DocumentSymbol[];
}

When the range and the selectionRange has the type Range then the DocumentUri is inferred. For example, when calling textDocument/documentSymbol, the DocumentUri can be inferred as DocumentSymbolParams.textDocument.uri from the param. If the range and the selectionRange are Location types, then the DocumentUri is explicitly defined (Location.uri).

If we accept this structural change in the DocumentSymbol, we can defined our two new methods as textDocument/subTypes and textDocument/superTypes with DocumentSymbol[] return type.

We can even stick to the children name for the textDocument/superTypes; the supertypes will be real children of the DocumentSymbol node, only the way of the traversal differs.

CC: @tsmaeder (since you have taken over the PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 See related #420 (comment).
Btw. the documentation for DocumentSymbol#selectionRange says

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`.

So with this contract it is enough if the range can alternatively be a Location.

Copy link
Contributor

@svenefftinge svenefftinge Oct 8, 2018

Choose a reason for hiding this comment

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

Actually, I would vote for adding an optional uri: URI property to DocumentSymbol, because the property name range suggests that the datatype is a Range, too. Also, it will not break existing clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* Immediate descendants of this node
*/
descendants? : SymbolNode[]
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's the immediate descendants, it should be called children

Copy link
Author

Choose a reason for hiding this comment

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

The doc is incorrect needs to be updated.

I am not sure about children either. This can actually hold hierarchies with depth larger than one. Probably needs a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it's depth larger than one, wouldn't the elements of this particular array still be the immediate children?

Copy link
Author

Choose a reason for hiding this comment

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

We use this same data structure for both subTypes and superTypes calls on the superTypes call they are actually the ancestors/parents. We probably need a new name to reflect that.

Copy link
Contributor

@svenefftinge svenefftinge May 9, 2018

Choose a reason for hiding this comment

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

Talking about children would be natural for me, as the symbol nodes will typically be presented in tree-like structures in the client. Also, it matches with hasChildrenbelow.

Copy link

Choose a reason for hiding this comment

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

It may sometimes be useful to expand multiple levels in one request, This is how ccls and cquery do.

/**
* true if this node has children. hasChildren can be
* true even when the descendants are empty. In that case
* the clients should do a call to retrieve the descendants.
*/
hasChildren: boolean
}
```

_Request_:

method: ‘textDocument/subTypes’
params: TextDocumentPositionParams

_Response_

* result: `SymbolNode[] | null`
* error: code and message set in case an exception happens during the 'textDocument/subTypes' request
Copy link
Contributor

Choose a reason for hiding this comment

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

When I open a type hierarchy on a certain position in the text, I would like to see the SymbolNode for that token together with its subtypes. How do I obtain the SymbolNode I am opening the hierarchy on?

Copy link

@MaskRay MaskRay Jun 8, 2018

Choose a reason for hiding this comment

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

There can be multiple dimensions of sub-super relations: template parameters related to a template, derived types related to a base, specializations. Is there some way to differentiate them in this proposal?

In ccls, both derived types and specializations are stored in the IndexType::derived field.




The `textDocument/superTypes` request is sent from the client to the server to resolve resolve the supertypes of a symbol at a given text document position. Returns all the supertypes in bottom-up order.

_Request_:

method: ‘textDocument/superTypes’
params: TextDocumentPositionParams

_Response_

* result: `SymbolNode[] | null`
Copy link
Contributor

Choose a reason for hiding this comment

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

So a type can have multiple parent types? If that is true, how would it be displayed in a tree UI (vs a graph)?

Copy link
Author

Choose a reason for hiding this comment

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

multiple-inheritance has been around and there have been tree based representations of it implemented before. The most generic approach I have seen is to replicate the same Type hierarchy tree under all parent nodes. We can add additional information to this data structure if it is going to help with its representation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should reclarify the goal of this proposal, I misunderstood it. The linked issue describes a symbol hierarchy (e.g. namespaces -> classes -> members), not inheritance

Copy link
Author

Choose a reason for hiding this comment

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

Also take a look at microsoft/language-server-protocol#472 I think that should include the symbol hirearchy

* error: code and message set in case an exception happens during the 'textDocument/superTypes' request

65 changes: 65 additions & 0 deletions protocol/src/protocol.typeHierarchy.proposed.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* --------------------------------------------------------------------------------------------
* Copyright (c) Red Hat Inc and others . All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
* ------------------------------------------------------------------------------------------ */
'use strict';

import { RequestType } from 'vscode-jsonrpc';
import {SymbolInformation} from 'vscode-languageserver-types';
import {TextDocumentPositionParams, TextDocumentRegistrationOptions} from './protocol'

export interface TypeHierarchyClientCapabilities {
/**
* The text document client capabilities
*/
textDocument?: {
/**
* Capabilities specific to the `textDocument/subTypes` and `textDocument/superTypes`
*/
typeHierarchy?: {
/**
* Whether implementation supports dynamic registration.
*/
dynamicRegistration?: boolean;
};
}
}

export interface TypeHierarchyServerCapabilities {
/**
* The server provides type hierarchy information
*/
typeHierarchyProvider?: boolean
}

/**
*
*/
export namespace SubTypesRequest {
export const type = new RequestType<TextDocumentPositionParams, SymbolNode[], void, TextDocumentRegistrationOptions>('textDocument/subTypes');
}

/**
*
*/
export namespace SuperTypesRequest {
export const type = new RequestType<TextDocumentPositionParams, SymbolNode[], void, TextDocumentRegistrationOptions>('textDocument/superTypes');
}


/**
* Represents hierarchical information about programming constructs like variables, classes,
* interfaces etc.
*/
export interface SymbolNode extends SymbolInformation {
/**
* Immediate descendants of this node
*/
descendants? : SymbolNode[]
/**
* true if this node has children. hasChildren can be
* true even when the descendants are empty. In that case
* the clients should do a call to retrieve the descendants.
*/
hasChildren: boolean
}