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

Proposal of the type hierarchy protocol extension. #426

Closed
wants to merge 1 commit into from

Conversation

kittaakos
Copy link

@kittaakos kittaakos commented Oct 15, 2018

Note:
Another related PR in this repository: #346
The original GH issue for the PR: microsoft/language-server-protocol#136


Type hierarchy in action with Eclipse Theia and the JDT language server.

46807217-b4993600-cd69-11e8-8fd6-cdd5771004f2

Signed-off-by: Akos Kitta [email protected]

CC: @svenefftinge

@kittaakos
Copy link
Author

I have updated the proposal based on #346 (comment).

/**
* `true` if the language client supports super- and subtype hierarchies. Defaults to `false`.
*/
typeHierarchy?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Actually the pattern here is to not provide textDocument.typeHierarchy property if the client doesn't support it. The capability should flag if dynamic registration is supported.

* Server capability for calculating super- and subtype hierarchies.
* The language server supports the type hierarchy language feature, if this capability is set to `true`.
*/
typeHierarchy?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a new request we should support to register that for a specific document selector as well. See https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/lib/protocol.implementation.d.ts#L26 how this is supported in the LSP. Servers are only allowed to use this registration option if the client signals dynamic registration.

* The direction of the hierarchy levels to resolve. Valid values are: `parents`, `children`, or `both`.
* Defaults to `children`.
*/
direction?: 'parents' | 'children' | 'both';
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to clarify what should happen if a hierarchy item has already the parents resolved and is asked to resolve its children. Should the client send the parents again or is it allowed to leave them out. I would opt for the second.

Copy link
Member

Choose a reason for hiding this comment

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

I do like strings literals over numbers but so far we used numbers for all enum like types. To keep things consistent I would like to opt to say with this unless we have a strong reason to do otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I know I proposed this earlier but now I am more for consistency :-)

Copy link
Author

Choose a reason for hiding this comment

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

I would opt for the second.

+1

@dbaeumer, I would have a follow-up question on this: what should happen in general, if something is already resolved and the client requests to resolve it again?

In the JDT reference implementation, I did for this language feature, I blindly resolve everything from scratch, because:

  • the text document that is referenced by the TypeHierarchyItem might have changed meanwhile, so the item I am trying to resolve is not at the same cursor position anymore,
  • the type hierarchy of my TypeHierarchyItem might have changed.

Thank you!

* The direction of the hierarchy levels to resolve. Valid values are: `parents`, `children`, or `both`.
* Defaults to `children`.
*/
direction?: 'parents' | 'children' | 'both';
Copy link
Member

Choose a reason for hiding this comment

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

I know I proposed this earlier but now I am more for consistency :-)

@kittaakos kittaakos force-pushed the type-hierarchy branch 2 times, most recently from dcfa9d3 to 3c9550a Compare January 25, 2019 13:24
@kittaakos
Copy link
Author

I have updated this PR based on the review feedback.

kittaakos pushed a commit to kittaakos/lsp4j that referenced this pull request Jan 28, 2019
/**
* The direction of the hierarchy levels to resolve.
*/
direction?: TypeHierarchyDirection

Choose a reason for hiding this comment

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

Does this have a default? What should happen if the client provides a value for resolve but not for direction?

AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Feb 1, 2019

_Requests_:

The `Type Hierarchy Request` is sent from the client to the server to retrieve a `TypeHierarchyItem` at a given cursor position. If computing full type hierarchy items is expensive, servers can additionally provide a handler for the type hierarchy item resolve request (`typeHierarchy/resolve`). This request would also allow to specify if the item should be resolved and whether sub- or supertypes are to be resolved. If no item can be retrieved for a given text document position, returns with `null`.

Choose a reason for hiding this comment

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

This seems pretty complicated - is there evidence that a) lazy resolving is needed and b) eager resolving is also needed?

/**
* The hierarchy levels to resolve. `0` indicates no level. When not defined, it is treated as `0`.
*/
resolve?: number;

Choose a reason for hiding this comment

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

can we specify that the server may resolve more than this number of levels, e.g. if it's cheap to do so?

Client doesn't know whether it's cheap or expensive, so may specify low resolve (e.g. 1). However for some servers (in particular for clangd), resolving parent links one-at-a-time in a loop is much more expensive than doing it all at once - the cost is in finding the first parent, the rest are free.

AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 6, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 6, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 6, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 6, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 7, 2019
AlexTugarev added a commit to typescript-language-server/typescript-language-server that referenced this pull request Mar 8, 2019
llvm-git-migration pushed a commit to llvm/llvm-project that referenced this pull request Mar 19, 2019
Summary:
Patch by Nathan Ridge(@nridge)!

This is an LSP extension proposed here:
microsoft/vscode-languageserver-node#426

An example client implementation can be found here:
eclipse-theia/theia#3802

Reviewers: kadircet, sammccall

Reviewed By: kadircet

Subscribers: jdoerfert, sammccall, cfe-commits, mgorny, dschaefer, simark, ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet

Tags: #clang

Differential Revision: https://reviews.llvm.org/D56370

llvm-svn: 356445
dtzWill pushed a commit to llvm-mirror/clang-tools-extra that referenced this pull request Mar 19, 2019
Summary:
Patch by Nathan Ridge(@nridge)!

This is an LSP extension proposed here:
microsoft/vscode-languageserver-node#426

An example client implementation can be found here:
eclipse-theia/theia#3802

Reviewers: kadircet, sammccall

Reviewed By: kadircet

Subscribers: jdoerfert, sammccall, cfe-commits, mgorny, dschaefer, simark, ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet

Tags: #clang

Differential Revision: https://reviews.llvm.org/D56370

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@356445 91177308-0d34-0410-b5e6-96231b3b80d8
arichardson pushed a commit to arichardson/llvm-project that referenced this pull request Apr 8, 2019
Summary:
Patch by Nathan Ridge(@nridge)!

This is an LSP extension proposed here:
microsoft/vscode-languageserver-node#426

An example client implementation can be found here:
eclipse-theia/theia#3802

Reviewers: kadircet, sammccall

Reviewed By: kadircet

Subscribers: jdoerfert, sammccall, cfe-commits, mgorny, dschaefer, simark, ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet

Tags: #clang

Differential Revision: https://reviews.llvm.org/D56370

llvm-svn: 356445
@HighCommander4
Copy link

Note that there is now a server implementation of this in clangd (C++ language server), using the protocol proposed here.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 19, 2019
9.0.0:
Improvements to clangd
* Background indexing is on by default
When using clangd, it will build an index of your code base (all files listed in your compile database). This index enables go-to-definition, find-references, and even code completion to find symbols across your project.
This feature can consume a lot of CPU. It can be disabled using the --background-index=false flag, and respects -j to use fewer threads. The index is written to .clangd/index in the project root.

* Contextual code actions
Extract variable, expand auto, expand macro, convert string to raw string. More to come in the future!

* Clang-tidy warnings are available
These will be produced for projects that have a .clang-tidy file in their source tree, as described in the clang-tidy documentation.

