[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 ↗

(On Diff #205056)

pull out a function for this?

83 ↗

(On Diff #205056)

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 ↗

(On Diff #205056)

does this not work for macros?

85 ↗

(On Diff #205056)

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 ↗

(On Diff #205056)

unfortunately, the rename library doesn't rename marcos.

85 ↗

(On Diff #205056)

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 ↗

(On Diff #205355)

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

113 ↗

(On Diff #205355)

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 ↗

(On Diff #205963)

...to find some other file...

98 ↗

(On Diff #205963)

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 ↗

(On Diff #205963)

(which is not a header)

107 ↗

(On Diff #205963)

renamableWithinFile

110 ↗

(On Diff #205963)

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

115 ↗

(On Diff #205963)

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

129 ↗

(On Diff #205963)

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.

167 ↗

(On Diff #205963)

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 ↗

(On Diff #205963)

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.

cfe/trunk/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;

cfe/trunk/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(

clang-tools-extra/trunk/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());
 
     auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
                                        InpAST->Inputs.FS.get());
     if (auto Formatted =
             cleanupAndFormat(InpAST->Inputs.Contents, *Changes, Style))
       *Changes = std::move(*Formatted);

clang-tools-extra/trunk/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);
+/// 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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H

clang-tools-extra/trunk/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"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 class RefactoringResultCollector final
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 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(
       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 = 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());
   if (!ResultCollector.Result.getValue())
     return expandDiagnostics(ResultCollector.Result->takeError(),
                              ASTCtx.getDiagnostics());
 
   tooling::Replacements FilteredChanges;

clang-tools-extra/trunk/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, 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