[clangd] Narrow rename to local symbols.

Summary:
Previously, we performed rename for all kinds of symbols (local, global).

This patch narrows the scope by only renaming symbols not being used
outside of the main file (with index asisitance). Renaming global
symbols is not supported at the moment (return an error).

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63426

llvm-svn: 364283

clang-tools-extra/clangd/ClangdServer.cpp

@@ -277,12 +277,12 @@ ClangdServer::formatOnType(llvm::StringRef Code, PathRef File, Position Pos,
 
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
                           Callback<std::vector<TextEdit>> CB) {
-  auto Action = [Pos](Path File, std::string NewName,
-                      Callback<std::vector<TextEdit>> CB,
-                      llvm::Expected<InputsAndAST> InpAST) {
+  auto Action = [Pos, this](Path File, std::string NewName,
+                            Callback<std::vector<TextEdit>> CB,
+                            llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
-    auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName);
+    auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
     if (!Changes)
       return CB(Changes.takeError());
 

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

@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "refactor/Rename.h"
+#include "AST.h"
+#include "Logger.h"
+#include "index/SymbolCollector.h"
 #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 
@@ -46,11 +49,95 @@ llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
   return Err;
 }
 
+llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
+                                     llvm::StringRef HintFilePath) {
+  if (!Loc)
+    return None;
+  auto Uri = URI::parse(Loc.FileURI);
+  if (!Uri) {
+    elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
+    return None;
+  }
+  auto U = URIForFile::fromURI(*Uri, HintFilePath);
+  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,
+                                            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))
+    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,
+};
+
+// Check the symbol Decl is renameable (per the index) within the file.
+llvm::Optional<ReasonToReject> renamableWithinFile(const NamedDecl &Decl,
+                                                   StringRef MainFile,
+                                                   const SymbolIndex *Index) {
+  auto &ASTCtx = Decl.getASTContext();
+  const auto &SM = ASTCtx.getSourceManager();
+  bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
+  bool DeclaredInMainFile =
+      SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.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())
+    return None;
+
+  if (!Index)
+    return ReasonToReject::NoIndexProvided;
+
+  bool IsIndexable =
+      SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false);
+  // If the symbol is not indexable, we disallow rename.
+  if (!IsIndexable)
+    return ReasonToReject::NonIndexable;
+  auto OtherFile = getOtherRefFile(Decl, 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;
+}
+
 } // namespace
 
 llvm::Expected<tooling::Replacements>
 renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
-                 llvm::StringRef NewName) {
+                 llvm::StringRef NewName, const SymbolIndex *Index) {
   RefactoringResultCollector ResultCollector;
   ASTContext &ASTCtx = AST.getASTContext();
   SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier(
@@ -62,6 +149,25 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
   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)) {
+    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)";
+      }
+      llvm_unreachable("unhandled reason kind");
+    };
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("Cannot rename symbol: {0}", Message(*Reject)),
+        llvm::inconvertibleErrorCode());
+  }
+
   Rename->invoke(ResultCollector, Context);
 
   assert(ResultCollector.Result.hasValue());

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

@@ -18,10 +18,11 @@ namespace clangd {
 
 /// Renames all occurrences of the symbol at \p Pos to \p NewName.
 /// Occurrences outside the current file are not modified.
-llvm::Expected<tooling::Replacements> renameWithinFile(ParsedAST &AST,
-                                                       llvm::StringRef File,
-                                                       Position Pos,
-                                                       llvm::StringRef NewName);
+/// Returns an error if rename a symbol that's used in another file (per the
+/// index).
+llvm::Expected<tooling::Replacements>
+renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
+                 llvm::StringRef NewName, const SymbolIndex *Index = nullptr);
 
 } // namespace clangd
 } // namespace clang

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

@@ -18,6 +18,10 @@ namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(RenameRange, Code, Range, "") {
+  return replacementToEdit(Code, arg).range == Range;
+}
+
 TEST(RenameTest, SingleFile) {
   struct Test {
     const char* Before;
@@ -87,6 +91,67 @@ TEST(RenameTest, SingleFile) {
   }
 }
 
+TEST(RenameTest, Renameable) {
+  // Test cases where the symbol is declared in header.
+  const char *Headers[] = {
+      R"cpp(// allow -- function-local
+        void f(int [[Lo^cal]]) {
+          [[Local]] = 2;
+        }
+      )cpp",
+
+      R"cpp(// allow -- symbol is indexable and has no refs in index.
+        void [[On^lyInThisFile]]();
+      )cpp",
+
+      R"cpp(// disallow -- symbol is indexable and has other refs in index.
+        void f() {
+          Out^side s;
+        }
+      )cpp",
+
+      R"cpp(// disallow -- symbol is not indexable.
+        namespace {
+        class Unin^dexable {};
+        }
+      )cpp",
+  };
+  const char *CommonHeader = "class Outside {};";
+  TestTU OtherFile = TestTU::withCode("Outside s;");
+  OtherFile.HeaderCode = CommonHeader;
+  OtherFile.Filename = "other.cc";
+  // The index has a "Outside" reference.
+  auto OtherFileIndex = OtherFile.index();
+
+  for (const char *Header : Headers) {
+    Annotations T(Header);
+    // We open the .h file as the main file.
+    TestTU TU = TestTU::withCode(T.code());
+    TU.Filename = "test.h";
+    TU.HeaderCode = CommonHeader;
+    TU.ExtraArgs.push_back("-xc++");
+    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) {
+      EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
+                            << T.code();
+      llvm::consumeError(Results.takeError());
+    } else {
+      EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+                                 << llvm::toString(Results.takeError());
+      std::vector<testing::Matcher<tooling::Replacement>> Expected;
+      for (const auto &R : T.ranges())
+        Expected.push_back(RenameRange(TU.Code, R));
+      EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
+    }
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h

@@ -54,6 +54,8 @@ class RenameOccurrences final : public SourceChangeRefactoringRule {
 
   static const RefactoringDescriptor &describe();
 
+  const NamedDecl *getRenameDecl() const;
+
 private:
   RenameOccurrences(const NamedDecl *ND, std::string NewName)
       : ND(ND), NewName(std::move(NewName)) {}

clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp

@@ -74,6 +74,8 @@ RenameOccurrences::initiate(RefactoringRuleContext &Context,
                            std::move(NewName));
 }
 
+const NamedDecl *RenameOccurrences::getRenameDecl() const { return ND; }
+
 Expected<AtomicChanges>
 RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   Expected<SymbolOccurrences> Occurrences = findSymbolOccurrences(ND, Context);