[clangd] Handle members of anon structs in SelectionTree

[kadircet]

Summary

References to fields inside anon structs contain an implicit children
for the container, which has the same SourceLocation with the field.
This was resulting in SelectionTree always picking the anon-struct rather than
the field as the selection.

This patch prevents that by claiming the range for the field early.

Fixes https://github.com/clangd/clangd/issues/877.

[kadircet]

clang-tools-extra/clangd/Selection.cpp

753

in theory this check should be redundant, as each field should own its tokens. but given the complexity of C++ just wanted to be conservative.

[sammccall]

Thanks for fixing!

Early-claim is a blunt hammer and I'm always dreading side-effects.
This also feels a *lot* like implicit-this which is handled in another way.

So that seems worth a shot, but this approach is fine if that one doesn't work.

clang-tools-extra/clangd/Selection.cpp

443

seems like it'd be more consistent to handle it here?

If it's a MemberExpr and the member is in an anonymous struct, then it's implicit if isImplicit(ME.getBase()).

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

368

This example is quite complicated to the point where it's hard to see what it's testing... The example you have in the comment is much clearer.

If we also want to test named fields with inline struct types, maybe that should be a second test...

[kadircet]

clang-tools-extra/clangd/Selection.cpp

443

the test case was actually to demonstrate why we can't do it here. we want to still keep traversing the AST after hitting a field inside an anon struct. handling here terminates the traversal for that subtree completely, and the real node we are interested might be down the tree (y.[[x]] in the test case).

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

368

it is actually important to demonstrate we are not claiming the whole range at the first encounter (and to make sure traversal is not stopped for any other reason).

[sammccall]

clang-tools-extra/clangd/Selection.cpp

443

That's what I meant by isImplicit(ME.getBase())

The following passes tests (added to isImplicit)...

if (auto *ME = llvm::dyn_cast<MemberExpr>(S)) 
  if (auto *FD = llvm::dyn_cast<FieldDecl>(ME->getMemberDecl()))
    if (FD->isAnonymousStructOrUnion())
      return isImplicit(ME->getBase());

and the member is in an anonymous struct

Oops, I meant "and the member *is* an anonymous struct".
It seems more natural to recognize and special-case the field-access to the anon struct itself (which is intuitively implicit!), than the one to a normal field within it.

[sammccall]

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

368

Makes sense, but without comment it's not clear.
Can we make the latter a separate test case, and simplify it too? This seems to be enough:

struct [[Foo]] {
  struct { int x; }
};
int a = ^Foo{}.x;

Here the Foo{} is the non-empty base of the anon member access that must be traversed.

[kadircet]

• Use isImplicit rather than earlyClaim

[kadircet]

clang-tools-extra/clangd/Selection.cpp

443

right, it makes sense. i was missing the isImplicit(base) bit + got stunned by the beautiful shape of the AST. thanks for bearing with me :)

[sammccall]

Thanks!

clang-tools-extra/clangd/Selection.cpp

446

hmm, "implicit accesses of anonymous structs"?

450

The double negatives and references to a "traversal" here seem confusing.
Maybe something more concrete?

// If Base is an implicit CXXThis, then the whole MemberExpr has no tokens.
// If it's a normal e.g. DeclRef, we treat the MemberExpr like an implicit cast.

[kadircet]

• update comments

clang-tools-extra/clangd/Selection.cpp

 bool isImplicit(const Stmt *S) {
   // Some Stmts are implicit and shouldn't be traversed, but there's no
   // "implicit" attribute on Stmt/Expr.
   // Unwrap implicit casts first if present (other nodes too?).
   if (auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(S))
     S = ICE->getSubExprAsWritten();
   // Implicit this in a MemberExpr is not filtered out by RecursiveASTVisitor.
   // It would be nice if RAV handled this (!shouldTraverseImplicitCode()).
   if (auto *CTI = llvm::dyn_cast<CXXThisExpr>(S))
     if (CTI->isImplicit())
       return true;
+  // Make sure anonymous structs don't end up owning tokens.
+  if (auto *ME = llvm::dyn_cast<MemberExpr>(S)) {
+    if (auto *FD = llvm::dyn_cast<FieldDecl>(ME->getMemberDecl()))
+      if (FD->isAnonymousStructOrUnion())
+        // If the base is not implicit, it can own tokens, otherwise we should
+        // stop traversal.
+        return isImplicit(ME->getBase());
+  }
   // Refs to operator() and [] are (almost?) always implicit as part of calls.
   if (auto *DRE = llvm::dyn_cast<DeclRefExpr>(S)) {
     if (auto *FD = llvm::dyn_cast<FunctionDecl>(DRE->getDecl())) {
       switch (FD->getOverloadedOperator()) {
       case OO_Call:
       case OO_Subscript:
         return true;
       default:
#endif
   }
 
   // Perform hit-testing of a complete Node against the selection.
   // This runs for every node in the AST, and must be fast in common cases.
   // This is usually called from pop(), so we can take children into account.
   // The existing state of Result is relevant (early/late claims can interact).
   void claimRange(SourceRange S, SelectionTree::Selection &Result) {
     for (const auto &ClaimedRange :
          UnclaimedExpandedTokens.erase(TokenBuf.expandedTokens(S)))
       update(Result, SelChecker.test(ClaimedRange));
 
     if (Result && Result != NoTokens)
       dlog("{1}hit selection: {0}", S.printToString(SM), indent());
   }
 
   std::string indent(int Offset = 0) {
     // Cast for signed arithmetic.

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

Test = Annotations(R"cpp(// definition in AST.
         class [[Forward]] {};
         F^orward create();
       )cpp");
   EXPECT_THAT(
       LocateWithIndex(Test),
       ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range())));
 }
 
+TEST(LocateSymbol, AnonymousStructFields) {
+  auto Code = Annotations(R"cpp(
+    struct $2[[Foo]] {
+      struct { int $1[[x]]; };
+      void foo() {
+        // Make sure the implicit base is skipped.
+        $1^x = 42;
+      }
+    };
+    // Check that we don't skip explicit bases.
+    int a = $2^Foo{}.x;
+  )cpp");
+  TestTU TU = TestTU::withCode(Code.code());
+  auto AST = TU.build();
+  EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()),
+              UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"))));
+  EXPECT_THAT(
+      locateSymbolAt(AST, Code.point("2"), TU.index().get()),
+      UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"))));
+}
+
 TEST(LocateSymbol, FindOverrides) {
   auto Code = Annotations(R"cpp(
     class Foo {
       virtual void $1[[fo^o]]() = 0;
     };
     class Bar : public Foo {
       void $2[[foo]]() override;
     };