[clangd] Fix a case where we fail to detect a header-declared symbol in rename.

[hokein]

Summary

Failing case:

#include "foo.h"
void fo^o() {}

getRenameDecl() returns the decl of the symbol under the cursor (which is
in the current main file), instead, we use the canonical decl to determine
whether a symbol is declared in #included header.

[sammccall]

clang-tools-extra/clangd/refactor/Rename.cpp

157 ↗

(On Diff #206833)

This cast isn't safe. I can't remember the exact case, but if you really need a NamedDecl you need to decide what to do if the canonical one isn't.

[hokein]

Address comment.

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

157 ↗

(On Diff #206833)

Thanks, it is a little surprising this is not safe (I don't have a corner case in mind either).

The shouldCollectSymbol requires a NamedDecl, I think when the CanonicalDecl is not a NamedDecl, we think this symbol is not indexable.

[sammccall]

clang-tools-extra/clangd/refactor/Rename.cpp

157 ↗

(On Diff #206833)

Sounds good.

See the comment in SymbolCollector about ObjCPropertyDecl.

clang-tools-extra/trunk/clangd/refactor/Rename.cpp

llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
   if (!U) {
     elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError());
     return None;
   }
   return U->file().str();
 }
 
 // Query the index to find some other files where the Decl is referenced.
-llvm::Optional<std::string> getOtherRefFile(const NamedDecl &Decl,
-                                            StringRef MainFile,
+llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
                                             const SymbolIndex &Index) {
   RefsRequest Req;
   // We limit the number of results, this is a correctness/performance
   // tradeoff. We expect the number of symbol references in the current file
   // is smaller than the limit.
   Req.Limit = 100;
-  if (auto ID = getSymbolID(&Decl))
+  if (auto ID = getSymbolID(&D))
     Req.IDs.insert(*ID);
   llvm::Optional<std::string> OtherFile;
   Index.refs(Req, [&](const Ref &R) {
     if (OtherFile)
       return;
     if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
       if (*RefFilePath != MainFile)
         OtherFile = *RefFilePath;
     }
   });
   return OtherFile;
 }
 
 enum ReasonToReject {
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
   UnsupportedSymbol,
 };
 
 // Check the symbol Decl is renameable (per the index) within the file.
-llvm::Optional<ReasonToReject> renamableWithinFile(const NamedDecl &Decl,
+llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
                                                    StringRef MainFile,
                                                    const SymbolIndex *Index) {
-  if (llvm::isa<NamespaceDecl>(&Decl))
+  if (llvm::isa<NamespaceDecl>(&RenameDecl))
     return ReasonToReject::UnsupportedSymbol;
-  auto &ASTCtx = Decl.getASTContext();
+  auto &ASTCtx = RenameDecl.getASTContext();
   const auto &SM = ASTCtx.getSourceManager();
   bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
   bool DeclaredInMainFile =
-      SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation()));
+      SM.isWrittenInMainFile(SM.getExpansionLoc(RenameDecl.getLocation()));
 
   // If the symbol is declared in the main file (which is not a header), we
   // rename it.
   if (DeclaredInMainFile && !MainFileIsHeader)
     return None;
 
   // Below are cases where the symbol is declared in the header.
   // If the symbol is function-local, we rename it.
-  if (Decl.getParentFunctionOrMethod())
+  if (RenameDecl.getParentFunctionOrMethod())
     return None;
 
   if (!Index)
     return ReasonToReject::NoIndexProvided;
 
-  bool IsIndexable =
-      SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false);
+  bool IsIndexable = isa<NamedDecl>(RenameDecl) &&
+                     SymbolCollector::shouldCollectSymbol(
+                         cast<NamedDecl>(RenameDecl), ASTCtx, {}, false);
   // If the symbol is not indexable, we disallow rename.
   if (!IsIndexable)
     return ReasonToReject::NonIndexable;
-  auto OtherFile = getOtherRefFile(Decl, MainFile, *Index);
+  auto OtherFile = getOtherRefFile(RenameDecl, MainFile, *Index);
   // If the symbol is indexable and has no refs from other files in the index,
   // we rename it.
   if (!OtherFile)
     return None;
   // If the symbol is indexable and has refs from other files in the index,
   // we disallow rename.
   return ReasonToReject::UsedOutsideFile;
 }
renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
   Context.setASTContext(ASTCtx);
   auto Rename = clang::tooling::RenameOccurrences::initiate(
       Context, SourceRange(SourceLocationBeg), NewName);
   if (!Rename)
     return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics());
 
   const auto *RenameDecl = Rename->getRenameDecl();
   assert(RenameDecl && "symbol must be found at this point");
-  if (auto Reject = renamableWithinFile(*RenameDecl, File, Index)) {
+  if (auto Reject =
+          renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index)) {
     auto Message = [](ReasonToReject Reason) {
       switch (Reason) {
       case NoIndexProvided:
         return "symbol may be used in other files (no index available)";
       case UsedOutsideFile:
         return "the symbol is used outside main file";
       case NonIndexable:
         return "symbol may be used in other files (not eligible for indexing)";

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

const char* After;
               }
             }
           )cpp",
       },
   };
   for (const Test &T : Tests) {
     Annotations Code(T.Before);
     auto TU = TestTU::withCode(Code.code());
-    TU.HeaderCode = "void foo();"; // outside main file, will not be touched.
     auto AST = TU.build();
     auto RenameResult =
         renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
     ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
     auto ApplyResult =
         tooling::applyAllReplacements(Code.code(), *RenameResult);
     ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
     EXPECT_EQ(T.After, *ApplyResult) << T.Before;
   }
 }
 
 TEST(RenameTest, Renameable) {
-  // Test cases where the symbol is declared in header.
   struct Case {
-    const char* HeaderCode;
+    const char *Code;
     const char* ErrorMessage; // null if no error
+    bool IsHeaderFile;
   };
+  const bool HeaderFile = true;
   Case Cases[] = {
       {R"cpp(// allow -- function-local
         void f(int [[Lo^cal]]) {
           [[Local]] = 2;
         }
       )cpp",
-       nullptr},
+       nullptr, HeaderFile},
 
       {R"cpp(// allow -- symbol is indexable and has no refs in index.
         void [[On^lyInThisFile]]();
       )cpp",
-       nullptr},
+       nullptr, HeaderFile},
 
       {R"cpp(// disallow -- symbol is indexable and has other refs in index.
         void f() {
           Out^side s;
         }
       )cpp",
-       "used outside main file"},
+       "used outside main file", HeaderFile},
 
       {R"cpp(// disallow -- symbol is not indexable.
         namespace {
         class Unin^dexable {};
         }
       )cpp",
-       "not eligible for indexing"},
+       "not eligible for indexing", HeaderFile},
 
       {R"cpp(// disallow -- namespace symbol isn't supported
         namespace fo^o {}
       )cpp",
-       "not a supported kind"},
+       "not a supported kind", HeaderFile},
+
+      {R"cpp(// foo is declared outside the file.
+        void fo^o() {}
+      )cpp", "used outside main file", !HeaderFile/*cc file*/},
   };
-  const char *CommonHeader = "class Outside {};";
-  TestTU OtherFile = TestTU::withCode("Outside s;");
+  const char *CommonHeader = R"cpp(
+    class Outside {};
+    void foo();
+  )cpp";
+  TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
   OtherFile.HeaderCode = CommonHeader;
   OtherFile.Filename = "other.cc";
-  // The index has a "Outside" reference.
+  // The index has a "Outside" reference and a "foo" reference.
   auto OtherFileIndex = OtherFile.index();
 
   for (const auto& Case : Cases) {
-    Annotations T(Case.HeaderCode);
-    // We open the .h file as the main file.
+    Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
-    TU.Filename = "test.h";
     TU.HeaderCode = CommonHeader;
+    if (Case.IsHeaderFile) {
+      // We open the .h file as the main file.
+      TU.Filename = "test.h";
       // Parsing the .h file as C++ include.
       TU.ExtraArgs.push_back("-xobjective-c++-header");
+    }
     auto AST = TU.build();
 
     auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
                                     "dummyNewName", OtherFileIndex.get());
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;
     if (!WantRename) {