Enable clangd on HLSL sources #392
Conversation
Clang is gaining support for HLSL. This change enables using clangd for the language server for vscode. I'm unsure if there is a good way to gate this support on a specific version of clang, but when used with clang-15 it produces an error at the beginning of the file noting that compilation failed. It doesn't do anything too terrible if clang can't handle HLSL, so it might be safe to enable everywhere as-is.
|
I think you could also advertise HLSL in the extension manifest: Lines 18 to 24 in 687314d |
| @@ -18,6 +18,7 @@ export const clangdDocumentSelector = [ | |||
| {scheme: 'file', language: 'cuda-cpp'}, | |||
| {scheme: 'file', language: 'objective-c'}, | |||
| {scheme: 'file', language: 'objective-cpp'}, | |||
| {scheme: 'file', language: 'hlsl'}, | |||
There was a problem hiding this comment.
this part is "just code", I think we can make it conditional on a config option.
| @@ -28,6 +28,7 @@ | |||
| "onLanguage:cuda-cpp", | |||
| "onLanguage:objective-c", | |||
| "onLanguage:objective-cpp", | |||
| "onLanguage:hlsl", | |||
There was a problem hiding this comment.
this can't be conditional, but I think this just wakes up the plugin when an HLSL file is opened, and would just waste a few resources if the documentSelector doesn't have HLSL.
|
Sorry, I keep losing my comment... I think we should get this in front of HLSL clang devs ASAP, and in front of "regular" users as soon as there's been an LLVM release with language support in a good state. At the moment I think all released versions of clangd will produce spurious errors on ~all valid programs. So I think it's too soon for it to be on by default, but we should be able to put it behind a setting. I'm not too worried about gating on clangd version, we have update/install prompts in the plugin now. |
This adds an option to enable HLSL support and only includes hlsl as a clangd document type if it is enabled.
|
@sam-mccall Are there any other changes you'd like me to make here? |
|
@sam-mccall & @i-ky, any additional changes I need to make here? |
|
LGTM, but I am not a keyholder here. |
|
@HighCommander4 or @hokein could one of you please review this? |
|
@sam-mccall any chance you can take another look at this? |
There was a problem hiding this comment.
@HighCommander4 or @hokein could one of you please review this?
I only have a passing familiarity with what HLSL is, but I looked through the discussion in this PR so far, and it seems like Sam is on board with adding this (disabled by default) and the updated patch addresses his comments, so I'm happy to approve this (modulo a comment inline).
Just to check my own understanding here: HLSL is being modelled as a "C family language" in clang, such that clang will build for it an AST represented using the same types (ASTContext, Decl, etc.) as a C/C++ AST, and this is why it's useful for clangd (whose features mostly operate on those AST node types) to be the language server for HLSL files?
I'm unsure if there is a good way to gate this support on a specific version of clang
The usual way to gate a client feature on server support is to have the server announce support for the feature in its server capabilities (around here; the capability name can be a non-standard extension that the server and client agree on). The client can check the capabilities in the initialize() method of a StaticFeature that it registers.
I'll leave it up to you if you'd like to do this, either in this patch or a follow-up. I'm also fine with not bothering with this.
| return vscode.languages.match(clangdDocumentSelector, document); | ||
| if (vscode.workspace.getConfiguration('clangd').get('enableHLSL')) | ||
| return vscode.languages.match(clangdDocumentSelector, document); | ||
| return vscode.languages.match(clangdDocumentSelector.slice(0, -1), document); |
There was a problem hiding this comment.
A couple of issues here:
- This is not the only use of
clangdDocumentSelector; its also used here - The
slice()call seems a bit brittle in that if someone adds a new element at the end of the selector, it would now slice that one off
Can we instead replace clangdDocumentSelector with a function whose implementation checks the config option and adds the HLSL entry to the returned array accordingly?
Note that the use of clangdDocumentSelector in the client options probably means that flipping the config option won't have full effect until a server restart, but that's probably unavoidable (the best we could do, if we wanted to, is probably to prompt the user to restart the server when the option is flipped).
|
One more thought: please include in the patch a change to CHANGELOG.md to add a new section at the top titled "Next", and a bullet point under it mentioning this new feature. |
Yes, HLSL is being added to Clang as a standard language. It has a few additional AST nodes to represent its unique syntaxes and semantics, but they are all derived from Clang I'll work on the other changes to address your feedback today. |
This change adds a list of supported langauges as part of the server capabilities. This is related to a PR to add HLSL support to the clangd VSCode plugin (clangd/vscode-clangd#392). The review there requested advertising HLSL support as a server capability. Adding a general "languages" capability seemed more appropriate.
|
I've posted a PR (llvm/llvm-project#75633) to add a list of languages to clangd's server capabilities. |
Clang is gaining support for HLSL. This change enables using clangd for the language server for vscode.
I'm unsure if there is a good way to gate this support on a specific version of clang, but when used with clang-15 it produces an error at the beginning of the file noting that compilation failed. It doesn't do anything too terrible if clang can't handle HLSL, so it might be safe to enable everywhere as-is.