[clangd][vscode] Add a flag to enable semantic highlighting in clangd

[ilya-biryukov]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

Why not avoid calling clangdClient.registerFeature instead?
Would allow to:

• not change the SemanticHighlightingFeature class, keeping it simpler,
• ensure we do not do any extra work if the feature is disabled.

[hokein]

address the comment, don't register the feature.

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

good point, done.

[ilya-biryukov]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

Should we update other uses of semanticHighlightingFeature too?

context.subscriptions.push(vscode.Disposable.from(semanticHighlightingFeature)) probably ensures we call dispose() when the clangdClient is getting removed, I guess we definitely don't need that.

Other uses as well:

• no need to pass notification is highlighting is disabled.
• no need to cleanup if highlighting is disabled.

Maybe assign null or undefined to semanticHighlightingFeature when the flag is false?
At each usage we can check whether the semanticHighlightingFeature is not null and only call relevant methods if that's the case.

[hokein]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

I don't think it is worth updating all usages, it is no harm to keep them here even when the highlighting is disabled (the dispose is a no-op; we never receive notifications from clangd); and it would add more guard code which I'd avoid.

[ilya-biryukov]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

How can we be sure that nothing bad is going to happen?
In particular, we are "binding" notification handling, but never registered a feature. How can we be sure we won't actually get any notifications?

If we don't create the object in the first place, we are confident nothing harmful can be done with it.

[hokein]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

How can we be sure we won't actually get any notifications?

If we receive a notification, that means we have clangd bugs.

I understand you point here, an ideal solution is to avoid too many usages of SemanticHighlightingFeature in the client side, after D67165, it'd help simplify the patch here.

[ilya-biryukov]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

If we receive a notification, that means we have clangd bugs.

True, but that might happen. It'd be better to not break in that case.
D67165 is definitely moving in the right direction, thanks!

[ilya-biryukov]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

It should be much simpler to not construct the semanticHighlightingFeature with D67165, right?

if (getConfig<boolean>('semanticHighlighting')) {
  const semanticHighlightingFeature =
      new semanticHighlighting.SemanticHighlightingFeature();
  context.subscriptions.push(
      vscode.Disposable.from(semanticHighlightingFeature));
  clangdClient.registerFeature(semanticHighlightingFeature);
}

Could we do that?

[hokein]

rebase and simplify the code.

[hokein]

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

113 ↗

(On Diff #218445)

yes, exactly.

[ilya-biryukov]

LGTM, thanks!

clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json

"contributes": {
                 "clangd.syncFileEvents": {
                     "type": "boolean",
                     "default": true,
                     "description": "Whether or not to send file events to clangd (File created, changed or deleted). This can be disabled for performance consideration."
                 },
                 "clangd.trace": {
                     "type": "string",
                     "description": "Names a file that clangd should log a performance trace to, in chrome trace-viewer JSON format."
+                },
+                "clangd.semanticHighlighting": {
+                    "type": "boolean",
+                    "default": "false",
+                    "description": "Enable semantic highlighting in clangd"
                 }
             }
         },
         "commands": [
             {
                 "command": "clangd-vscode.switchheadersource",
                 "title": "Switch between Source/Header"
             },

clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

const clientOptions: vscodelc.LanguageClientOptions = {
         },
         initializationOptions: { clangdFileStatus: true },
         // Do not switch to output window when clangd returns output
         revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
     };
 
   const clangdClient = new ClangdLanguageClient('Clang Language Server',
                                                 serverOptions, clientOptions);
+  if (getConfig<boolean>('semanticHighlighting')) {
     const semanticHighlightingFeature =
       new semanticHighlighting.SemanticHighlightingFeature(clangdClient,
-                                                           context);
     context.subscriptions.push(
       vscode.Disposable.from(semanticHighlightingFeature));
     clangdClient.registerFeature(semanticHighlightingFeature);
+  }
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(
       'clangd-vscode.switchheadersource', async () => {
         const uri =
             vscode.Uri.file(vscode.window.activeTextEditor.document.fileName);
         if (!uri) {
           return;