[clangd] Narrow rename to local symbols.

[hokein]

Summary

Previously, we perform 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).

[sammccall]

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

147

pull out a function for this?

151

it's unfortunate that our "what's under the cursor" logic here has to duplicate what's done by RenameOccurrences::initiate().

Can we have RenameOccurrences expose the NamedDecl instead?

151

does this not work for macros?

153

We're effectively doing this test twice (see if (!Rename) below) and emitting different error mesages

maybe we should just skip the rest of the check in this case?
(Exposing the ND from the RenameOccurrences would help make this duplication more obvious)

[hokein]

Address review comments

[hokein]

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

151

unfortunately, the rename library doesn't rename marcos.

153

The duplication doesn't exist if we expose the ND from the RenameOccurrence.

[sammccall]

Thanks for exposing the ND, cleaner.
Found another problem :-( not in your code but about this whole approach to using the index.
Let's talk tomorrow

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

82

What about cases where the symbol has a USR but is not indexed? Non-toplevel, template bits we ignore, etc?

Not sure what we should do honestly. The "real" solution is probably to use QNames instead of symbol id here, and index then all. Silently allowing the rename seems pretty bad and needs at least to be explicit

155

This would be fixed in the helper function, move the fixme there

[hokein]

Switch to a hybrid solution (language semantic + index) to detect "symbol local".

[hokein]

Remove the unrelated changes.

[hokein]

Use the hybrid approach based on our discussion, the implementation should be fine to review, I'm still working on adding more tests.

[hokein]

Improve the tests.

[sammccall]

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

70

...to find some other file...

99

Can we move these steps next to the associated code? The separate lists can be hard to maintain.

e.g.

// If the symbol is in the main file (which is not a header), we can rename it
if (DeclaredInMainFile && !MainFileIsHeader)
  ...

100

(which is not a header)

108

renamableWithinFile

111

this isn't the right check - LangOpts::IsHeaderFile

116

can we avoid the double negative here? maybe just inline into the if

130

I can't parse "not support for now" - not sure this comment is necessary, the whole point of this function (with its current signature) is to reject this.

168

this is a user-visible error message, right?

• "reject to rename" doesn't have any obvious meaning to me. Maybe "Cannot rename symbol" ?
• "no index is provided" -> "symbol may be used in other files (no index available)"
• "the symbol is not indexable" -> "symbol may be used in other files (not eligible for indexing)"

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

21–24

It's uncler what "only support" means here.
"Renaming a symbol that's used in another file (per the index) returns an error"

[hokein]

Address review comments.

[hokein]

More cleanups.

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::formatOnType(llvm::StringRef Code, PathRef File, Position Pos,
   for (const tooling::Replacement &R :
        formatIncremental(Code, *CursorPos, TriggerText, *Style))
     Result.push_back(replacementToEdit(Code, R));
   return Result;
 }
 
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
                           Callback<std::vector<TextEdit>> CB) {
-  auto Action = [Pos](Path File, std::string NewName,
+  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());
     std::vector<TextEdit> Edits;
     for (const auto &Rep : *Changes)
       Edits.push_back(replacementToEdit(InpAST->Inputs.Contents, Rep));
     return CB(std::move(Edits));
   };
 

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

 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 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);
+/// Only support renaming symbols not being used outside the file.
+llvm::Expected<tooling::Replacements>
+renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
+                 llvm::StringRef NewName, const SymbolIndex *Index = nullptr);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H

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

 //===--- Rename.cpp - Symbol-rename refactorings -----------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 class RefactoringResultCollector final
     : public tooling::RefactoringResultConsumer {
 public:
if (auto Diag = DiagnosticError::take(Err)) {
     SmallVector<char, 128> DiagMessage;
     Diag->second.EmitToString(DE, DiagMessage);
     return llvm::make_error<llvm::StringError>(DiagMessage,
                                                llvm::inconvertibleErrorCode());
   }
   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 get other file 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, details are:
+//   1. symbol declared in the main file (not a header) => allow
+//   2. symbol declared in the header
+//      1) symbol is function-local => allow
+//      2) symbol is not indexable => disallow
+//      3) symbol is indexable and has no refs in index => allow
+//      4) symbol is indexable and has refs from other files => disallow (for
+//         now)
+llvm::Optional<ReasonToReject>
+renamable(const NamedDecl &Decl, StringRef MainFile, const SymbolIndex *Index) {
+  auto &ASTCtx = Decl.getASTContext();
+  const auto &SM = ASTCtx.getSourceManager();
+  bool MainFileIsHeader = llvm::sys::path::extension(MainFile) == ".h";
+  bool DeclaredInMainFile =
+      SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation()));
+
+  bool DeclaredInHeader =
+      !DeclaredInMainFile || (DeclaredInMainFile && MainFileIsHeader);
+  if (!DeclaredInHeader)
+    return None; // Case 1
+
+  if (Decl.getParentFunctionOrMethod())
+    return None; // Case 2.1: function-local
+
+  if (!Index)
+    return ReasonToReject::NoIndexProvided;
+
+  bool IsIndexable =
+      SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false);
+  if (!IsIndexable)
+    return ReasonToReject::NonIndexable; // Case 2.2
+  // query index for xrefs. (not support for now)
+  auto OtherFile = getOtherRefFile(Decl, MainFile, *Index);
+  if (!OtherFile)
+    return None; // Case 2.3
+  return ReasonToReject::UsedOutsideFile; // Case 2.4
+}
+
 } // 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(
       AST, Pos, AST.getSourceManager().getMainFileID());
   tooling::RefactoringRuleContext Context(AST.getSourceManager());
   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 = renamable(*RenameDecl, File, Index)) {
+    auto Message = [](ReasonToReject Reason) {
+      switch (Reason) {
+      case NoIndexProvided:
+        return "no index is provided";
+      case UsedOutsideFile:
+        return "the symbol is used outside main file";
+      case NonIndexable:
+        return "the symbol is not indexable";
+      }
+      llvm_unreachable("unhandled reason kind");
+    };
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("reject to rename: {0}", Message(*Reject)),
+        llvm::inconvertibleErrorCode());
+  }
+
   Rename->invoke(ResultCollector, Context);
 
   assert(ResultCollector.Result.hasValue());
   if (!ResultCollector.Result.getValue())
     return expandDiagnostics(ResultCollector.Result->takeError(),
                              ASTCtx.getDiagnostics());
 
   tooling::Replacements FilteredChanges;

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

 #include "clang/Tooling/Core/Replacement.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(RenameRange, Code, Range, "") {
