[clangd] Go-to-definition on 'override' jumps to overridden method(s)

[merge_guards_bot]

Unit tests: fail. 62170 tests passed, 5 failed and 813 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 1 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

please fix clang-format and tidy warnings

clang-tools-extra/clangd/XRefs.cpp

287

when do we have OverrideAttribute but no overriden_methods ? It looks like

struct Foo { virtual void foo(); };
struct Bar {
  void foo() override;
  void bar() override;
};

Bar::bar doesn't seem to have OverrideAttr set for example:

|-CXXMethodDecl 0x8e57e88 <line:6:3, col:14> col:8 bar 'void ()'
|-CXXMethodDecl 0x8e57fa0 <line:7:3, col:14> col:8 foo 'void ()'
| |-Overrides: [ 0x8e57668 Foo::foo 'void ()' ]
  | `-OverrideAttr 0x8e58040 <col:14>

[sammccall]

address comment and clang-format

[sammccall]

(the clang-tidy check doesn't reflect a style guideline and I'm not sure it's a good idea, raised on D72217 which added it)

clang-tools-extra/clangd/XRefs.cpp

287

templated code: if foo<t>::bar() overrides base<t>::bar() then the list is empty.
But I think returning "no definition available" is fine, so removed this.

[merge_guards_bot]

Unit tests: pass. 62255 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 1 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

Thanks, LGTM

clang-tools-extra/clangd/XRefs.cpp

 #include "Selection.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "index/Relation.h"
 #include "index/SymbolLocation.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
   if (auto L = sourceLocationInMainFile(SM, Pos)) {
     SourceLoc = *L;
   } else {
     elog("locateSymbolAt failed to convert position to source location: {0}",
          L.takeError());
     return Result;
   }
 
-  // Emit all symbol locations (declaration or definition) from AST.
-  DeclRelationSet Relations =
-      DeclRelation::TemplatePattern | DeclRelation::Alias;
-  for (const NamedDecl *D : getDeclAtPosition(AST, SourceLoc, Relations)) {
+  auto AddResultDecl = [&](const NamedDecl *D) {
     const NamedDecl *Def = getDefinition(D);
     const NamedDecl *Preferred = Def ? Def : D;
 
-    // If we're at the point of declaration of a template specialization,
-    // it's more useful to navigate to the template declaration.
-    if (SM.getMacroArgExpandedLocation(Preferred->getLocation()) ==
-        IdentStartLoc) {
-      if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Preferred)) {
-        D = CTSD->getSpecializedTemplate();
-        Def = getDefinition(D);
-        Preferred = Def ? Def : D;
-      }
-    }
-
     auto Loc = makeLocation(AST.getASTContext(), nameLocation(*Preferred, SM),
                             *MainFilePath);
     if (!Loc)
-      continue;
+      return;
 
     Result.emplace_back();
-    Result.back().Name = printName(AST.getASTContext(), *D);
+    Result.back().Name = printName(AST.getASTContext(), *Preferred);
     Result.back().PreferredDeclaration = *Loc;
     // Preferred is always a definition if possible, so this check works.
     if (Def == Preferred)
       Result.back().Definition = *Loc;
 
     // Record SymbolID for index lookup later.
     if (auto ID = getSymbolID(Preferred))
       ResultIndex[*ID] = Result.size() - 1;
+  };
+
+  // Emit all symbol locations (declaration or definition) from AST.
+  DeclRelationSet Relations =
+      DeclRelation::TemplatePattern | DeclRelation::Alias;
+  for (const NamedDecl *D : getDeclAtPosition(AST, SourceLoc, Relations)) {
+    // Special case: void foo() ^override: jump to the overridden method.
+    if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
+      const auto *Attr = D->getAttr<OverrideAttr>();
+      const syntax::Token *Tok =
+          spelledIdentifierTouching(SourceLoc, AST.getTokens());
+      if (Attr && Tok &&
+          SM.getSpellingLoc(Attr->getLocation()) == Tok->location()) {
+        bool FoundOverride = false;
+        // We may be overridding multiple methods - offer them all.
+        for (const NamedDecl* ND : CMD->overridden_methods()) {
+          AddResultDecl(ND);
+          FoundOverride = true;
+        }
+        // Only return this decl if there's no overridden method (broken code).
+        if (FoundOverride)
+          continue;
+      }
+    }
+
+    // Special case: the point of declaration of a template specialization,
+    // it's more useful to navigate to the template declaration.
+    if (SM.getMacroArgExpandedLocation(D->getLocation()) ==
+        IdentStartLoc) {
+      if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
+        AddResultDecl(CTSD->getSpecializedTemplate());
+        continue;
+      }
+    }
+
+    // Otherwise the target declaration is the right one.
+    AddResultDecl(D);
   }
 
   // Now query the index for all Symbol IDs we found in the AST.
   if (Index && !ResultIndex.empty()) {
     LookupRequest QueryRequest;
     for (auto It : ResultIndex)
       QueryRequest.IDs.insert(It.first);
     std::string Scratch;

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

const char *Tests[] = {
       R"cpp(// Declaration of partial template specialization
         template <typename T>
         struct $decl[[Foo]] {};
 
         template <typename T>
         struct Fo^o<T*> {};
       )cpp",
 
+      R"cpp(// Override specifier jumps to overridden method
+        class Y { virtual void $decl[[a]]() = 0; };
+        class X : Y { void a() ^override {} };
+      )cpp",
+
       R"cpp(// Heuristic resolution of dependent method
         template <typename T>
         struct S {
           void [[bar]]() {}
         };
 
         template <typename T>
         void foo(S<T> arg) {