* Improved diagnostics
Errors from headers are now shown (on the #including line). The message now indicates if fixes are available. Navigation between errors and associated notes is improved (for editors that support Diagnostic.relatedInformation).

* Suggested includes
When a class or other name is not found, clangd may suggest to fix this by adding the corresponding #include directive.

* Semantic highlighting
clangd can push syntax information to the editor, allowing it to highlight e.g. member variables differently from locals. (requires editor support)
This implements the proposed protocol from microsoft/vscode-languageserver-node#367

* Type hierachy
Navigation to base/derived types is possible in editors that support the proposed protocol from microsoft/vscode-languageserver-node#426

* Improvements to include insertion
Only headers with #include-guards will be inserted, and the feature can be disabled with the --header-insertion=never flag.
Standard library headers should now be inserted more accurately, particularly for C++ other than libstdc++, and for the C standard library.

* Code completion
Overloads are bundled into a single completion item by default. (for editors that support signature-help).
Redundant const/non-const overloads are no longer shown.
Before clangd is warmed up (during preamble build), limited identifier- and index-based code completion is available.

* Format-on-type
A new implementation of format-on-type is triggered by hitting enter: it attempts to reformat the previous line and reindent the new line. (Requires editor support).

* Toolchain header detection
Projects that use an embedded gcc toolchain may only work when used with the corresponding standard library. clangd can now query the toolchain to find these headers. The compilation database must correctly specify this toolchain, and the --query-driver=/path/to/toolchain/bin/* flag must be passed to clangd.

* Miscellaneous improvements
Hover now produces richer Markdown-formatted text (for supported editors).
Rename is safer and more helpful, though is still within one file only.
Files without extensions (e.g. C++ standard library) are handled better.
clangd can understand offsets in UTF-8 or UTF-32 through command-line flags or protocol extensions. (Useful with editors/platforms that don’t speak UTF-16).
Editors that support edits near the cursor in code-completion can set the textDocument.completion.editsNearCursor capability to true, and clangd will provide completions that correct . to ->, and vice-versa.

Improvements to clang-tidy

New OpenMP module.
New abseil-duration-addition check.
New abseil-duration-conversion-cast check.
New abseil-duration-unnecessary-conversion check.
New abseil-time-comparison check.
New abseil-time-subtraction check.
New android-cloexec-pipe check.
New android-cloexec-pipe2 check.
New bugprone-branch-clone check.
New bugprone-posix-return check.
New bugprone-unhandled-self-assignment check.
New fuchsia-default-arguments-calls check.
New fuchsia-default-arguments-declarations check.
New google-objc-avoid-nsobject-new check.
New google-readability-avoid-underscore-in-googletest-name check.
New llvm-prefer-isa-or-dyn-cast-in-conditionals check.
New modernize-use-trailing-return-type check.
New objc-super-self check.
New openmp-exception-escape check.
New openmp-use-default-none check.
New readability-convert-member-functions-to-static check.
New alias cert-oop54-cpp to bugprone-unhandled-self-assignment was added.
New alias cppcoreguidelines-explicit-virtual-functions to modernize-use-override was added.
Added UseAssignment option to cppcoreguidelines-pro-type-member-init
The fuchsia-default-arguments check has been removed.
The google-runtime-int check has been disabled in Objective-C++.
The modernize-use-override now supports OverrideSpelling and FinalSpelling options.
The misc-throw-by-value-catch-by-reference now supports WarnOnLargeObject and MaxSize options to warn on any large trivial object caught by value.
The Acronyms and IncludeDefaultAcronyms options for the objc-property-declaration check have been removed.

Improvements to pp-trace
* Added a new option -callbacks to filter preprocessor callbacks. It replaces the -ignore option.
@yudar1024
Copy link

@HighCommander4 I wanna know weather this spec is finalized? wait this feature long time and an implement pr merge 823 depends on this spec

@HighCommander4
Copy link

@yudar1024 I'm not the spec's author, nor a maintainer of this repo. I just implemented the spec in the clangd language server.

My understanding is the spec continues to be subject to change until it merges. However, clients and servers may choose to implement it eagerly (as we did in clangd), with the understanding that if the spec changes the implementation will need to be updated as well.

shrouxm pushed a commit to sourcegraph/lsif-clang that referenced this pull request Jul 12, 2020
Summary:
Patch by Nathan Ridge(@nridge)!

This is an LSP extension proposed here:
microsoft/vscode-languageserver-node#426

An example client implementation can be found here:
eclipse-theia/theia#3802

Reviewers: kadircet, sammccall

Reviewed By: kadircet

Subscribers: jdoerfert, sammccall, cfe-commits, mgorny, dschaefer, simark, ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet

Tags: #clang

Differential Revision: https://reviews.llvm.org/D56370

llvm-svn: 356445
@kittaakos kittaakos closed this Sep 11, 2020
@HighCommander4
Copy link

@kittaakos What should I make of this PR being closed? It does not appear to have been merged. Was there a decision to reject it somewhere?

@Trias
Copy link

Trias commented Nov 24, 2020

I would like you to consider making range optional in the request. This could be used for displaying document level type requests (i. e inheritance chain of the current class).

I would also like to know the status of this proposal, as it seemed to be rather popular.

@dbaeumer
Copy link
Member

@Trias there is currently no active work happening to support type hierarchies in LSP.

@HighCommander4
Copy link

@Trias there is currently no active work happening to support type hierarchies in LSP.

To clarify: is that just because there's no one driving it forward at the moment? Or is it because the LSP maintainers feel there is no value in this feature?

That is, if someone were to dust off this PR, rebase it to be current, and address any outstanding feedback -- would you / other maintainers continue to entertain it?

@dbaeumer
Copy link
Member

Such a request makes sense in LSP. But please keep in mind that beside having a spec part we need an implementation in a client library and client as well.

@Trias
Copy link

Trias commented Nov 25, 2020

client meaning vscode? There apparently is already an existing implementation as shown in the first post but in the theia IDE.

@HighCommander4
Copy link

We also have a vscode implementation in vscode-clangd.

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

Successfully merging this pull request may close these issues.

6 participants