[clangd] Consider () part of the FunctionDecl, not the FunctionTypeLoc

[sammccall]

Summary

CODE: int foo(int);
OLD: TTT FFFTTTTT
NEW: TTT FFFFTTTF

This works around targeting issues when the cursor is between name and (

[merge_guards_bot]

Build result: pass - 60682 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[kadircet]

clang-tools-extra/clangd/Selection.cpp

578

comment needs to be updated

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

253

could you also add case for closing paren (possibly with a trailing const attribute ?

[sammccall]

D71345 is a different approach that would solve this problem and others (at higher cost).
I don't think we should land both, so let's park this until we decide on the fate of that patch.

[kadircet]

I believe this revision is obsolete since D71345 has landed.

clang-tools-extra/clangd/Selection.cpp

bool shouldSkipChildren(const Stmt *X) const {
     // Unfortunately TokenBuffer sees 12_i as one token and can't split it.
     // So we treat UserDefinedLiteral as a leaf node, owning the token.
     return llvm::isa<UserDefinedLiteral>(X);
   }
 
   // Pushes a node onto the ancestor stack. Pairs with pop().
   // Performs early hit detection for some nodes (on the earlySourceRange).
   void push(DynTypedNode Node) {
-    SourceRange Early = earlySourceRange(Node);
     dlog("{1}push: {0}", printNodeToString(Node, PrintPolicy), indent());
     Nodes.emplace_back();
     Nodes.back().ASTNode = std::move(Node);
     Nodes.back().Parent = Stack.top();
     Nodes.back().Selected = NoTokens;
     Stack.push(&Nodes.back());
-    claimRange(Early, Nodes.back().Selected);
+    claimEarlyRange(Node, 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);
     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());
       Nodes.pop_back();
     }
     Stack.pop();
   }
 
   // Returns the range of tokens that this node will claim directly, and
   // is not available to the node's children.
   // Usually empty, but sometimes children cover tokens but shouldn't own them.
-  SourceRange earlySourceRange(const DynTypedNode &N) {
+  void claimEarlyRange(const DynTypedNode &N,
+                       SelectionTree::Selection &Selected) {
+    auto Claim = [&](SourceRange R) { claimRange(R, Selected); };
     if (const Decl *D = N.get<Decl>()) {
+      if (auto *FD = llvm::dyn_cast<FunctionDecl>(D)) {
         // void [[foo]]();
-      if (auto *FD = llvm::dyn_cast<FunctionDecl>(D))
-        return FD->getNameInfo().getSourceRange();
+        Claim(FD->getNameInfo().getSourceRange());
+        // Also claim the parens. Formally, the FunctionTypeLoc is more precise,
+        // but it's not a very "useful" node (e.g. we can't find its target).
+        // Associating the parens with the function is what users expect.
+        Claim(FD->getFunctionTypeLoc().getLParenLoc());
+        Claim(FD->getFunctionTypeLoc().getRParenLoc());
+      }
       // int (*[[s]])();
       else if (auto *VD = llvm::dyn_cast<VarDecl>(D))
-        return VD->getLocation();
+        Claim(VD->getLocation());
     } else if (const auto* CCI = N.get<CXXCtorInitializer>()) {
       // : [[b_]](42)
-      return CCI->getMemberLocation();
+      Claim(CCI->getMemberLocation());
     }
-    return SourceRange();
   }
 
   // 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 :

clang-tools-extra/clangd/test/hover.test

 # CHECK-NEXT:      "start": {
 # CHECK-NEXT:        "character": 25,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      }
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":10}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":0}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": null
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main2.cpp","languageId":"cpp","version":1,"text":"enum foo{}; int main() { foo f; }\n"}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main2.cpp"},"position":{"line":0,"character":27}}}
 #      CHECK:  "id": 1,

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

Case Cases[] = {
       {"void foo() { [[foo^]] (); }", "DeclRefExpr"},
       {"int bar; void foo() [[{ foo (); }]]^", "CompoundStmt"},
       {"int x = [[42]]^;", "IntegerLiteral"},
 
       // Ignores whitespace, comments, and semicolons in the selection.
       {"void foo() { [[foo^()]]; /*comment*/^}", "CallExpr"},
 
       // Tricky case: FunctionTypeLoc in FunctionDecl has a hole in it.
-      {"[[^void]] foo();", "BuiltinTypeLoc"},
-      {"[[void foo^()]];", "FunctionProtoTypeLoc"},
-      {"[[^void foo^()]];", "FunctionDecl"},
-      {"[[void ^foo()]];", "FunctionDecl"},
+      {"[[^void]] foo(int);", "BuiltinTypeLoc"},
+      {"[[void foo^(int)]];", "FunctionDecl"},
+      {"[[^void foo^(int)]];", "FunctionDecl"},
+      {"[[void ^foo(int)]];", "FunctionDecl"},
       // Tricky case: two VarDecls share a specifier.
       {"[[int ^a]], b;", "VarDecl"},
       {"[[int a, ^b]];", "VarDecl"},
       // Tricky case: CXXConstructExpr wants to claim the whole init range.
       {
           R"cpp(
             class X { X(int); };
             class Y {

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

[[void [[f^o^o]]() [[{
     return;
   }]]]]
   )cpp");
 
   EXPECT_UNAVAILABLE(R"cpp(
   // Not a definition
   vo^i[[d^ ^f]]^oo();
 
-  [[vo^id ]]foo[[()]] {[[
+  [[vo^id ]]foo() {[[
     [[(void)(5+3);
     return;]]
   }]]
 
   // Definition with no body.
   class Bar { Bar() = def^ault; }
   )cpp");
 }
EXPECT_UNAVAILABLE(R"cpp(
     }]]]])cpp");
 
   FileName = "Test.hpp";
   // Not available unless function name or fully body is selected.
   EXPECT_UNAVAILABLE(R"cpp(
     // Not a definition
     vo^i[[d^ ^f]]^oo();
 
-    [[vo^id ]]foo[[()]] {[[
+    [[vo^id ]]foo() {[[
       [[(void)(5+3);
       return;]]
     }]])cpp");
 
   // Available even if there are no implementation files.
   EXPECT_AVAILABLE(R"cpp(
     [[void [[f^o^o]]() [[{
       return;