[clangd] Move Rename into its own file, and add unit test. NFC

[kadircet]

LGTM

clangd/ClangdServer.cpp

264

Why change Path to std::string ?

[sammccall]

clangd/ClangdServer.cpp

264

Oops, removed this capture and then re-added it.

[sammccall]

Incidentally, this was not NFC - it deduplicates edits and therefore works around a bug in the visitor!

clangd/CMakeLists.txt

add_clang_library(clangDaemon
   index/SymbolOrigin.cpp
   index/YAMLSerialization.cpp
 
   index/dex/Dex.cpp
   index/dex/Iterator.cpp
   index/dex/PostingList.cpp
   index/dex/Trigram.cpp
 
+  refactor/Rename.cpp
   refactor/Tweak.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangDriver
   clangFormat

clangd/ClangdServer.cpp

 #include "Headers.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "TUScheduler.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
 #include "index/Merge.h"
+#include "refactor/Rename.h"
 #include "refactor/Tweak.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
-#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <future>
 #include <memory>
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-// Expand a DiagnosticError to make it print-friendly (print the detailed
-// message, rather than "clang diagnostic").
-llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
-  if (auto Diag = DiagnosticError::take(Err)) {
-    llvm::cantFail(std::move(Err));
-    SmallVector<char, 128> DiagMessage;
-    Diag->second.EmitToString(DE, DiagMessage);
-    return llvm::make_error<llvm::StringError>(DiagMessage,
-                                               llvm::inconvertibleErrorCode());
-  }
-  return Err;
-}
-
-class RefactoringResultCollector final
-    : public tooling::RefactoringResultConsumer {
-public:
-  void handleError(llvm::Error Err) override {
-    assert(!Result.hasValue());
-    Result = std::move(Err);
-  }
-
-  // Using the handle(SymbolOccurrences) from parent class.
-  using tooling::RefactoringResultConsumer::handle;
-
-  void handle(tooling::AtomicChanges SourceReplacements) override {
-    assert(!Result.hasValue());
-    Result = std::move(SourceReplacements);
-  }
-
-  llvm::Optional<llvm::Expected<tooling::AtomicChanges>> Result;
-};
-
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
   UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer &DiagConsumer)
       : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
 
   void onPreambleAST(PathRef Path, ASTContext &Ctx,
                      std::shared_ptr<clang::Preprocessor> PP,
                      const CanonicalIncludes &CanonIncludes) override {
if (PreviousLBracePos == llvm::StringRef::npos)
     PreviousLBracePos = *CursorPos;
   size_t Len = *CursorPos - PreviousLBracePos;
 
   return formatCode(Code, File, {tooling::Range(PreviousLBracePos, Len)});
 }
 
 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) {
     if (!InpAST)
       return CB(InpAST.takeError());
-    auto &AST = InpAST->AST;
-
-    RefactoringResultCollector ResultCollector;
-    const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
-    SourceLocation SourceLocationBeg =
-        clangd::getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
-    tooling::RefactoringRuleContext Context(
-        AST.getASTContext().getSourceManager());
-    Context.setASTContext(AST.getASTContext());
-    auto Rename = clang::tooling::RenameOccurrences::initiate(
-        Context, SourceRange(SourceLocationBeg), NewName);
-    if (!Rename)
-      return CB(expandDiagnostics(Rename.takeError(),
-                                  AST.getASTContext().getDiagnostics()));
-
-    Rename->invoke(ResultCollector, Context);
-
-    assert(ResultCollector.Result.hasValue());
-    if (!ResultCollector.Result.getValue())
-      return CB(expandDiagnostics(ResultCollector.Result->takeError(),
-                                  AST.getASTContext().getDiagnostics()));
-
-    std::vector<TextEdit> Replacements;
-    for (const tooling::AtomicChange &Change : ResultCollector.Result->get()) {
-      tooling::Replacements ChangeReps = Change.getReplacements();
-      for (const auto &Rep : ChangeReps) {
-        // FIXME: Right now we only support renaming the main file, so we
-        // drop replacements not for the main file. In the future, we might
-        // consider to support:
-        //   * rename in any included header
-        //   * rename only in the "main" header
-        //   * provide an error if there are symbols we won't rename (e.g.
-        //     std::vector)
-        //   * rename globally in project
-        //   * rename in open files
-        if (Rep.getFilePath() == File)
-          Replacements.push_back(
-              replacementToEdit(InpAST->Inputs.Contents, Rep));
-      }
-    }
-    return CB(std::move(Replacements));
+    auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName);
+    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));
   };
 
   WorkScheduler.runWithAST(
       "Rename", File, Bind(Action, File.str(), NewName.str(), std::move(CB)));
 }
 
 static llvm::Expected<Tweak::Selection>
 tweakSelection(const Range &Sel, const InputsAndAST &AST) {

clangd/ClangdUnit.h

 /// result of calling buildPreamble.
 llvm::Optional<ParsedAST>
 buildAST(PathRef FileName, std::unique_ptr<CompilerInvocation> Invocation,
          const ParseInputs &Inputs,
          std::shared_ptr<const PreambleData> Preamble);
 
 /// Get the beginning SourceLocation at a specified \p Pos.
 /// May be invalid if Pos is, or if there's no identifier.
-SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
-                                        const FileID FID);
+SourceLocation getBeginningOfIdentifier(const ParsedAST &Unit,
+                                        const Position &Pos, const FileID FID);
 
 /// For testing/debugging purposes. Note that this method deserializes all
 /// unserialized Decls, so use with care.
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H

clangd/ClangdUnit.cpp

buildAST(PathRef FileName, std::unique_ptr<CompilerInvocation> Invocation,
   }
 
   return ParsedAST::build(llvm::make_unique<CompilerInvocation>(*Invocation),
                           Preamble,
                           llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents),
                           std::move(VFS), Inputs.Index, Inputs.Opts);
 }
 
-SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
-                                        const FileID FID) {
+SourceLocation getBeginningOfIdentifier(const ParsedAST &Unit,
+                                        const Position &Pos, const FileID FID) {
   const ASTContext &AST = Unit.getASTContext();
   const SourceManager &SourceMgr = AST.getSourceManager();
   auto Offset = positionToOffset(SourceMgr.getBufferData(FID), Pos);
   if (!Offset) {
     log("getBeginningOfIdentifier: {0}", Offset.takeError());
     return SourceLocation();
   }
 

clangd/refactor/Rename.h

+//===--- Rename.h - 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 "ClangdUnit.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);
+
+} // namespace clangd
+} // namespace clang

clangd/refactor/Rename.cpp

+#include "refactor/Rename.h"
+#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
+#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+class RefactoringResultCollector final
+    : public tooling::RefactoringResultConsumer {
+public:
+  void handleError(llvm::Error Err) override {
+    assert(!Result.hasValue());
+    Result = std::move(Err);
+  }
+
+  // Using the handle(SymbolOccurrences) from parent class.
+  using tooling::RefactoringResultConsumer::handle;
+
+  void handle(tooling::AtomicChanges SourceReplacements) override {
+    assert(!Result.hasValue());
+    Result = std::move(SourceReplacements);
+  }
+
+  llvm::Optional<llvm::Expected<tooling::AtomicChanges>> Result;
+};
+
+// Expand a DiagnosticError to make it print-friendly (print the detailed
+// message, rather than "clang diagnostic").
+llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
+  if (auto Diag = DiagnosticError::take(Err)) {
+    llvm::cantFail(std::move(Err));
+    SmallVector<char, 128> DiagMessage;
+    Diag->second.EmitToString(DE, DiagMessage);
+    return llvm::make_error<llvm::StringError>(DiagMessage,
+                                               llvm::inconvertibleErrorCode());
+  }
+  return Err;
+}
+
+} // namespace
+
+llvm::Expected<tooling::Replacements>
+renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
+                 llvm::StringRef NewName) {
+  RefactoringResultCollector ResultCollector;
+  ASTContext &ASTCtx = AST.getASTContext();
+  const SourceManager &SourceMgr = ASTCtx.getSourceManager();
+  SourceLocation SourceLocationBeg =
+      clangd::getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+  tooling::RefactoringRuleContext Context(ASTCtx.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.
+  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

clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   FuzzyMatchTests.cpp
   GlobalCompilationDatabaseTests.cpp
   HeadersTests.cpp
   IndexActionTests.cpp
   IndexTests.cpp
   JSONTransportTests.cpp
   PrintASTTests.cpp
   QualityTests.cpp
+  RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
   SymbolInfoTests.cpp
   SyncAPI.cpp
   TUSchedulerTests.cpp

clangd/unittests/RenameTests.cpp

+//===-- RenameTests.cpp -----------------------------------------*- 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 "Annotations.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "refactor/Rename.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(RenameTest, SingleFile) {
+  Annotations Code(R"cpp(
+    void foo() {
+      fo^o();
+    }
+  )cpp");
+  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();
+
+  const char *Want = R"cpp(
+    void abcde() {
+      abcde();
+    }
+  )cpp";
+  EXPECT_EQ(Want, *ApplyResult);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang