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

Enable clangd on HLSL sources #392

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

llvm-beanz
Copy link

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.

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-ky
Copy link

i-ky commented Oct 14, 2022

I think you could also advertise HLSL in the extension manifest:

"keywords": [
"C",
"C++",
"clang",
"clangd",
"LLVM"
],

@@ -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'},
Copy link
Member

Choose a reason for hiding this comment

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

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",
Copy link
Member

Choose a reason for hiding this comment

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

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.

@sam-mccall
Copy link
Member

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.

llvm-beanz and others added 2 commits October 28, 2022 09:55
This adds an option to enable HLSL support and only includes hlsl as a
clangd document type if it is enabled.
@llvm-beanz
Copy link
Author

@sam-mccall Are there any other changes you'd like me to make here?

@llvm-beanz
Copy link
Author

@sam-mccall & @i-ky, any additional changes I need to make here?

@i-ky
Copy link

i-ky commented Sep 30, 2023

LGTM, but I am not a keyholder here.

@llvm-beanz
Copy link
Author

@HighCommander4 or @hokein could one of you please review this?

@llvm-beanz
Copy link
Author

@sam-mccall any chance you can take another look at this?

Copy link
Contributor

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

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

@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);
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of issues here:

  1. This is not the only use of clangdDocumentSelector; its also used here
  2. 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).

@HighCommander4
Copy link
Contributor

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.

@llvm-beanz
Copy link
Author

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?

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 Decl, Expr and Stmt base types and integrated in the core Clang AST library.

I'll work on the other changes to address your feedback today.

llvm-beanz added a commit to llvm-beanz/llvm-project that referenced this pull request Dec 15, 2023
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.
@llvm-beanz
Copy link
Author

I've posted a PR (llvm/llvm-project#75633) to add a list of languages to clangd's server capabilities.

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.

4 participants