Fixes clangd/clangd#1797

clang-tools-extra/clangd/HeaderSourceSwitch.cpp

@@ -82,26 +82,39 @@ std::optional<Path> getCorrespondingHeaderOrSource(PathRef OriginalFile,
       Request.IDs.insert(ID);
   }
   llvm::StringMap<int> Candidates; // Target path => score.
-  auto AwardTarget = [&](const char *TargetURI) {
-    if (auto TargetPath = URI::resolve(TargetURI, OriginalFile)) {
-      if (!pathEqual(*TargetPath, OriginalFile)) // exclude the original file.
-        ++Candidates[*TargetPath];
-    } else {
-      elog("Failed to resolve URI {0}: {1}", TargetURI, TargetPath.takeError());
-    }
-  };
-  // If we switch from a header, we are looking for the implementation
-  // file, so we use the definition loc; otherwise we look for the header file,
-  // we use the decl loc;
+  // When in the implementation file, we always search for the header file,
+  // using the decl loc. When we are in a header, this usually implies searching
+  // for implementation, for which we use the definition loc. For templates, we
+  // can have separate implementation headers, which behave as an implementation
+  // file. As such, we always have to add the decl loc and conditionally
+  // definition loc.
   //
   // For each symbol in the original file, we get its target location (decl or
   // def) from the index, then award that target file.
-  const bool IsHeader = isHeaderFile(OriginalFile, AST.getLangOpts());
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+  auto pathEqual = [](llvm::StringRef l, llvm::StringRef r) {
+    return l.equals_insensitive(r);
+  };
+#else
+  auto pathEqual = [](llvm::StringRef l, llvm::StringRef r) { return l == r; };
+#endif
   Index->lookup(Request, [&](const Symbol &Sym) {
-    if (IsHeader)
-      AwardTarget(Sym.Definition.FileURI);
-    else
-      AwardTarget(Sym.CanonicalDeclaration.FileURI);
+    if (llvm::StringRef{Sym.Definition.FileURI}.empty() ||
+        llvm::StringRef{Sym.CanonicalDeclaration.FileURI}.empty())
+      return;
+    auto TargetPathDefinition =
+        URI::resolve(Sym.Definition.FileURI, OriginalFile);
+    if (!TargetPathDefinition)
+      return;
+    auto TargetPathDeclaration =
+        URI::resolve(Sym.CanonicalDeclaration.FileURI, OriginalFile);
+    if (!TargetPathDeclaration)
+      return;
+    if (pathEqual(*TargetPathDefinition, OriginalFile)) {
+      if (!pathEqual(*TargetPathDeclaration, OriginalFile))
+        ++Candidates[*TargetPathDeclaration];
+    } else if (pathEqual(*TargetPathDeclaration, OriginalFile))
+      ++Candidates[*TargetPathDefinition];
   });
   // FIXME: our index doesn't have any interesting information (this could be
   // that the background-index is not finished), we should use the decl/def
@@ -139,6 +152,11 @@ std::vector<const Decl *> getIndexableLocalDecls(ParsedAST &AST) {
         for (auto *D : Scope->decls())
           TraverseDecl(D);
       }
+      // ClassTemplateDecl does not inherit from DeclContext
+      if (auto *Scope = llvm::dyn_cast<ClassTemplateDecl>(ND)) {
+        for (auto *D : Scope->getTemplatedDecl()->decls())
+          TraverseDecl(D);
+      }
     }
     if (llvm::isa<NamespaceDecl>(D))
       return; // namespace is indexable, but we're not interested.

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

@@ -223,6 +223,204 @@ TEST(HeaderSourceSwitchTest, FromHeaderToSource) {
   }
 }
 
