[clangd] Cleans up the semantic highlighting resources if clangd crashes.

[jvikstrom]

Summary

Disposes of the vscode listeners when clangd crashes and reuses the old highlighter when it restarts. The reason for reusing the highlighter is because this way the highlightings will not disappear as we won't have to dispose of them.

[hokein]

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

60 ↗

(On Diff #217152)

nit: call it subscriptions to better align with vscode pattern.

114 ↗

(On Diff #217152)

we just dispose some class members (leading the class to an intermediate state), I'd suggest that we dispose the whole class.

nit: please remove the "clangd crashes" bit in the API.

[jvikstrom]

Renamed disposables to subscriptions and removed clangd crashes in api.

[jvikstrom]

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

114 ↗

(On Diff #217152)

The only thing remaining to dispose is the highlighter. Should we remove all highlightings if clangd restarts?

[jvikstrom]

Dispose of all resources when disposing.

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

const clientOptions: vscodelc.LanguageClientOptions = {
         revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
     };
 
   const clangdClient = new ClangdLanguageClient('Clang Language Server',
                                                 serverOptions, clientOptions);
   const semanticHighlightingFeature =
       new semanticHighlighting.SemanticHighlightingFeature();
   clangdClient.registerFeature(semanticHighlightingFeature);
-  // The notification handler must be registered after the client is ready or
-  // the client will crash.
-  clangdClient.onReady().then(
-      () => clangdClient.onNotification(
-          semanticHighlighting.NotificationType,
-          semanticHighlightingFeature.handleNotification.bind(
-              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;
export function activate(context: vscode.ExtensionContext) {
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
       () => { status.updateStatus(); }));
   clangdClient.onDidChangeState(({newState}) => {
     if (newState == vscodelc.State.Running) {
       // clangd starts or restarts after crash.
       clangdClient.onNotification(
           'textDocument/clangd.fileStatus',
           (fileStatus) => { status.onFileUpdated(fileStatus); });
+      clangdClient.onNotification(
+          semanticHighlighting.NotificationType,
+          semanticHighlightingFeature.handleNotification.bind(
+              semanticHighlightingFeature));
     } else if (newState == vscodelc.State.Stopped) {
       // Clear all cached statuses when clangd crashes.
       status.clear();
+      semanticHighlightingFeature.dispose();
     }
   })
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
       'clangd-vscode.activate', async () => {}));
 }

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

 // The feature that should be registered in the vscode lsp for enabling
 // experimental semantic highlighting.
 export class SemanticHighlightingFeature implements vscodelc.StaticFeature {
   // The TextMate scope lookup table. A token with scope index i has the scopes
   // on index i in the lookup table.
   scopeLookupTable: string[][];
   // The object that applies the highlightings clangd sends.
   highlighter: Highlighter;
+  // Any disposables that should be cleaned up when clangd crashes.
+  private subscriptions: vscode.Disposable[] = [];
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
     // Extend the ClientCapabilities type and add semantic highlighting
     // capability to the object.
     const textDocumentCapabilities: vscodelc.TextDocumentClientCapabilities&
         {semanticHighlightingCapabilities?: {semanticHighlighting : boolean}} =
         capabilities.textDocument;
     textDocumentCapabilities.semanticHighlightingCapabilities = {
       semanticHighlighting : true,
const serverCapabilities: vscodelc.ServerCapabilities&
         {semanticHighlighting?: {scopes : string[][]}} = capabilities;
     if (!serverCapabilities.semanticHighlighting)
       return;
     this.scopeLookupTable = serverCapabilities.semanticHighlighting.scopes;
     // Important that highlighter is created before the theme is loading as
     // otherwise it could try to update the themeRuleMatcher without the
     // highlighter being created.
     this.highlighter = new Highlighter(this.scopeLookupTable);
+    this.subscriptions.push(vscode.Disposable.from(this.highlighter));
     this.loadCurrentTheme();
     // Event handling for handling with TextDocuments/Editors lifetimes.
-    vscode.window.onDidChangeVisibleTextEditors(
+    this.subscriptions.push(vscode.window.onDidChangeVisibleTextEditors(
         (editors: vscode.TextEditor[]) =>
             editors.forEach((e) => this.highlighter.applyHighlights(
-                                e.document.uri.toString())));
-    vscode.workspace.onDidCloseTextDocument(
-        (doc) => this.highlighter.removeFileHighlightings(doc.uri.toString()));
+                                e.document.uri.toString()))));
+    this.subscriptions.push(vscode.workspace.onDidCloseTextDocument(
+        (doc) => this.highlighter.removeFileHighlightings(doc.uri.toString())));
   }
 
   handleNotification(params: SemanticHighlightingParams) {
     const lines: SemanticHighlightingLine[] = params.lines.map(
         (line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
     this.highlighter.highlight(params.textDocument.uri, lines);
   }
+  // Disposes of all disposable resources used by this object.
+  public dispose() {
+    this.subscriptions.forEach((d) => d.dispose());
+    this.subscriptions = [];
+  }
 }
 
 // Converts a string of base64 encoded tokens into the corresponding array of
 // HighlightingTokens.
 export function decodeTokens(tokens: string): SemanticHighlightingToken[] {
   const scopeMask = 0xFFFF;
   const lenShift = 0x10;
   const uint32Size = 4;
export class Highlighter {
   // SemanticHighlightingToken with scopeIndex i should have the decoration at
   // index i in this list.
   private decorationTypes: vscode.TextEditorDecorationType[] = [];
   // The clangd TextMate scope lookup table.
   private scopeLookupTable: string[][];
   constructor(scopeLookupTable: string[][]) {
     this.scopeLookupTable = scopeLookupTable;
   }
+  public dispose() {
+    this.files.clear();
+    this.decorationTypes.forEach((t) => t.dispose());
+    // Dispose must not be not called multiple times if initialize is
+    // called again.
+    this.decorationTypes = [];
+  }
   // This function must be called at least once or no highlightings will be
   // done. Sets the theme that is used when highlighting. Also triggers a
   // recolorization for all current highlighters. Should be called whenever the
   // theme changes and has been loaded. Should also be called when the first
   // theme is loaded.
   public initialize(themeRuleMatcher: ThemeRuleMatcher) {
     this.decorationTypes.forEach((t) => t.dispose());
     this.decorationTypes = this.scopeLookupTable.map((scopes) => {
public highlight(fileUri: string,
     if (!this.files.has(fileUri)) {
       this.files.set(fileUri, new Map());
     }
     const fileHighlightings = this.files.get(fileUri);
     highlightingLines.forEach((line) => fileHighlightings.set(line.line, line));
     this.applyHighlights(fileUri);
   }
 
+  // Applies all the highlightings currently stored for a file with fileUri.
+  public applyHighlights(fileUri: string) {
+    if (!this.files.has(fileUri))
+      // There are no highlightings for this file, must return early or will get
+      // out of bounds when applying the decorations below.
+      return;
+    if (!this.decorationTypes.length)
+      // Can't apply any decorations when there is no theme loaded.
+      return;
+    // This must always do a full re-highlighting due to the fact that
+    // TextEditorDecorationType are very expensive to create (which makes
+    // incremental updates infeasible). For this reason one
+    // TextEditorDecorationType is used per scope.
+    const ranges = this.getDecorationRanges(fileUri);
+    vscode.window.visibleTextEditors.forEach((e) => {
+      if (e.document.uri.toString() !== fileUri)
+        return;
+      this.decorationTypes.forEach((d, i) => e.setDecorations(d, ranges[i]));
+    });
+  }
+
   // Called when a text document is closed. Removes any highlighting entries for
   // the text document that was closed.
   public removeFileHighlightings(fileUri: string) {
     // If there exists no entry the call to delete just returns false.
     this.files.delete(fileUri);
   }
 
   // Gets the uris as strings for the currently visible text editors.
lines.forEach((line) => {
       line.tokens.forEach((token) => {
         decorations[token.scopeIndex].push(new vscode.Range(
             new vscode.Position(line.line, token.character),
             new vscode.Position(line.line, token.character + token.length)));
       });
     });
     return decorations;
   }
-
-  // Applies all the highlightings currently stored for a file with fileUri.
-  public applyHighlights(fileUri: string) {
-    if (!this.files.has(fileUri))
-      // There are no highlightings for this file, must return early or will get
-      // out of bounds when applying the decorations below.
-      return;
-    if (!this.decorationTypes.length)
-      // Can't apply any decorations when there is no theme loaded.
-      return;
-    // This must always do a full re-highlighting due to the fact that
-    // TextEditorDecorationType are very expensive to create (which makes
-    // incremental updates infeasible). For this reason one
-    // TextEditorDecorationType is used per scope.
-    const ranges = this.getDecorationRanges(fileUri);
-    vscode.window.visibleTextEditors.forEach((e) => {
-      if (e.document.uri.toString() !== fileUri)
-        return;
-      this.decorationTypes.forEach((d, i) => e.setDecorations(d, ranges[i]));
-    });
-  }
 }
 
 // A rule for how to color TextMate scopes.
 interface TokenColorRule {
   // A TextMate scope that specifies the context of the token, e.g.
   // "entity.name.function.cpp".
   scope: string;
   // foreground is the color tokens of this scope should have.

clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

test('Colorizer groups decorations correctly', async () => {
     const tm = new semanticHighlighting.ThemeRuleMatcher([
       {scope : 'variable', foreground : '1'},
       {scope : 'entity.type', foreground : '2'},
     ]);
     // Recolorizes when initialized.
     highlighter.highlight('file1', []);
     assert.deepEqual(highlighter.applicationUriHistory, [ 'file1' ]);
     highlighter.initialize(tm);
-    assert.deepEqual(highlighter.applicationUriHistory, [ 'file1', 'file1', 'file2' ]);
+    assert.deepEqual(highlighter.applicationUriHistory,
+                     [ 'file1', 'file1', 'file2' ]);
     // Groups decorations into the scopes used.
     let highlightingsInLine: semanticHighlighting.SemanticHighlightingLine[] = [
       {
         line : 1,
         tokens : [
           {character : 1, length : 2, scopeIndex : 1},
           {character : 10, length : 2, scopeIndex : 2},
         ]