[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

79

pull out a function for this?

83

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?

83

does this not work for macros?

85

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

83

unfortunately, the rename library doesn't rename marcos.

85

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

81

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

125

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

69

...to find some other file...

98

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)
  ...

99

(which is not a header)

107

renamableWithinFile

110

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

115

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

129

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.

138

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 "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;
 }
 
+static 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();
+}
+
 } // 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());
+
+  if (Index) {
+    // Consult the index to determine whether the symbol is used outside of
+    // the current file.
+    // FIXME: we may skip querying the index if D is function-local.
+    const NamedDecl *D =
+        clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg);
+    if (!D)
+      return llvm::make_error<llvm::StringError>(
+          "there is no symbol at the given location",
+          llvm::inconvertibleErrorCode());
+    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(D))
+      Req.IDs.insert(*ID);
+    bool OutsideMainFile = false;
+    std::string OtherFile;
+    Index->refs(Req, [&](const Ref &R) {
+      if (OutsideMainFile)
+        return;
+      if (auto RefFilePath = filePath(R.Location, File)) {
+        if (*RefFilePath != File) {
+          OtherFile = *RefFilePath;
+          OutsideMainFile = true;
+        }
+      }
+    });
+    if (OutsideMainFile)
+      return llvm::make_error<llvm::StringError>(
+          llvm::formatv(
+              "renaming symbol used outside of the file is not supported: ",
+              OtherFile),
+          llvm::inconvertibleErrorCode());
+  }
+
   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());
 
   Rename->invoke(ResultCollector, Context);
 
   assert(ResultCollector.Result.hasValue());
   if (!ResultCollector.Result.getValue())
     return expandDiagnostics(ResultCollector.Result->takeError(),
                              ASTCtx.getDiagnostics());
 
   tooling::Replacements FilteredChanges;
   // Right now we only support renaming the main file, so we
   // drop replacements not for the main file. In the future, we might
   // also support rename with wider scope.
   // Rename sometimes returns duplicate edits (which is a bug). A side-effect of
   // adding them to a single Replacements object is these are deduplicated.
   for (const tooling::AtomicChange &Change : ResultCollector.Result->get()) {
     for (const auto &Rep : Change.getReplacements()) {
       if (Rep.getFilePath() == File)
         cantFail(FilteredChanges.add(Rep));
     }
   }
   return FilteredChanges;
 }
 
 } // namespace clangd
 } // namespace clang

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