[clangd] Added class for mapping TokenColorRules to their associated clangd TextMate scope

[jvikstrom]

Summary

Maps an array of TM scopes to the most specific scope that is added. Needed to have fast access to a rule when doing the actual coloring.

[jvikstrom]

Rebased into master.

[jvikstrom]

Updated to use string[][] as scopes.

[jvikstrom]

Updated patch description.

[hokein]

I think we could make the layering clearer:

• we now have a list of theme color rules, and scope names provided by clangd; and we we want to find the best match theme rule for a particular clangd scope;
• we could define a function like getBestThemeRule(clangd_scope_name: string) - when we render a token, we get the index of the lookup table, find the exact clangd scope name, and call the function to get the corresponding theme rule;
• we need to cache the result to avoid re-computing, we could make the function as a class method;

[jvikstrom]

Lazy load the best theme rule for a scope.

[jvikstrom]

Added a missing test case.

[hokein]

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

65 ↗

(On Diff #214814)

maybe just get<string>, and get rid of the following check.

119 ↗

(On Diff #214814)

I'd name it ThemeRuleMatcher?

131 ↗

(On Diff #214814)

hmm, here comes the question, we need algorithm to find the best match rule. not doing it in this patch is fine, please add a FIXME.

could you also document what's the strategy using here?

143 ↗

(On Diff #214814)

nit: no need to change this method, you could construct the ThemeRules from the returned results.

[jvikstrom]

Address comments.

[jvikstrom]

Removed stray edits from loadTheme.

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

131 ↗

(On Diff #214814)

But isn't the best match for a scope just the rule that is the longest prefix of the scope (or a perfect match if one exists)? (as that should be the most specific rule)

[hokein]

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

51 ↗

(On Diff #214834)

the variable name should be updated as well.

119 ↗

(On Diff #214834)

The best rule for a scope is the rule ... sounds like implementation details, should be moved to the function body.

128 ↗

(On Diff #214834)

The algorithm doesn't seems correct to me, if scope.length > rule.scope.length, then we drop it.

I think we should

• calculate the common prefix between two scopes
• update the bestRule if the length of common prefix is greater than the current best length

131 ↗

(On Diff #214814)

that depends, given the fact that the theme scope maybe not exactly the same with the scope provided by clangd. The longest-prefix match may not work well on cases like a.b.<theme-specific-field>.c.d and a.b.c.d. But we don't know yet without further experiment, I'd add a FIXME for revisiting the strategy here.

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

47 ↗

(On Diff #214834)

I'd use assert.deepEqual(tm.getBestThemeRule('c.b').scope, 'c.b'); to make the code more readable.

[jvikstrom]

Changed variable name.

[jvikstrom]

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

128 ↗

(On Diff #214834)

If the scope's length is less than the rule's length the rule can not be a prefix.
(Not sure I fully follow with what you mean in the first sentence though)

If we check common prefixes we run into this weird case (this is taken from the Light+ theme):

variable.css
variable.scss
variable.other.less
variable

With that kind of matching we have now this means that variable.other.less will match variable.other and variable.other.less and the variables would be colored as less variables while they should be variable.other. Same goes for variable.other.field.

And even if variable.other.less did not exist variable.other would still match variable.css and now be highlighted as css variables.

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

47 ↗

(On Diff #214834)

For the a case we are interested in the foreground color as well. Should I change the others and keep assert.deepEqual(tm.getBestThemeRule('a'), rules[1]); as is or be consistent?

[jvikstrom]

Added fixme for ranking.

[hokein]

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

115 ↗

(On Diff #214845)

I'd name it themeRules.

133 ↗

(On Diff #214845)

I think we could simplify the code like

if (scope.startsWith(rule.scope) && rule.scope.length > bestRule.scope.length) {
  ...
}

128 ↗

(On Diff #214834)

I thought that we are finding longest common prefix, but actually you are using a different one (I checked that "vscode-cpptools" is also using the same matching algorithm).

That sounds fair enough, could you add this context to the comment?

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

40 ↗

(On Diff #214845)

Even for the test, I'd use the real scopes, e.g. variable.css, variable.other.less.

47 ↗

(On Diff #214834)

a case is interesting here - we have duplicates, but actually we won't have duplicates in the theme rules (as we check the scope is being seen when parsing theme file).

I think for a, just use assert.deepEqual(tm.getBestThemeRule('a'), rules[1]); // we match the first element if there are duplicates; for others, use the scope.

[jvikstrom]

Simplified matching code. Use real scopes for test.

[hokein]

thanks, looks good.

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

new vscodelc.NotificationType<SemanticHighlightingParams, void>(
         'textDocument/semanticHighlighting');
 
 // 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 rules for the current theme.
+  themeRuleMatcher: ThemeRuleMatcher;
   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,
     };
   }
 
+  async loadCurrentTheme() {
+    this.themeRuleMatcher = new ThemeRuleMatcher(
+        await loadTheme(vscode.workspace.getConfiguration('workbench')
+                            .get<string>('colorTheme')));
+  }
+
   initialize(capabilities: vscodelc.ServerCapabilities,
              documentSelector: vscodelc.DocumentSelector|undefined) {
     // The semantic highlighting capability information is in the capabilities
     // object but to access the data we must first extend the ServerCapabilities
     // type.
     const serverCapabilities: vscodelc.ServerCapabilities&
         {semanticHighlighting?: {scopes : string[][]}} = capabilities;
     if (!serverCapabilities.semanticHighlighting)
       return;
     this.scopeLookupTable = serverCapabilities.semanticHighlighting.scopes;
+    this.loadCurrentTheme();
   }
 
   handleNotification(params: SemanticHighlightingParams) {}
 }
 
 // Converts a string of base64 encoded tokens into the corresponding array of
 // HighlightingTokens.
 export function decodeTokens(tokens: string): SemanticHighlightingToken[] {

 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.
   foreground: string;
 }
 
+export class ThemeRuleMatcher {
+  // The rules for the theme.
+  private themeRules: TokenColorRule[];
+  // A cache for the getBestThemeRule function.
+  private bestRuleCache: Map<string, TokenColorRule> = new Map();
+  constructor(rules: TokenColorRule[]) { this.themeRules = rules; }
+  // Returns the best rule for a scope.
+  getBestThemeRule(scope: string): TokenColorRule {
+    if (this.bestRuleCache.has(scope))
+      return this.bestRuleCache.get(scope);
+    let bestRule: TokenColorRule = {scope : '', foreground : ''};
+    this.themeRules.forEach((rule) => {
+      // The best rule for a scope is the rule that is the longest prefix of the
+      // scope (unless a perfect match exists in which case the perfect match is
+      // the best). If a rule is not a prefix and we tried to match with longest
+      // common prefix instead variables would be highlighted as `less`
+      // variables when using Light+ (as variable.other would be matched against
+      // variable.other.less in this case). Doing common prefix matching also
+      // means we could match variable.cpp to variable.css if variable.css
+      // occurs before variable in themeRules.
+      // FIXME: This is not defined in the TextMate standard (it is explicitly
+      // undefined, https://macromates.com/manual/en/scope_selectors). Might
+      // want to rank some other way.
+      if (scope.startsWith(rule.scope) &&
+          rule.scope.length > bestRule.scope.length)
+        // This rule matches and is more specific than the old rule.
+        bestRule = rule;
+    });
+    this.bestRuleCache.set(scope, bestRule);
+    return bestRule;
+  }
+}
+
 // Get all token color rules provided by the theme.
 function loadTheme(themeName: string): Promise<TokenColorRule[]> {
   const extension =
       vscode.extensions.all.find((extension: vscode.Extension<any>) => {
         const contribs = extension.packageJSON.contributes;
         if (!contribs || !contribs.themes)
           return false;
         return contribs.themes.some((theme: any) => theme.id === themeName ||

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

 import * as assert from 'assert';
 import * as path from 'path';
 
-import * as TM from '../src/semantic-highlighting';
+import * as SM from '../src/semantic-highlighting';
 
 suite('SemanticHighlighting Tests', () => {
   test('Parses arrays of textmate themes.', async () => {
     const themePath =
         path.join(__dirname, '../../test/assets/includeTheme.jsonc');
-    const scopeColorRules = await TM.parseThemeFile(themePath);
+    const scopeColorRules = await SM.parseThemeFile(themePath);
     const getScopeRule = (scope: string) =>
         scopeColorRules.find((v) => v.scope === scope);
     assert.equal(scopeColorRules.length, 3);
     assert.deepEqual(getScopeRule('a'), {scope : 'a', foreground : '#fff'});
     assert.deepEqual(getScopeRule('b'), {scope : 'b', foreground : '#000'});
     assert.deepEqual(getScopeRule('c'), {scope : 'c', foreground : '#bcd'});
   });
   test('Decodes tokens correctly', () => {
const expected = [
       ],
       [
         {character : 0, scopeIndex : 9, length : 3},
         {character : 4, scopeIndex : 0, length : 1},
         {character : 10, scopeIndex : 0, length : 1}
       ]
     ];
     testCases.forEach((testCase, i) => assert.deepEqual(
-                          TM.decodeTokens(testCase), expected[i]));
+                          SM.decodeTokens(testCase), expected[i]));
+  });
+  test('ScopeRules overrides for more specific themes', () => {
+    const rules = [
+      {scope : 'variable.other.css', foreground : '1'},
+      {scope : 'variable.other', foreground : '2'},
+      {scope : 'storage', foreground : '3'},
+      {scope : 'storage.static', foreground : '4'},
+      {scope : 'storage', foreground : '5'},
+      {scope : 'variable.other.parameter', foreground : '6'},
+    ];
+    const tm = new SM.ThemeRuleMatcher(rules);
+    assert.deepEqual(tm.getBestThemeRule('variable.other.cpp').scope,
+                     'variable.other');
+    assert.deepEqual(tm.getBestThemeRule('storage.static').scope,
+                     'storage.static');
+    assert.deepEqual(
+        tm.getBestThemeRule('storage'),
+        rules[2]); // Match the first element if there are duplicates.
+    assert.deepEqual(tm.getBestThemeRule('variable.other.parameter').scope,
+                     'variable.other.parameter');
+    assert.deepEqual(tm.getBestThemeRule('variable.other.parameter.cpp').scope,
+                     'variable.other.parameter');
   });
 });