[clangd] Fixes colon escaping on Windows

[lh123]

Summary

vscode always escapes the colon on the file uri, which causes the semantic highlighting fails on windows.

fixes: https://github.com/clangd/clangd/issues/176

[lh123]

it will cause whitspace escaped problem.

[lh123]

update diff

[lh123]

Remove irrelevant files from the patch

[ilya-biryukov]

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

128 ↗

(On Diff #228405)

Could we accept a URI in the highlight (and similar function in highlighting) instead and compare the URIs instead of strings in the Highlighter.applyHighlightings?

i.e. in code:

fileUri : string;
if (e.document.uri.toString() !== fileUri) 
  return;

Could instead be:

fileUri : Uri;
if (e.document.uri.toString() !== fileUri.toString()) 
  return;

This should normalize accordingly and is generally safer and more readable than passing around strings.

[lh123]

address comment

[lh123]

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

128 ↗

(On Diff #228405)

Yes, you are right, I will refactor it.

[ilya-biryukov]

LGTM, thanks!
Should I land this patch for you?

If you don't have commit access yet, you could consider applying to get one.

[lh123]

Thanks for your reminder, I have sent an email to apply for commit access, but I don't know how long it will take.

Should I land this patch for you?

Yes, you can land this patch.

[ilya-biryukov]

Sorry for the delay, forgot to submit on Friday.
Also updated package-lock.json before submitting.

[hokein]

I believe this patch fixes the issue, but we need to update the file test/semantic-highlighting.test.ts), the test "Colorizer groups decorations correctly" is diverged from the actual code, although it is still passed (both string and vscode.Uri have the toString method unfortunately).

@lh123 do you mind doing that?

[lh123]

In D69996#1740412, @hokein wrote:

I believe this patch fixes the issue, but we need to update the file test/semantic-highlighting.test.ts), the test "Colorizer groups decorations correctly" is diverged from the actual code, although it is still passed (both string and vscode.Uri have the toString method unfortunately).

@lh123 do you mind doing that?

Sorry, I forgot to update the test case, I have submitted a new patch. D70078

package.json

 {
     "name": "vscode-clangd",
     "displayName": "vscode-clangd",
     "description": "Clang Language Server",
-    "version": "0.0.18",
+    "version": "0.0.19",
     "publisher": "llvm-vs-code-extensions",
     "homepage": "https://clang.llvm.org/extra/clangd.html",
     "engines": {
         "vscode": "^1.36.0"
     },
     "categories": [
         "Programming Languages",
         "Linters",

src/semantic-highlighting.ts

this.subscriptions.push(
         vscode.workspace.onDidChangeConfiguration((conf) => {
           if (!conf.affectsConfiguration('workbench.colorTheme'))
             return;
           this.loadCurrentTheme();
         }));
     this.loadCurrentTheme();
     // Event handling for handling with TextDocuments/Editors lifetimes.
     this.subscriptions.push(vscode.window.onDidChangeVisibleTextEditors(
-        (editors: vscode.TextEditor[]) =>
-            editors.forEach((e) => this.highlighter.applyHighlights(
-                                e.document.uri.toString()))));
+        (editors: vscode.TextEditor[]) => editors.forEach(
+            (e) => this.highlighter.applyHighlights(e.document.uri))));
     this.subscriptions.push(vscode.workspace.onDidCloseTextDocument(
-        (doc) => this.highlighter.removeFileHighlightings(doc.uri.toString())));
+        (doc) => this.highlighter.removeFileHighlightings(doc.uri)));
   }
 
   handleNotification(params: SemanticHighlightingParams) {
     const lines: SemanticHighlightingLine[] = params.lines.map(
         (line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
-    this.highlighter.highlight(params.textDocument.uri, lines);
+    this.highlighter.highlight(vscode.Uri.parse(params.textDocument.uri),
+                               lines);
   }
   // Disposes of all disposable resources used by this object.
   public dispose() {
     this.subscriptions.forEach((d) => d.dispose());
     this.subscriptions = [];
   }
 }
 
public initialize(themeRuleMatcher: ThemeRuleMatcher) {
     });
     this.getVisibleTextEditorUris().forEach((fileUri) =>
                                                 this.applyHighlights(fileUri));
   }
 
   // Adds incremental highlightings to the current highlightings for the file
   // with fileUri. Also applies the highlightings to any associated
   // TextEditor(s).
-  public highlight(fileUri: string,
+  public highlight(fileUri: vscode.Uri,
                    highlightingLines: SemanticHighlightingLine[]) {
-    if (!this.files.has(fileUri)) {
-      this.files.set(fileUri, new Map());
+    const fileUriStr = fileUri.toString();
+    if (!this.files.has(fileUriStr)) {
+      this.files.set(fileUriStr, new Map());
     }
-    const fileHighlightings = this.files.get(fileUri);
+    const fileHighlightings = this.files.get(fileUriStr);
     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))
+  public applyHighlights(fileUri: vscode.Uri) {
+    const fileUriStr = fileUri.toString();
+    if (!this.files.has(fileUriStr))
       // 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)
+      if (e.document.uri.toString() !== fileUriStr)
         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) {
+  public removeFileHighlightings(fileUri: vscode.Uri) {
     // If there exists no entry the call to delete just returns false.
-    this.files.delete(fileUri);
+    this.files.delete(fileUri.toString());
   }
 
   // Gets the uris as strings for the currently visible text editors.
-  protected getVisibleTextEditorUris(): string[] {
-    return vscode.window.visibleTextEditors.map((e) =>
-                                                    e.document.uri.toString());
+  protected getVisibleTextEditorUris(): vscode.Uri[] {
+    return vscode.window.visibleTextEditors.map((e) => e.document.uri);
   }
 
   // Returns the ranges that should be used when decorating. Index i in the
   // range array has the decoration type at index i of this.decorationTypes.
-  protected getDecorationRanges(fileUri: string): vscode.Range[][] {
-    if (!this.files.has(fileUri))
+  protected getDecorationRanges(fileUri: vscode.Uri): vscode.Range[][] {
+    const fileUriStr = fileUri.toString();
+    if (!this.files.has(fileUriStr))
       // this.files should always have an entry for fileUri if we are here. But
       // if there isn't one we don't want to crash the extension. This is also
       // useful for tests.
       return [];
     const lines: SemanticHighlightingLine[] =
-        Array.from(this.files.get(fileUri).values());
+        Array.from(this.files.get(fileUriStr).values());
     const decorations: vscode.Range[][] = this.decorationTypes.map(() => []);
     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)));
       });
     });