[clangd] Selection handles CXXBaseSpecifier

[njames93]

Summary

Selection now includes the virtual and access modifier as part of their range for cxx base specifiers.

[njames93]

Update FindTargets to fix symbol renaming on a CXXBaseSpecifier.
Add test to ensure rename wont trigger on a CXXBaseSpecifier.

[njames93]

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

272–283

Is this good behaviour, or should Foo selection resolve as a RecordTypeLoc, That is the behaviour previously observed for these 2 cases. And instead have CXXBaseSpecifier as its parent selection?

[sammccall]

clang-tools-extra/clangd/Selection.cpp

496

can we not use traverseNode here, rather than repeating?

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

272–283

Yes, this should be a typeloc with the CXXBaseSpecifier as its parent, unless there's a really compelling reason to make an exception.

Two main reasons:

• it matches RecursiveASTVisitor's idea of the tree shape, which people mostly understand. If we diverge, it'll be a special case we need to remember and so potentially a source of bugs.
• This allows code that handles types generically to work with fewer special cases. (I do like the FindTarget change though - it seems reasonable to allow go-to-def on e.g. the public keyword) It still preserves flexibility if we want to handle types as base-specifiers specially (we can examine the parent)

clang/lib/AST/ASTTypeTraits.cpp

196

nice, thanks!
We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a simple pattern to follow for sourcerange)

[njames93]

clang-tools-extra/clangd/Selection.cpp

496

Yeah, especially now I'm changing it to actually traverse :)

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

272–283

OK, funnily enough that change to find target was to fix renaming when the recordtypeloc was removed from the selection. That change will be redundant if i fix that, but do you still want it in here?

clang/lib/AST/ASTTypeTraits.cpp

196

Not really trivial as there is no matcher for cxxBaseSpecifier. Annoyingly its my fault there isn't one.

[njames93]

Update behaviour to keep RecordTypeLoc seletion.

[sammccall]

(I don't see a new snapshot but LG with the traverseNode change)

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

272–283

Up to you, but I'd lean towards keeping it and adding a FindTarget test for struct S : ^private T or so, which is the visible change.

clang/lib/AST/ASTTypeTraits.cpp

196

Oh, I've been down this rabbithole recently... clangd support for attrs -> DynTypedNode support for Attr -> matchers for use in tests -> matchers get complicated...

Let's not block on this (and I need to revive my attr patch that got too big...)

[njames93]

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

272–283

I think I should remove it, then add it back in a follow up. It was needed to fix a issue in here that was then resolved in here.

[njames93]

Remove FindTarget to instead add in a follow up patch.
Side note, Think I'll wait til after tomorrow to land this, unsure if the changes will break anything elsewhere.

clang-tools-extra/clangd/Selection.cpp

public:
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc X) {
     return traverseNode(
         &X, [&] { return Base::TraverseNestedNameSpecifierLoc(X); });
   }
   bool TraverseConstructorInitializer(CXXCtorInitializer *X) {
     return traverseNode(
         X, [&] { return Base::TraverseConstructorInitializer(X); });
   }
+  bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &X) {
+    return traverseNode(&X, [&] { return Base::TraverseCXXBaseSpecifier(X); });
+  }
   // Stmt is the same, but this form allows the data recursion optimization.
   bool dataTraverseStmtPre(Stmt *X) {
     if (!X || isImplicit(X))
       return false;
     auto N = DynTypedNode::create(*X);
     if (canSafelySkipNode(N))
       return false;
     push(std::move(N));

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

Case Cases[] = {
        nullptr, !HeaderFile, nullptr, "Conflict"},
 
       {R"cpp(// Trying to rename into the same name, SameName == SameName.
         void func() {
           int S^ameName;
         }
       )cpp",
        "new name is the same", !HeaderFile, nullptr, "SameName"},
+      {R"cpp(// Ensure it doesn't associate base specifier with base name.
+        struct A {};
+        struct B : priv^ate A {};
+      )cpp",
+       "Cannot rename symbol: there is no symbol at the given location", false,
+       nullptr},
   };
 
   for (const auto& Case : Cases) {
     SCOPED_TRACE(Case.Code);
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.HeaderCode = CommonHeader;
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");

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

Case Cases[] = {
       },
       {
           R"cpp(
             auto lambda = [](const char*){ return 0; };
             int x = lambda([["y^"]]);
           )cpp",
           "StringLiteral", // Not DeclRefExpr to operator()!
       },
+      {
+          R"cpp(
+            struct Foo {};
+            struct Bar : [[v^ir^tual private Foo]] {};
+          )cpp",
+          "CXXBaseSpecifier",
+      },
+      {
+          R"cpp(
+            struct Foo {};
+            struct Bar : private [[Fo^o]] {};
+          )cpp",
+          "RecordTypeLoc",
+      },
+      {
+          R"cpp(
+            struct Foo {};
+            struct Bar : [[Fo^o]] {};
+          )cpp",
+          "RecordTypeLoc",
+      },
 
       // Point selections.
       {"void foo() { [[^foo]](); }", "DeclRefExpr"},
       {"void foo() { [[f^oo]](); }", "DeclRefExpr"},
       {"void foo() { [[fo^o]](); }", "DeclRefExpr"},
       {"void foo() { [[foo^()]]; }", "CallExpr"},
       {"void foo() { [[foo^]] (); }", "DeclRefExpr"},
       {"int bar; void foo() [[{ foo (); }]]^", "CompoundStmt"},

clang/lib/AST/ASTTypeTraits.cpp

SourceRange DynTypedNode::getSourceRange() const {
   if (const Decl *D = get<Decl>())
     return D->getSourceRange();
   if (const Stmt *S = get<Stmt>())
     return S->getSourceRange();
   if (const TemplateArgumentLoc *TAL = get<TemplateArgumentLoc>())
     return TAL->getSourceRange();
   if (const auto *C = get<OMPClause>())
     return SourceRange(C->getBeginLoc(), C->getEndLoc());
+  if (const auto *CBS = get<CXXBaseSpecifier>())
+    return CBS->getSourceRange();
   return SourceRange();
 }