[clangd] Fix selection on multi-dimensional array.

[sammccall]

Summary

This involves separating out the concepts of "which tokens should we
descend into this node for" vs "which tokens should this node claim".

While here, move decltype() hack into a more appropriate place.

based on D116218

[sammccall]

cleaner, comments

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.
 }
 
+// Return the range covering a node and all its children.
 SourceRange getSourceRange(const DynTypedNode &N) {
+  // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to
+  // failing
+  // to descend into the child expression.
+  // decltype(2+2);
+  // ~~~~~~~~~~~~~ <-- correct range
+  // ~~~~~~~~      <-- range reported by getSourceRange()
+  // ~~~~~~~~~~~~  <-- range with this hack(i.e, missing closing paren)
+  // FIXME: Alter DecltypeTypeLoc to contain parentheses locations and get
+  // rid of this patch.
+  if (const auto *TL = N.get<TypeLoc>()) {
+    if (auto DT = TL->getAs<DecltypeTypeLoc>()) {
+      SourceRange S = DT.getSourceRange();
+      S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+      return S;
+    }
+  }
   // 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
#endif
   // don't intersect the selection may be recursively skipped.
   bool canSafelySkipNode(const DynTypedNode &N) {
     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.
-      // decltype(2+2);
-      // ~~~~~~~~~~~~~ <-- correct range
-      // ~~~~~~~~      <-- range reported by getSourceRange()
-      // ~~~~~~~~~~~~  <-- range with this hack(i.e, missing closing paren)
-      // FIXME: Alter DecltypeTypeLoc to contain parentheses locations and get
-      // rid of this patch.
-      if (auto DT = TL->getAs<DecltypeTypeLoc>())
-        S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
       // AttributedTypeLoc may point to the attribute's range, NOT the modified
       // type's range.
       if (auto AT = TL->getAs<AttributedTypeLoc>())
         S = AT.getModifiedLoc().getSourceRange();
     }
     // SourceRange often doesn't manage to accurately cover attributes.
     // Fortunately, attributes are rare.
     if (llvm::any_of(getAttributes(N),
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(getSourceRange(N.ASTNode), N.Selected);
+    claimTokensFor(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 *D = N.get<Decl>()) {
         return DD->getLocation();
     } else if (const auto *CCI = N.get<CXXCtorInitializer>()) {
       // : [[b_]](42)
       return CCI->getMemberLocation();
     }
     return SourceRange();
   }
 
+  // Claim tokens for N, after processing its children.
+  // By default this claims all unclaimed tokens in getSourceRange().
+  // We override this if we want to claim fewer tokens (e.g. there are gaps).
+  void claimTokensFor(const DynTypedNode &N, SelectionTree::Selection &Result) {
+    if (const auto *TL = N.get<TypeLoc>()) {
+      // e.g. EltType Foo[OuterSize][InnerSize];
+      //      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ArrayType (Outer)
+      //      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |-ArrayType (Inner)
+      //      ~~~~~~~                           | |-RecordType
+      //                             ~~~~~~~~~  | `-Expr
+      //                  ~~~~~~~~~             `-Expr
+      // Inner must not claim its whole SourceRange, or it clobbers Outer.
+      if (TL->getAs<ArrayTypeLoc>()) {
+        claimRange(TL->getLocalSourceRange(), Result);
+        return;
+      }
+      // FIXME: maybe LocalSourceRange is a better default for TypeLocs.
+      // It doesn't seem to be usable for FunctionTypeLocs.
+    }
+    claimRange(getSourceRange(N), Result);
+  }
+
   // 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));

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

Case Cases[] = {
             };
           )cpp",
           "CXXCtorInitializer", // Not the CXXConstructExpr!
       },
       // Tricky case: anonymous struct is a sibling of the VarDecl.
       {"[[st^ruct {int x;}]] y;", "CXXRecordDecl"},
       {"[[struct {int x;} ^y]];", "VarDecl"},
       {"struct {[[int ^x]];} y;", "FieldDecl"},
+
+      // Tricky case: nested ConstantArrayTypeLocs have the same token range.
+      {"const int x = 1, y = 2; int array[^[[x]]][10][y];", "DeclRefExpr"},
+      {"const int x = 1, y = 2; int array[x][10][^[[y]]];", "DeclRefExpr"},
+      {"const int x = 1, y = 2; int array[x][^[[10]]][y];", "IntegerLiteral"},
+      {"const int x = 1, y = 2; [[i^nt]] array[x][10][y];", "BuiltinTypeLoc"},
+
       // FIXME: the AST has no location info for qualifiers.
       {"const [[a^uto]] x = 42;", "AutoTypeLoc"},
       {"[[co^nst auto x = 42]];", "VarDecl"},
 
       {"^", nullptr},
       {"void foo() { [[foo^^]] (); }", "DeclRefExpr"},
 
       // FIXME: Ideally we'd get a declstmt or the VarDecl itself here.