+  return replacementToEdit(Code, arg).range == Range;
+}
+
 TEST(RenameTest, SingleFile) {
   struct Test {
     const char* Before;
     const char* After;
   } Tests[] = {
       // Rename function.
       {
           R"cpp(
for (const Test &T : Tests) {
     auto ApplyResult =
         tooling::applyAllReplacements(Code.code(), *RenameResult);
     ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
     EXPECT_EQ(T.After, *ApplyResult) << T.Before;
   }
 }
 
+TEST(RenameTest, WithIndex) {
+  const char *Tests[] = {
+      R"cpp(
+        class [[Foo]] {};
+        void f() {
+          [[Fo^o]] b;
+        }
+     )cpp",
+
+      R"cpp(
+        void f(int [[Lo^cal]]) {
+          [[Local]] = 2;
+        }
+      )cpp",
+
+      // Cases where we reject to do the rename.
+      R"cpp( // no symbol at the cursor
+        // This is in comme^nt.
+       )cpp",
+
+      R"cpp(
+        void f() { // Outside main file.
+          Out^side s;
+        }
+      )cpp",
+  };
+  const char *Header = "class Outside {};";
+
+  TestTU OtherFile = TestTU::withCode("Outside s;");
+  OtherFile.HeaderCode = Header;
+  OtherFile.Filename = "other.cc";
+
+  for (const char *Test : Tests) {
+    Annotations T(Test);
+    TestTU MainFile;
+    MainFile.Code = T.code();
+    MainFile.HeaderCode = Header;
+    auto AST = MainFile.build();
+    // Build a fake index containing refs of MainFile and OtherFile
+    auto OtherFileIndex = OtherFile.index();
+    auto MainFileIndex = MainFile.index();
+    auto Index = MergedIndex(MainFileIndex.get(), OtherFileIndex.get());
+
+    auto Results = renameWithinFile(AST, testPath(MainFile.Filename), T.point(),
+                                    "dummyNewName", &Index);
+    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(MainFile.Code, R));
+      EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
+    }
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 class RenameOccurrences final : public SourceChangeRefactoringRule {
 public:
   static Expected<RenameOccurrences> initiate(RefactoringRuleContext &Context,
                                               SourceRange SelectionRange,
                                               std::string NewName);
 
   static const RefactoringDescriptor &describe();
 
+  const NamedDecl *getRenameDecl() const;
+
 private:
   RenameOccurrences(const NamedDecl *ND, std::string NewName)
       : ND(ND), NewName(std::move(NewName)) {}
 
   Expected<AtomicChanges>
   createSourceReplacements(RefactoringRuleContext &Context) override;
 
   const NamedDecl *ND;

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

const NamedDecl *ND =
       getNamedDeclAt(Context.getASTContext(), SelectionRange.getBegin());
   if (!ND)
     return Context.createDiagnosticError(
         SelectionRange.getBegin(), diag::err_refactor_selection_no_symbol);
   return RenameOccurrences(getCanonicalSymbolDeclaration(ND),
                            std::move(NewName));
 }
 
+const NamedDecl *RenameOccurrences::getRenameDecl() const { return ND; }
+
 Expected<AtomicChanges>
 RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   Expected<SymbolOccurrences> Occurrences = findSymbolOccurrences(ND, Context);
   if (!Occurrences)
     return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
   SymbolName Name(NewName);
   return createRenameReplacements(