[clangd] Use the localSourceRange as the default claimed range for typelocs.

[hokein]

Summary

It also fixes a bug where SelectionTree doesn't select the right AST node on
int (*Fun(OuterT^ype))(InnerType);.

[hokein]

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

234

these two new behavior changes seem to be more appropriate.

[sammccall]

This is neat, and I we're getting closer to the principled answer.
After staring at this patch for a while, and seeing what arrays, functions, and now parens have in common, I finally understand. The problem is *declarators*.

Declarators are inside-out: ((*x)[](double) says that x is a pointer to an array to a function from double.
Whereas types that are decl-specifiers nest the other way: that would be written pointer<array<function<double>>>, just like decls or stmts nest.

So for decl-specifier TypeLocs the usual getSourceRange should apply. (Modulo hacks like decltype() where the range is just wrong).

For typelocs that are part of the declarator, there are two approaches that seem principled:

• claim their full range, but in the opposite order (parents before children)
• claim only the specific tokens used, in which case order doesn't matter

These are basically equivalent. declarator types have a "before" and an "after" chunk (see TypePrinter). They can claim these by claiming their full range, but the "gap" was claimed earlier, or they can claim the two ends separately.
If we can make it work, claiming the full range is probably better because it will handle unexpected stray tokens in a more natural way.
Either way we should be able to unify this with the DeclaratorDecl hack.

I'll have a play with this idea...

[sammccall]

In D116618#3220744, @sammccall wrote:

• claim their full range, but in the opposite order (parents before children)

To refine this a little more, for declarator type nodes:

• first, we should recurse into non-declarator children (such as array bounds and function parameters) and let them claim tokens
• then we claim the range
• then we recurse into declarator children

That said I think it's probably not worth it, there are only a few declarator nodes and the approach of not claiming the gap seems simpler.

[sammccall]

OK, I hacked up this and your other patch into https://reviews.llvm.org/D116630.

The TypeLoc part seems a little more principled there, because:

• most types are treated the same as any other nodes
• the types that are treated differently have a precise reason, and we can be confident the list is exhaustive

Feel free to do what you like with that patch - review it, adopt it, discard it.

clang-tools-extra/clangd/Selection.cpp

SourceRange earlySourceRange(const DynTypedNode &N) {
     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];
+      // We need the DecltypeTypeLoc hack in getSourceRange.
+      if (TL->getAs<DecltypeTypeLoc>())
+        return claimRange(getSourceRange(N), Result);
+
+      // E.g. a function which returns a function pointer.
+      // int (*Fun(OuterType))(InnerType);
+      // ~~~~~~~~~XXXXXXXXXXX~~~~~~~~~~~~ FunctionProtoTypeLoc
+      // ~~~~~X~~~~~~~~~~~~~~~~~~~~~~~~~~ |- PointerTypeLoc: int (*)(InnerType)
+      // ~~~~XXXXXXXXXXXXXXXXX~~~~~~~~~~~ |  `- ParenTypeLoc: int (InnerType)
+      // ~~~~~~~~~~~~~~~~~~~~~XXXXXXXXXXX |     `-FunctionProtoTypeLoc: int (InnerType)
+      //           ~~~~~~~~~              `- ParmVarDecl: OuterType
+      // The ParenTypeLoc must not claim its local source range (marked as XXX
+      // above), or it clobbers the outer ParmVarDecl.
+      if ( auto PTL = TL->getAs<ParenTypeLoc>()) {
+        PTL.getLocalSourceRange().dump(SM);
+        // Only claim the parentheses tokens, other tokens are handled by its
+        // children or are expected unclaimed.
+        claimRange(PTL.getLParenLoc(), Result);
+        claimRange(PTL.getRParenLoc(), Result);
+        return;
+      }
+
+      // We use LocalSourceRange as a default claimed range for TypeLocs -- it
+      // is better than getSourceRange (which returns a range covering itself
+      // and its children), because we want to claim fewer tokens (multiple
+      // discontiguous ranges) for some TypeLocs.
+      // E.g. EltType Foo[OuterSize][InnerSize];
       //      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ArrayTypeLoc (Outer)
       //      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |-ArrayTypeLoc (Inner)
       //      ~~~~~~~                           | |-RecordType
       //                             ~~~~~~~~~  | `-Expr (InnerSize)
       //                  ~~~~~~~~~             `-Expr (OuterSize)
       // Inner ATL 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.
+      return claimRange(TL->getLocalSourceRange(), Result);
     }
     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).

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

Case Cases[] = {
             [[void (*S)^(int)]] = nullptr;
           )cpp",
           "FunctionProtoTypeLoc",
       },
       {
           R"cpp(
             [[void (^*S)(int)]] = nullptr;
           )cpp",
-          "FunctionProtoTypeLoc",
+          "PointerTypeLoc",
       },
       {
           R"cpp(
             [[void (*^S)(int) = nullptr]];
           )cpp",
           "VarDecl",
       },
       {
           R"cpp(
             [[void ^(*S)(int)]] = nullptr;
           )cpp",
-          "FunctionProtoTypeLoc",
+          "ParenTypeLoc",
       },
       {
           R"cpp(
             struct S {
               int foo() const;
               int bar() { return [[f^oo]](); }
             };
           )cpp",
Case Cases[] = {
 
       // Tricky case: nested ArrayTypeLocs 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"},
       {"void func(int x) { int v_array[^[[x]]][10]; }", "DeclRefExpr"},
 
+      {"int (*getFunc([[do^uble]]))(int);", "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.