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

[clangd] Add languages as server capabilities #75633

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

llvm-beanz
Copy link
Collaborator

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.

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.
@llvmbot llvmbot added the clangd label Dec 15, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 15, 2023

@llvm/pr-subscribers-clangd

Author: Chris B (llvm-beanz)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/75633.diff

2 Files Affected:

  • (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+2)
  • (modified) clang-tools-extra/clangd/test/initialize-params.test (+8)
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..4136155403fc1d 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -623,6 +623,8 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
       {"clangdInlayHintsProvider", true},
       {"inlayHintProvider", true},
       {"foldingRangeProvider", true},
+      {"languages",
+       {"c", "cpp", "cuda-cpp", "objective-c", "objective-cpp", "hlsl"}},
   };
 
   {
diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test
index a1fdae9870ab6e..81f00e0f469c52 100644
--- a/clang-tools-extra/clangd/test/initialize-params.test
+++ b/clang-tools-extra/clangd/test/initialize-params.test
@@ -48,6 +48,14 @@
 # CHECK-NEXT:      "implementationProvider": true,
 # CHECK-NEXT:      "inactiveRegionsProvider": true,
 # CHECK-NEXT:      "inlayHintProvider": true,
+# CHECK-NEXT:      "languages": [
+# CHECK-NEXT:        "c",
+# CHECK-NEXT:        "cpp",
+# CHECK-NEXT:        "cuda-cpp",
+# CHECK-NEXT:        "objective-c",
+# CHECK-NEXT:        "objective-cpp",
+# CHECK-NEXT:        "hlsl"
+# CHECK-NEXT:      ],
 # CHECK-NEXT:      "memoryUsageProvider": true,
 # CHECK-NEXT:      "referencesProvider": true,
 # CHECK-NEXT:      "renameProvider": true,

Copy link
Collaborator

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

Thanks for the patch!

I like the idea of having the server announce support for a list of languages, that seems nice and general and extensible.

A couple of thoughts:

  1. Since the key we're adding to the server capabilities is a non-standard extension, I'm a bit uneasy about giving it a general name like "languages". If the LSP were to add a key with that name but a different format or interpretation, we could run into trouble. Can we name it "clangdLanguages" instead, thereby staying within our own (improvised) "namespace"?

  2. Given that the plan described by Sam in this comment is to enable vscode-clangd support for HLSL by default once clang's support for HLSL is a good enough state, what do you think about the following arrangement:

    • don't include "hlsl" in "clangdLanguages" yet
    • add "hlsl" to "clangdLanguages" once the "good enough" threshold is reached
    • on the client side, add HLSL to the document selector if the server announces support for "hlsl" in "clangdLanguages", or a client-side preference (which we could name "experimental", e.g. "clangd.experimentalHLSLSupport") is enabled?

    This way:

    • early adopters can set "clangd.experimentalHLSLSupport" with an up-to-date client paired even with a clangd 17 or older server, and get the corresponding level of support
    • once the implementation quality is good enough, the server can start announcing the support and all clients (not just early adopters who set the pref) will have it by default with newer server versions

@llvm-beanz
Copy link
Collaborator Author

  1. Since the key we're adding to the server capabilities is a non-standard extension, I'm a bit uneasy about giving it a general name like "languages". If the LSP were to add a key with that name but a different format or interpretation, we could run into trouble. Can we name it "clangdLanguages" instead, thereby staying within our own (improvised) "namespace"?

+1 This makes sense.

  1. Given that the plan described by Sam in this comment is to enable vscode-clangd support for HLSL by default once clang's support for HLSL is a good enough state, what do you think about the following arrangement:
    • don't include "hlsl" in "clangdLanguages" yet
    • add "hlsl" to "clangdLanguages" once the "good enough" threshold is reached
    • on the client side, add HLSL to the document selector if the server announces support for "hlsl" in "clangdLanguages", or a client-side preference (which we could name "experimental", e.g. "clangd.experimentalHLSLSupport") is enabled?

What about a slightly different take? What if in addition to adding clangdLanguages (without HLSL) we also add clangdExperimentalFeatures and add HLSL there. The problem I'm trying to solve here is to avoid someone enabling HLSL on a language server that doesn't advertise that it has support.

That would still allow your points:

  • early adopters can set "clangd.experimentalHLSLSupport" with an up-to-date client paired even with a clangd 17 or older server, and get the corresponding level of support
  • once the implementation quality is good enough, the server can start announcing the support and all clients (not just early adopters who set the pref) will have it by default with newer server versions

@HighCommander4
Copy link
Collaborator

What about a slightly different take? What if in addition to adding clangdLanguages (without HLSL) we also add clangdExperimentalFeatures and add HLSL there. The problem I'm trying to solve here is to avoid someone enabling HLSL on a language server that doesn't advertise that it has support.

That sounds fine to me!

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

Successfully merging this pull request may close these issues.

None yet

3 participants