[clangd] Correct SelectionTree behavior around anonymous field access.

[sammccall]

Summary

struct A { struct { int b; }; };
A().^b;

This should be considered a reference to b, but currently it's
considered a reference to the anonymous struct field.

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

[hokein]

clang-tools-extra/clangd/Selection.cpp

71

I still think it is worth to explore this direction (will take a look on this if I have time), but also ok with the current approach.

[sammccall]

clang-tools-extra/clangd/Selection.cpp

71

SG, this isn't urgent so I'll leave this open while out on vacation. I agree it's suspect/suggestive that we need to override the AST source range in exactly one case.

My concern was conceptual: in A().b if you want to describe where the anon field ref happens, I'd pick . or b so placing those outside the range is suspect. Conversely a range of A() seems weird because that's a perfectly valid expression with no dereferencing.

OTOH if you squint I guess this looks a bit like an implicit cast from the A to the anon member. So maybe it's fine and just a question of mental model.

clang-tools-extra/clangd/Selection.cpp

if (const auto *RE = N->ASTNode.get<RecoveryExpr>()) {
       RecoveryType.record(RE->isTypeDependent() ? 0 : 1, LanguageLabel);
       return;
     }
   }
   if (Common)
     SelectionUsedRecovery.record(0, LanguageLabel); // unused.
 }
 
+SourceRange getSourceRange(const DynTypedNode &N) {
+  // MemberExprs to implicitly access anonymous fields should not claim any
+  // tokens for themselves. Given:
+  //   struct A { struct { int b; }; };
+  // The clang AST reports the following nodes for an access to b:
+  //   A().b;
+  //   [----] MemberExpr, base = A().<anonymous>, member = b
+  //   [----] MemberExpr: base = A(), member = <anonymous>
+  //   [-]    CXXConstructExpr
+  // For our purposes, we don't want the second MemberExpr to own any tokens,
+  // so we reduce its range to match the CXXConstructExpr.
+  // (It's not clear that changing the clang AST would be correct in general).
+  if (const auto *ME = N.get<MemberExpr>()) {
+    if (!ME->getMemberDecl()->getDeclName())
+      return ME->getBase()
+                 ? getSourceRange(DynTypedNode::create(*ME->getBase()))
+                 : SourceRange();
+  }
+  return N.getSourceRange();
+}
+
 // An IntervalSet maintains a set of disjoint subranges of an array.
 //
 // Initially, it contains the entire array.
 //           [-----------------------------------------------------------]
 //
 // When a range is erased(), it will typically split the array in two.
 //  Claim:                     [--------------------]
 //  after:   [----------------]                      [-------------------]
#endif
   //                    ~~~~~~~~ y
   // Here the first ("leftmost") sibling claims the tokens it wants, and the
   // other sibling gets what's left. So selecting "int" only includes the left
   // VarDecl in the selection tree.
 
   // An optimization for a common case: nodes outside macro expansions that
   // don't intersect the selection may be recursively skipped.
   bool canSafelySkipNode(const DynTypedNode &N) {
-    SourceRange S = N.getSourceRange();
+    SourceRange S = getSourceRange(N);
     if (auto *TL = N.get<TypeLoc>()) {
       // FIXME: TypeLoc::getBeginLoc()/getEndLoc() are pretty fragile
       // heuristics. We should consider only pruning critical TypeLoc nodes, to
       // be more robust.
 
       // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to
       // failing
       // to descend into the child expression.
void push(DynTypedNode Node) {
     claimRange(Early, Nodes.back().Selected);
   }
 
   // Pops a node off the ancestor stack, and finalizes it. Pairs with push().
   // Performs primary hit detection.
   void pop() {
     Node &N = *Stack.top();
     dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy), indent(-1));
-    claimRange(N.ASTNode.getSourceRange(), N.Selected);
+    claimRange(getSourceRange(N.ASTNode), N.Selected);
     if (N.Selected == NoTokens)
       N.Selected = SelectionTree::Unselected;
     if (N.Selected || !N.Children.empty()) {
       // Attach to the tree.
       N.Parent->Children.push_back(&N);
     } else {
       // Neither N any children are selected, it doesn't belong in the tree.
       assert(&N == &Nodes.back());
if (const Decl *Current = CurrentNode->ASTNode.get<Decl>()) {
       return *Current->getDeclContext();
     }
   }
   llvm_unreachable("A tree must always be rooted at TranslationUnitDecl.");
 }
 
 const SelectionTree::Node &SelectionTree::Node::ignoreImplicit() const {
   if (Children.size() == 1 &&
-      Children.front()->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+      getSourceRange(Children.front()->ASTNode) == getSourceRange(ASTNode))
     return Children.front()->ignoreImplicit();
   return *this;
 }
 
 const SelectionTree::Node &SelectionTree::Node::outerImplicit() const {
-  if (Parent && Parent->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+  if (Parent && getSourceRange(Parent->ASTNode) == getSourceRange(ASTNode))
     return Parent->outerImplicit();
   return *this;
 }
 
 } // namespace clangd
 } // namespace clang

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

 }
 
 TEST(LocateSymbol, All) {
   // Ranges in tests:
   //   $decl is the declaration location (if absent, no symbol is located)
   //   $def is the definition location (if absent, symbol has no definition)
   //   unnamed range becomes both $decl and $def.
   const char *Tests[] = {
+      R"cpp(
+        struct X {
+          union {
+            int [[a]];
+            float b;
+          };
+        };
+        int test(X &x) {
+          return x.^a;
+        }
+      )cpp",
+
       R"cpp(// Local variable
         int main() {
           int [[bonjour]];
           ^bonjour = 2;
           int test1 = bonjour;
         }
       )cpp",