[clangd] Fix selection on multi-dimensional array.

[sammccall]

Ooh, this is messy indeed. Thanks for digging!

clang-tools-extra/clangd/Selection.cpp

526

I don't think this is a complete solution: won't the inner ArrayTypeLoc still end up owning both sets of brackets?

I think maybe a better solution is making getSourceRange(ArrayTypeLoc) return only ATL.getBracketRange(), and then modify canSafelySkipNode to to avoid pruning based on this small range.

This is vaguely similar to how DeclTypeLoc works today (though in that case the reduced range is the one reported by the AST).

527

ConstantArrayType isn't the only kind of array, see the other subclasses of ArrayType

[hokein]

clang-tools-extra/clangd/Selection.cpp

526

won't the inner ArrayTypeLoc still end up owning both sets of brackets?

yes, unfortunately, the inner ATL still owns both sets of brackets, that means go-to-def (etc) won't work on an overloaded subscript operator [], I don't have a better solution to fix that. I guess it is ok to live with that as it might be a rare case in practice.

I think maybe a better solution is making getSourceRange(ArrayTypeLoc) return only ATL.getBracketRange(), and then modify canSafelySkipNode to to avoid pruning based on this small range.

yeah, indeed I tried this approach before coming up the current approach, and I didn't find a satisfied solution (BracketRange is not enough, otherwise we will lose the array type int).

// int array[Size][100];
// ~~~      [-2 -]~~~~~  ConstantArrayTypeLoc int[Size][100]
// [1]            [-3-]  |-ConstantArrayTypeLoc int[100]

Ideally, the inner ATL should own tokens int ([1]) and tokens [100] ([3]). And in the canSafelySkipNode, we want to skip the inner ATL if the cursor is in the source range of array[Size], it seems quite tricky to get the source range (the range of int [1] in particular). Of course we could use some lexer hacks or call ATL.getElementLoc() recursively until we hit a non-array type loc (it can easily go up to O(n^2) for super-multi-dimensional arrays).

527

oh, right. There are three others, we could do similar for them. Will do that if we agree on the current approach.

[hokein]

refine the patch based on D116536, and one more test.

clang-tools-extra/clangd/Selection.cpp

bool TraverseDecl(Decl *X) {
     // Base::TraverseDecl will suppress children, but not this node itself.
     if (X && X->isImplicit())
       return true;
     return traverseNode(X, [&] { return Base::TraverseDecl(X); });
   }
   bool TraverseTypeLoc(TypeLoc X) {
     return traverseNode(&X, [&] { return Base::TraverseTypeLoc(X); });
   }
+
+  // Multi-dimensional arrays are tricky:
+  //
+  // int array[Size][10];
+  // ~~~      ~~~~~~~~~~  ConstantArrayTypeLoc int[Size][10]
+  // ~~~            ~~~~  |-ConstantArrayTypeLoc int[10]
+  //
+  // We have two nested ConstantArrayTypeLocs, both claim the same token range.
+  // We want to the SizeExpr in the first ConstantArrayTypeLoc to own the "Size"
+  // token rather than the second ConstantArrayTypeLoc.
+  // We override the default ConstantArrayTypeLoc traversal behavior by swaping
+  // the traversal order of SizeExpr and ElementTypeLoc, which gives a chance
+  // for the SizeExpr to claim its tokens.
+  bool TraverseConstantArrayTypeLoc(ConstantArrayTypeLoc X) {
+    if (!Base::TraverseStmt(X.getSizeExpr()))
+      return false;
+    return TraverseTypeLoc(X.getElementLoc());
+  }
   bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &X) {
     return traverseNode(&X,
                         [&] { return Base::TraverseTemplateArgumentLoc(X); });
   }
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc X) {
     return traverseNode(
         &X, [&] { return Base::TraverseNestedNameSpecifierLoc(X); });
   }

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.