+TEST(HeaderSourceSwitchTest, FromHeaderToImplHeader) {
+  // build a proper index, which contains symbols:
+  //   A_Sym1, declared in TestTU.h, defined in a_ext.h
+  //   B_Sym[1-2], declared in TestTU.h, defined in b_ext.h
+  SymbolSlab::Builder AllSymbols;
+  auto addHeader = [&](auto implName, auto declarationContent,
+                       auto implContent) {
+    TestTU Testing;
+    Testing.ExtraArgs.push_back(
+        "-xc++-header"); // inform clang this is a header.
+
+    Testing.Filename = implName;
+    Testing.Code = implContent;
+    Testing.HeaderFilename = "TestTU.h";
+    Testing.HeaderCode = declarationContent;
+
+    for (auto &Sym : Testing.headerSymbols())
+      AllSymbols.insert(Sym);
+  };
+
+  addHeader("a_ext.h", "template<typename T> void A_Sym1();",
+            "template<typename T> void A_Sym1() {};");
+  addHeader("b_ext.h", R"cpp(
+  template<typename T> void B_Sym1();
+  template<typename T> void B_Sym2();
+  template<typename T> void B_Sym3_NoDef();
+  )cpp",
+            R"cpp(
+  template<typename T> void B_Sym1() {}
+  template<typename T> void B_Sym2() {}
+  )cpp");
+  auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
+
+  // Test for switch from .h header to .cc source
+  struct {
+    llvm::StringRef HeaderCode;
+    std::optional<std::string> ExpectedSource;
+  } TestCases[] = {
+      {"// empty, no header found", std::nullopt},
+      {R"cpp(
+         // no definition found in the index.
+         template<typename T> void NonDefinition();
+       )cpp",
+       std::nullopt},
+      {R"cpp(
+         template<typename T> void A_Sym1();
+       )cpp",
+       testPath("a_ext.h")},
+      {R"cpp(
+         // b_ext.h wins.
+         template<typename T> void A_Sym1();
+         template<typename T> void B_Sym1();
+         template<typename T> void B_Sym2();
+       )cpp",
+       testPath("b_ext.h")},
+      {R"cpp(
+         // a_ext.h and b_ext.h have same scope, but a_ext.h because "a_ext.h" < "b_ext.h".
+         template<typename T> void A_Sym1();
+         template<typename T> void B_Sym1();
+       )cpp",
+       testPath("a_ext.h")},
+
+      {R"cpp(
+          // We don't have definition in the index, so stay in the header.
+          template<typename T> void B_Sym3_NoDef();
+       )cpp",
+       std::nullopt},
+  };
+  for (const auto &Case : TestCases) {
+    TestTU TU = TestTU::withCode(Case.HeaderCode);
+    TU.Filename = "TestTU.h";
+    TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
+    auto HeaderAST = TU.build();
+    EXPECT_EQ(Case.ExpectedSource,
+              getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST,
+                                             Index.get()));
+  }
+}
+
+TEST(HeaderSourceSwitchTest, FromImplHeaderToHeader) {
+  // build a proper index, which contains symbols:
+  //   A_Sym1, declared in TestTU.h, defined in a_ext.h
+  //   B_Sym[1-2], declared in TestTU.h, defined in b_ext.h
+  SymbolSlab::Builder AllSymbols;
+  auto addHeader = [&](auto declName, auto declContent, auto implContent) {
+    TestTU Testing;
+    Testing.ExtraArgs.push_back(
+        "-xc++-header"); // inform clang this is a header.
+
+    Testing.Filename = "TestTU.h";
+    Testing.Code = implContent;
+    Testing.HeaderFilename = declName;
+    Testing.HeaderCode = declContent;
+
+    for (auto &Sym : Testing.headerSymbols())
+      AllSymbols.insert(Sym);
+  };
+
+  addHeader("a.h", "template<typename T> void A_Sym1();",
+            "template<typename T> void A_Sym1() {};");
+  addHeader("b.h", R"cpp(
+  template<typename T> void B_Sym1();
+  template<typename T> void B_Sym2();
+  template<typename T> void B_Sym3_NoDef();
+  )cpp",
+            R"cpp(
+  template<typename T> void B_Sym1() {}
+  template<typename T> void B_Sym2() {}
+  )cpp");
+  auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
+
+  // Test for switch from .h header to .cc source
+  struct {
+    llvm::StringRef HeaderCode;
+    std::optional<std::string> ExpectedSource;
+  } TestCases[] = {
+      {"// empty, no header found", std::nullopt},
+      {R"cpp(
+         // no definition found in the index.
+         template<typename T> void NonDefinition(){}
+       )cpp",
+       std::nullopt},
+      {R"cpp(
+         template<typename T> void A_Sym1(){}
+       )cpp",
+       testPath("a.h")},
+      {R"cpp(
+         // b.h wins.
+         template<typename T> void A_Sym1(){}
+         template<typename T> void B_Sym1(){}
+         template<typename T> void B_Sym2(){}
+       )cpp",
+       testPath("b.h")},
+      {R"cpp(
+         // a.h and b.h have same scope, but a.h because "a.h" < "b.h".
+         template<typename T> void A_Sym1(){}
+         template<typename T> void B_Sym1(){}
+       )cpp",
+       testPath("a.h")},
+
+      {R"cpp(
+          // We don't have definition in the index, so stay in the header.
+          template<typename T> void B_Sym3_NoDef();
+       )cpp",
+       std::nullopt},
+  };
+  for (const auto &Case : TestCases) {
+    TestTU TU = TestTU::withCode(Case.HeaderCode);
+    TU.Filename = "TestTU.h";
+    TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
+    auto HeaderAST = TU.build();
+    EXPECT_EQ(Case.ExpectedSource,
+              getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST,
+                                             Index.get()));
+  }
+}
+
+TEST(HeaderSourceSwitchTest, FromHeaderToImplHeaderClassTemplate) {
+  // build a proper index, which contains symbols:
+  //   A_Sym1, declared in TestTU.h, defined in a_ext.h
+  //   B_Sym[1-2], declared in TestTU.h, defined in b_ext.h
+  SymbolSlab::Builder AllSymbols;
+  TestTU Testing;
+  Testing.Filename = "a.cpp";
+  Testing.Code = R"cpp(void f() {}
+    void g(){})cpp";
+  Testing.HeaderFilename = "TestTU.h";
+  Testing.HeaderCode = R"cpp(void f();
+    void g();
+    template<typename T> struct S
+    {
+      void a();
+      void b();
+      void c();
+    };)cpp";
+  for (auto &Sym : Testing.headerSymbols())
+    AllSymbols.insert(Sym);
+
+  Testing.Filename = "b_ext.h";
+  Testing.Code =
+      R"cpp(template<typename T> void S<T>::a(){}
+    template<typename T> void S<T>::b(){}
+    template<typename T> void S<T>::c(){})cpp";
+  Testing.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
+
+  for (auto &Sym : Testing.headerSymbols())
+    AllSymbols.insert(Sym);
+  auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
+
+  TestTU TU = TestTU::withCode(Testing.HeaderCode);
+  TU.Filename = "TestTU.h";
+  TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
+  auto HeaderAST = TU.build();
+  EXPECT_EQ(testPath("b_ext.h"),
+            getCorrespondingHeaderOrSource(testPath(TU.Filename), HeaderAST,
+                                           Index.get()));
+}
+
 TEST(HeaderSourceSwitchTest, FromSourceToHeader) {
   // build a proper index, which contains symbols:
   //   A_Sym1, declared in a.h, defined in TestTU.cpp
@@ -281,7 +479,7 @@ TEST(HeaderSourceSwitchTest, FromSourceToHeader) {
   };
   for (const auto &Case : TestCases) {
     TestTU TU = TestTU::withCode(Case.SourceCode);
-    TU.Filename = "Test.cpp";
+    TU.Filename = "TestTU.cpp";
     auto AST = TU.build();
     EXPECT_EQ(Case.ExpectedResult,
               getCorrespondingHeaderOrSource(testPath(TU.Filename), AST,
@@ -337,14 +535,14 @@ TEST(HeaderSourceSwitchTest, CaseSensitivity) {
   // We expect the heuristics to pick:
   // - header on case sensitive file systems, because the HeaderAbsPath doesn't
   //   match what we've seen through index.
-  // - source on case insensitive file systems, as the HeaderAbsPath would match
-  //   the filename in index.
+  // - nullopt on case insensitive file systems, as the HeaderAbsPath doesn't
+  //   match the filename in index.
 #ifdef CLANGD_PATH_CASE_INSENSITIVE
   EXPECT_THAT(getCorrespondingHeaderOrSource(HeaderAbsPath, AST, Index.get()),
               llvm::ValueIs(testing::StrCaseEq(testPath(TU.Filename))));
 #else
-  EXPECT_THAT(getCorrespondingHeaderOrSource(HeaderAbsPath, AST, Index.get()),
-              llvm::ValueIs(testing::StrCaseEq(testPath(TU.HeaderFilename))));
+  EXPECT_EQ(std::optional<std::string>{},
+            getCorrespondingHeaderOrSource(HeaderAbsPath, AST, Index.get()));
 #endif
 }