[clangd] Added highlighting for structured bindings.

[jvikstrom]

Summary

Structured bindings are in a BindingDecl. The decl the declRefExpr points to are the BindingDecls. So this adds an additional if statement in the addToken function to highlight them.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

236

nit: could you spell out the type? it is not straight forward to infer from the method name.

238

nit: we can group the above two ifs into one, like if (const auto* D = dyn_cast_or_null<...>(...getReferencedDecl).

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

437

can we add a non-primitive field and test it?

442

From the code, I can't tell the Variable for the binding decl is from the underlying namedDecl or the fallback, could you add a comment clarifying it?

447

The above 4 lines seems not relevant to this patch, I think?

[jvikstrom]

Abandoned trying to highlight the same as the bound decl.

Reasoning: If you have a reference to a parameter we are not trying to highlight the reference as a parameter, the reference is highlighted as a variable. Don't really think bindings should be different.

[hokein]

I don't have a strong preference on how to highlight BindingDecl, but I think highlighting them as Variable is better than no highlighting them at all, so LGTM from myside. @ilya-biryukov thoughts?

again, please update the patch description in phabricator and your git commit message.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

443

this is not related to the patch, but the highlighting behavior for auto here is weird, some of them are highlighted while some of them are not.

[jvikstrom]

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

443

The reason for that is broken down here: https://reviews.llvm.org/D66516#inline-599982

Basically we don't traverse the type hierarchy when trying to add highlightings for auto so if we have a pointer/array type it will not find the actual type and just silently not add any highlightings.

[ilya-biryukov]

In D66738#1652285, @hokein wrote:

I don't have a strong preference on how to highlight BindingDecl, but I think highlighting them as Variable is better than no highlighting them at all, so LGTM from myside. @ilya-biryukov thoughts?

Also think highlighting them as variables is the right thing.
e.g. if I do something like:

void foo(std::pair<int, int> p) {
  auto [f,s] = p;
  return f + s;
}

this is roughly equivalent to:

void foo(std::pair<int, int> p) {
  auto f = p.first;
  auto s = p.second;
  return f + s;
}

Therefore, it should also be highlighted in the same way.

[hokein]

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

443

thanks for the explanation.

clang-tools-extra/clangd/SemanticHighlighting.cpp

void addToken(SourceLocation Loc, const NamedDecl *D) {
     if (isa<ParmVarDecl>(D)) {
       addToken(Loc, HighlightingKind::Parameter);
       return;
     }
     if (isa<VarDecl>(D)) {
       addToken(Loc, HighlightingKind::Variable);
       return;
     }
+    if (const auto *B = dyn_cast<BindingDecl>(D)) {
+      // If we can find the underlying decl and highlight that we should do it.
+      // So first try to find and highlight the underlying NamedDecl for the
+      // binding if one exist.
+      if (const auto *BB = B->getBinding())
+        if (const auto *RD = BB->getReferencedDeclOfCallee())
+          if (const auto *D = dyn_cast<NamedDecl>(RD)) {
+            addToken(Loc, D);
+            return;
+          }
+      // Could not find a more specific decl for this BindingDecl. So just
+      // highlight as a normal variable.
+      addToken(Loc, HighlightingKind::Variable);
+      return;
+    }
     if (isa<FunctionDecl>(D)) {
       addToken(Loc, HighlightingKind::Function);
       return;
     }
     if (isa<NamespaceDecl>(D)) {
       addToken(Loc, HighlightingKind::Namespace);
       return;
     }

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

R"cpp(
       #define assert(COND) if (!(COND)) { fail("assertion failed" #COND); }
       $Primitive[[int]] $Variable[[x]];
       $Primitive[[int]] $Variable[[y]];
       $Primitive[[int]] $Function[[f]]();
       $Primitive[[void]] $Function[[foo]]() {
         assert($Variable[[x]] != $Variable[[y]]);
         assert($Variable[[x]] != $Function[[f]]());
       }
+    )cpp",
+    R"cpp(
+      struct $Class[[S]] {
+        $Primitive[[float]] $Field[[Member]];
+      };
+      $Class[[S]] $Function[[foo]]();
+      $Primitive[[void]] $Function[[f]]() {
+        $Primitive[[int]] $Variable[[A]][2] = {1,2};
+        auto [$Variable[[B1]], $Variable[[B2]]] = $Variable[[A]];
+        auto& [$Variable[[R1]], $Variable[[R2]]] = $Variable[[A]];
+        $Class[[auto]] [$Field[[M1]]] = $Class[[S]]();
+        $Class[[auto]] [$Field[[F1]]] = $Function[[foo]]();
+        $Field[[M1]] += 12.2;
+        $Variable[[B1]] += 2;
+        $Class[[S]] $Variable[[SArr]][2] = {$Class[[S]](), $Class[[S]]()};
+        auto [$Variable[[S1]], $Variable[[S2]]] = $Variable[[SArr]];
+      }
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 
   checkHighlightings(R"cpp(
     class $Class[[A]] {
       #include "imp.h"