[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 ↗

(On Diff #217130)

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

238 ↗

(On Diff #217130)

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 ↗

(On Diff #217130)

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

442 ↗

(On Diff #217130)

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 ↗

(On Diff #217130)

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 ↗

(On Diff #217652)

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 ↗

(On Diff #217652)

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 ↗

(On Diff #217652)

thanks for the explanation.

clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp

void addToken(SourceLocation Loc, const NamedDecl *D) {
     }
     if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
       addToken(Loc, VD->isStaticDataMember()
                         ? HighlightingKind::StaticField
                         : VD->isLocalVarDecl() ? HighlightingKind::LocalVariable
                                                : HighlightingKind::Variable);
       return;
     }
+    if (isa<BindingDecl>(D)) {
+      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/trunk/clangd/unittests/SemanticHighlightingTests.cpp

)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[[Value]];
+        $Class[[S]] *$Field[[Next]];
+      };
+      $Class[[S]] $Variable[[Global]][2] = {$Class[[S]](), $Class[[S]]()};
+      $Primitive[[void]] $Function[[f]]($Class[[S]] $Parameter[[P]]) {
+        $Primitive[[int]] $LocalVariable[[A]][2] = {1,2};
+        auto [$Variable[[B1]], $Variable[[B2]]] = $LocalVariable[[A]];
+        auto [$Variable[[G1]], $Variable[[G2]]] = $Variable[[Global]];
+        $Class[[auto]] [$Variable[[P1]], $Variable[[P2]]] = $Parameter[[P]];
+        // Highlights references to BindingDecls.
+        $Variable[[B1]]++;
+      }
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 
   checkHighlightings(R"cpp(
     class $Class[[A]] {
       #include "imp.h"
EXPECT_THAT(ActualDiff,
               testing::UnorderedElementsAre(
                   testing::AllOf(LineNumber(3), EmptyHighlightings()),
                   testing::AllOf(LineNumber(4), EmptyHighlightings())));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 No newline at end of file