[clangd] Fix early selection for non-vardecl declarators

[kadircet]

Summary

Selection tree was performing an early claim only for VarDecls, but
there are other cases where we can have declarators, e.g. FieldDecls. This patch
extends the early claim logic to all types of declarators.

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

[sammccall]

clang-tools-extra/clangd/Selection.cpp

651–653

I think we can combine these two cases.
The weird cases handled by FunctionDecl are actually a bit wrong at the moment (early claiming types).

• conversion operator: we should probably be claiming operator only, which hapepns to be getLocation()
• destructor: we should be claiming ~ only, which is also getLocation()
• constructor: we shouldn't be claiming anything, need to blacklist
• all the others seem fine to be handled by the DeclaratorDecl case

This changes more behaviour (and the changes are likely to be seen as regressions in some cases, though more consistent) so we could also consider just landing this as-is.

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

380

I'd suggest using digraphs instead, if it works:

int hash<:32:>

[kadircet]

• Address comments

[sammccall]

clang-tools-extra/clangd/Selection.cpp

644

we may have to blacklist deduction guides too?

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

378

can you add the other special cases we discussed offline? conversion operations should be triggered by operator, destructors by ~.

[kadircet]

• Address comments

[sammccall]

clang-tools-extra/clangd/Selection.cpp

641

can you check this at HEAD?
IIRC this was related to the reason kythe was failing to report constructors as references to the class, which is now fixed. But I don't know if they fixed the AST or worked around it.

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

342

want to keep the conversion operator test too? (by pointing at operator)

[kadircet]

• Add the old test for hover over operator back.

clang-tools-extra/clangd/Selection.cpp

641

nope, seems like still broken.

clang-tools-extra/clangd/Selection.cpp

 //===--- Selection.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Selection.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
void pop() {
     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) {
     if (const Decl *D = N.get<Decl>()) {
+      // We want constructor name to be claimed by TypeLoc not the constructor
+      // itself. Similar for deduction guides, we rather want to select the
+      // underlying TypeLoc.
+      // FIXME: Unfortunately this doesn't work, even though RecursiveASTVisitor
+      // traverses the underlying TypeLoc inside DeclarationName, it is null for
+      // constructors.
+      if (isa<CXXConstructorDecl>(D) || isa<CXXDeductionGuideDecl>(D))
+        return SourceRange();
+      // This will capture Field, Function, MSProperty, NonTypeTemplateParm and
+      // VarDecls. We want the name in the declarator to be claimed by the decl
+      // and not by any children. For example:
       // void [[foo]]();
-      if (auto *FD = llvm::dyn_cast<FunctionDecl>(D))
-        return FD->getNameInfo().getSourceRange();
       // int (*[[s]])();
-      else if (auto *VD = llvm::dyn_cast<VarDecl>(D))
-        return VD->getLocation();
+      // struct X { int [[hash]] [32]; [[operator]] int();}
+      if (const auto *DD = llvm::dyn_cast<DeclaratorDecl>(D))
+        return DD->getLocation();
     } else if (const auto *CCI = N.get<CXXCtorInitializer>()) {
       // : [[b_]](42)
       return CCI->getMemberLocation();
     }
     return SourceRange();
   }
 
   // Perform hit-testing of a complete Node against the selection.

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

class Foo {})cpp";
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "~X";
          HI.LocalScope = "X::";
          HI.Kind = index::SymbolKind::Destructor;
          HI.Definition = "~X()";
          HI.Parameters.emplace();
        }},
-      {"class X { operator [[in^t]](); };",
+      {"class X { [[op^erator]] int(); };",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "operator int";
          HI.LocalScope = "X::";
          HI.Kind = index::SymbolKind::ConversionFunction;
          HI.Definition = "operator int()";
          HI.Parameters.emplace();
        }},
+      {"class X { operator [[^X]](); };",
+       [](HoverInfo &HI) {
+         HI.NamespaceScope = "";
+         HI.Name = "X";
+         HI.Kind = index::SymbolKind::Class;
+         HI.Definition = "class X {}";
+       }},
 
       // auto on lambda
       {R"cpp(
         void foo() {
           [[au^to]] lamb = []{};
         }
         )cpp",
        [](HoverInfo &HI) {

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

Case Cases[] = {
       {
           R"cpp(
             @interface I{}
             -(int)foo;
             @end
             int test(I *f) { return 42 + [[f.^foo]]; }
           )cpp",
           "ObjCPropertyRefExpr"},
+      {"struct foo { [[int has^h<:32:>]]; };", "FieldDecl"},
+      {"struct foo { [[op^erator int()]]; };", "CXXConversionDecl"},
+      {"struct foo { [[^~foo()]]; };", "CXXDestructorDecl"},
+      // FIXME: The following to should be class itself instead.
+      {"struct foo { [[fo^o(){}]] };", "CXXConstructorDecl"},
   };
   for (const Case &C : Cases) {
     Annotations Test(C.Code);
 
     TestTU TU;
     TU.Code = std::string(Test.code());
 
     // FIXME: Auto-completion in a template requires disabling delayed template