-
Notifications
You must be signed in to change notification settings - Fork 146
Export client from activation as API for use in other extensions #575
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
Conversation
|
Added some reviewers for feedback. This has been proposed before in #501 with some work in progress towards it, but the approach discussed there was to expose the AST request specifically, rather than exposing the entire language client. |
Thanks, I hadn't seen that
I see, the benefit of exposing the entire client is that all functionality is available through a known VS Code type (BaseLanguageClient) and there is less need to maintain a hand-crafted interface. It also means additions to the LSP in future should be available through the client automatically. There are 2 potential areas of change for this PR, but I wanted to keep it simple initially.
If exposing the client (rather than a custom AST API), I would suggest the API includes the RequestTypes as well as the shape of the API (e.g. |
|
I've updated this PR to add a versioned API which can be extended (the languageClient no longer stomps the root export). I would appreciate some feedback on getting this merged @HighCommander4 @sam-mccall @kadircet @hokein |
I'm broadly supportive of making vscode-clangd make its functionality available to other vscode extensions. I do think this is a significant enough decision for the project that I'm not comfortable making it on my own, and would like one of the other mentioned folks to weigh in as well. |
|
I think a link to a real-world example usage of the proposed API (perhaps in a branch of another repo) would help reason about it. |
Our usage of this is not OSS to link to, but essentially exposing this access means this AST can be leveraged and enhanced functionality can be offered by another vs code extension without having to load clangd twice. Simply put, if a user needs access to a C/C++ AST for adding better IDE experience, this PR offers a simple approach to doing this. Our use cases are primarily around accessing the AST, but it is cleaner, easier and more extensible to expose the language client in case there are other needs in future. AST use cases include:
|
My question was more geared towards understanding what the code consuming the API will look like. I do see that there is a code example in the PR description -- is that what consuming code would actually look like? For example, it would have to provide its own definition of
Not sure I'm following these particular examples -- by "instructions", do you mean assembly instructions? (Would that come up in the context of editing inline assembly code in a C/C++ code file then?) (This question is mostly out of curiosity. I don't doubt that there are interesting things an extension can do with a C/C++ AST.) |
In this simplistic form, yes. But it is also common practice to extract this API as typescript types and publish them on GitHub releases or npm. I'm happy to do that too and can look into it if this PR gets merged.
e.g. 1 A customised clangd is created from a downstream LLVM which includes extra instructions/macros Another interesting use case would be to expose AST data for consumption by AI engines for code helper inference etc. This PR has been open for >3 Months, is there any further changes required in order to increase interest in merging it @sam-mccall @kadircet @hokein |
Is it possible to have those type definitions live in this repo (I think that doesn't preclude them being a distinct npm package), and have them added as part of this PR?
In a situation like this, where you're working with a downstream LLVM (which includes clangd in-tree), why not make these hover enhancements on the server side in your downstream clangd? |
I've extracted these into a package which can be published to npm/GitHub
Because:
|
|
Can I have an update on this, please? |
|
In the interest of trying to move this forward, I'm going to add myself as a reviewer, and if we don't hear a strong objection from @sam-mccall, @kadircet, or @hokein, I will take the liberty of approving this. I want to play around with the patch a bit, including writing a (toy) example plugin that consumes the API to get a feel for what using the API looks like. I have a few other patches in my review queue ahead of this, so it may take me a couple of weeks to get around to that. Your patience is appreciated. |
Thanks! |
|
@thegecko I checked out this branch in order to play around with it; however, it's not building for me. After running Apologies if I'm missing something obvious, I'm mainly a C++ dev and I get lost easily in Typescript land. |
|
Thanks @HighCommander4 I added a const to the types at the last minute which isn't supported! That would have failed straight away if the CI had run, but is now fixed. I can look at adding the extension instantiation (along with the AST Type const) into the api package, but would prefer that to be a follow up PR. |
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.
Ok, I adapted the code from the README to turn it into a complete example plugin at https://github.com/HighCommander4/vscode-clangd-api-example, and I've tried it out and it seems to be working well (it successfully adds the AST node kind to the hover contents provided by vscode-clangd itself).
I think this generally looks great. I have a few minor comments below, after which I think this should be good to go!
api/README.md
Outdated
|
|
||
| const ASTType = 'textDocument/ast'; | ||
|
|
||
| const provideHover = (document: vscode.TextDocument, position: vscode.Position, _token: vscode.CancellationToken): Promise<vscode.Hover | undefined> => { |
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.
nit: this needs async before the parameter list to actually compile
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.
Added
| @@ -0,0 +1,13 @@ | |||
| { | |||
| "name": "@clangd/vscode-clangd", | |||
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.
Please add a "license" field (MIT, like the main plugin)
Also, maybe a "homepage" link to a hosted version of the README (something like https://github.com/clangd/vscode-clangd/blob/master/api/README.md) would make sense?
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.
Both added
src/ast.ts
Outdated
| import {ClangdContext} from './clangd-context'; | ||
| import type {ASTParams, ASTNode} from '../api/vscode-clangd'; | ||
|
|
||
| const ASTType = 'textDocument/ast'; |
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.
nit: Type seems like a strange word to describe this string.
The name of the parameter of the RequestType constructor that this gets passed to is method (which in turn comes from the terminology that JSON-RPC uses).
ASTRequestMethod perhaps?
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.
Switched to ASTRequestMethod in the code and example
api/vscode-clangd.d.ts
Outdated
| import {BaseLanguageClient} from 'vscode-languageclient'; | ||
| import * as vscodelc from 'vscode-languageclient/node'; | ||
|
|
||
| // The wire format: we send a position, and get back a tree of ASTNode. |
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.
Please add some comments to explain that:
- The V1 API is exposing vscode-clangd's language client, which can be used to send a variety of requests, some of which are standard and some of which are clangd extensions
- The standard requests are documented at https://microsoft.github.io/language-server-protocol/specifications/specification-current
- The clangd extensions are documented at https://clangd.llvm.org/extensions
- This file currently contains type declarations for one of those extensions,
textDocument/ast. Type declarations for other extensions may be added later. - (It's fine to not expose the string
"textDocument/ast"as a constant for now, but please mention it in a comment.)
- This file currently contains type declarations for one of those extensions,
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.
Comments added, also reordered the types to make more sense
| const clangdExtension = vscode.extensions.getExtension<ClangdExtension>(CLANGD_EXTENSION); | ||
|
|
||
| if (clangdExtension) { | ||
| const api = (await clangdExtension.activate()).getApi(CLANGD_API_VERSION); |
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.
We should probably discourage consumers unconditionally calling activate(), since the clangd extension may already be activated and it doesn't expect activate() to be called again in that state.
I believe the right pattern here is for consuming code to:
- use
clangdExtension.exportsto access theClangdExtensionobject - if it's unsure whether the clangd extension is active yet at that point, then guard the access to
.exportswith aclangdExtension.isActivecheck- if the check fails, either bail or call
activate()at that point
- if the check fails, either bail or call
This is what I've done in my example plugin, and I'd prefer the README does the same. (Sorry to be picky on this point, I worry that users who may not have much familiarity with VSCode extensions may look to this README for how to use the API and assume that what it shows is a good way to do things.)
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.
vscode.Extension.activate() is designed to be called multiple times and only calls into the extension's activate() method once. Subsequent calls return the resolved extension. This is a common pattern and (just to be sure) I tested this scenario by activating it multiple times with logging in the extension activate() function. It worked as expected and only activates once.
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.
Thanks for checking on this, I think this is fine then.
| contents: [ast.kind] | ||
| }; | ||
| } | ||
| }; |
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.
can we show the registerHoverProvider() call in the README, just to make it a more complete example?
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.
Added
|
Thanks for the feedback @HighCommander4 , comments above. Also ran the format command which should (hopefully) fix the CI. |
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.
Thanks! The updates look good.
Can you get the clang-format CI check to pass please? Then this should be good to merge.
| const clangdExtension = vscode.extensions.getExtension<ClangdExtension>(CLANGD_EXTENSION); | ||
|
|
||
| if (clangdExtension) { | ||
| const api = (await clangdExtension.activate()).getApi(CLANGD_API_VERSION); |
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.
Thanks for checking on this, I think this is fine then.
Took me a while to realise the error output is a diff and it wasn't happy with line length. Should now be fixed. |
|
Merged. Thanks for your work on this! If anyone writes publically available plugins that make use of this API, please feel free to mention them in a comment for visibility. At some point we could list such plugins (if they are of general interest) on vscode-clangd's Marketplace page. |
|
I’m very glad that this PR was accepted - now, instead of supporting my own fork, I just moved everything I needed into separate extensions: |
Very neat! This one still requires a forked server, right? |
right |
|
Sent out #653 so the API appears in a vscode-clangd release |
This PR exports the language client so other VS Code extensions can take advantage of the clangd LSP features.
For example, domain specific functionality (e.g. intrinsics) could be implemented in a separate extension and trivially leverage the full-featured AST functionality exposed by clangd.
This has been tested and working with a trivial hover example: