hokein updated this revision to Diff 227278.
hokein marked 8 inline comments as done.
hokein added a comment.
address comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69263/new/
https://reviews.llvm.org/D69263
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Rename.h
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/SyncAPI.cpp
clang-tools-extra/clangd/unittests/SyncAPI.h
Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===================================================================
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
llvm::Expected<std::vector<DocumentHighlight>>
runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
-llvm::Expected<std::vector<TextEdit>>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
+ Position Pos, StringRef NewName);
std::string runDumpAST(ClangdServer &Server, PathRef File);
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
return std::move(*Result);
}
-llvm::Expected<std::vector<TextEdit>> runRename(ClangdServer &Server,
- PathRef File, Position Pos,
- llvm::StringRef NewName) {
- llvm::Optional<llvm::Expected<std::vector<TextEdit>>> Result;
- Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
+ Position Pos, llvm::StringRef NewName) {
+ llvm::Optional<llvm::Expected<FileEdits>> Result;
+ Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+ /*DirtyBuffer=*/nullptr, capture(Result));
return std::move(*Result);
}
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
#include "Annotations.h"
#include "TestFS.h"
#include "TestTU.h"
+#include "index/Ref.h"
#include "refactor/Rename.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -18,9 +20,68 @@
namespace clangd {
namespace {
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
MATCHER_P2(RenameRange, Code, Range, "") {
return replacementToEdit(Code, arg).range == Range;
}
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+ using ReplacementMatcher = testing::Matcher<tooling::Replacement>;
+ std::vector<ReplacementMatcher> Expected;
+ for (const auto &R : Ranges)
+ Expected.push_back(RenameRange(Code, R));
+ auto Matcher =
+ testing::internal::UnorderedElementsAreArrayMatcher<ReplacementMatcher>(
+ Expected.begin(), Expected.end());
+ return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr<RefSlab> buildRefSlab(const Annotations &Code,
+ llvm::StringRef SymbolName,
+ llvm::StringRef Path) {
+ RefSlab::Builder Builder;
+ TestTU TU;
+ TU.HeaderCode = Code.code();
+ auto Symbols = TU.headerSymbols();
+ const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+ for (const auto &Range : Code.ranges()) {
+ Ref R;
+ R.Kind = RefKind::Reference;
+ R.Location.Start.setLine(Range.start.line);
+ R.Location.Start.setColumn(Range.start.character);
+ R.Location.End.setLine(Range.end.line);
+ R.Location.End.setColumn(Range.end.character);
+ auto U = URI::create(Path).toString();
+ R.Location.FileURI = U.c_str();
+ Builder.insert(SymbolID, R);
+ }
+
+ return std::make_unique<RefSlab>(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+ llvm::StringRef Path,
+ const SymbolIndex *Index = nullptr,
+ bool CrossFile = false) {
+ RenameInputs Inputs;
+ Inputs.Pos = Code.point();
+ Inputs.MainFileCode = Code.code();
+ Inputs.MainFilePath = Path;
+ Inputs.NewName = NewName;
+ Inputs.Index = Index;
+ Inputs.AllowCrossFile = CrossFile;
+ return Inputs;
+}
+
+std::vector<std::pair<std::string, Edit>> toVector(FileEdits FE) {
+ std::vector<std::pair<std::string, Edit>> Results;
+ for (auto &It : FE)
+ Results.emplace_back(It.first().str(), std::move(It.getValue()));
+ return Results;
+}
TEST(RenameTest, SingleFile) {
struct Test {
@@ -80,10 +141,13 @@
auto TU = TestTU::withCode(Code.code());
auto AST = TU.build();
auto RenameResult =
- renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+ rename(AST, renameInputs(Code, "abcde", testPath(TU.Filename)));
+
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
- auto ApplyResult =
- tooling::applyAllReplacements(Code.code(), *RenameResult);
+ ASSERT_TRUE(RenameResult->size() == 1);
+ EXPECT_EQ(Code.code(), RenameResult->begin()->second.InitialCode);
+ auto ApplyResult = tooling::applyAllReplacements(
+ Code.code(), RenameResult->begin()->second.Replacements);
ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
EXPECT_EQ(T.After, *ApplyResult) << T.Before;
@@ -177,28 +241,106 @@
}
auto AST = TU.build();
- auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
- "dummyNewName", Case.Index);
+ auto Results = rename(AST, renameInputs(T, "dummyNewName",
+ testPath(TU.Filename), Case.Index));
bool WantRename = true;
if (T.ranges().empty())
WantRename = false;
if (!WantRename) {
assert(Case.ErrorMessage && "Error message must be set!");
- EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
- << T.code();
+ EXPECT_FALSE(Results)
+ << "expected rename returned an error: " << T.code();
auto ActualMessage = llvm::toString(Results.takeError());
EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage));
} else {
- EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+ EXPECT_TRUE(bool(Results)) << "rename 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));
+ EXPECT_THAT(toVector(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(testing::_, EqualFileEdit(T.code(), T.ranges()))));
}
}
}
+TEST(RenameTests, CrossFile) {
+ Annotations FooCode("class [[Foo]] {};");
+ std::string FooPath = testPath("foo.cc");
+ std::string FooDirtyBuffer =
+ (FooCode.code() + "\n// this is dirty buffer").str();
+ Annotations BarCode("void [[Bar]]() {}");
+ std::string BarPath = testPath("bar.cc");
+ // Build the index, the index has "Foo" references from foo.cc and "Bar"
+ // references from bar.cc.
+ FileSymbols FSymbols;
+ FSymbols.update(FooPath, nullptr, buildRefSlab(FooCode, "Foo", FooPath),
+ nullptr, false);
+ FSymbols.update(BarPath, nullptr, buildRefSlab(BarCode, "Bar", BarPath),
+ nullptr, false);
+ auto Index = FSymbols.buildIndex(IndexType::Light);
+
+ // Prepare the rename environment:
+ // - dirty buffer for foo.cc;
+ // - file on VFS for bar.cc;
+ Annotations MainCode("class [[Fo^o]] {};");
+ auto MainFilePath = testPath("main.cc");
+ RenameInputs Input =
+ renameInputs(MainCode, "newName", MainFilePath, Index.get(),
+ /*CrossFile*/ true);
+ Input.GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
+ if (Path == FooPath)
+ return FooDirtyBuffer;
+ return llvm::None;
+ };
+ llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> MemFS(
+ new llvm::vfs::InMemoryFileSystem);
+ MemFS->addFile(testPath("bar.cc"), 0,
+ llvm::MemoryBuffer::getMemBuffer(BarCode.code()));
+ Input.VFS = MemFS;
+
+ // Run rename on Foo, there is a dirty buffer for foo.cc, rename should
+ // respect the dirty buffer.
+ TestTU TU = TestTU::withCode(MainCode.code());
+ auto AST = TU.build();
+ auto Results = rename(AST, Input);
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(
+ toVector(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(Eq(FooPath), EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),
+ Pair(Eq(MainFilePath),
+ EqualFileEdit(MainCode.code(), MainCode.ranges()))));
+
+ // Run rename on Bar, there is no dirty buffer for the affected file bar.cc,
+ // so we should read file content from VFS.
+ MainCode = Annotations("void [[Bar]]() { [[B^ar]](); }");
+ Input.MainFileCode = MainCode.code();
+ Input.Pos = MainCode.point();
+ TU = TestTU::withCode(MainCode.code());
+ AST = TU.build();
+ Results = rename(AST, Input);
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(
+ toVector(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(Eq(BarPath), EqualFileEdit(BarCode.code(), BarCode.ranges())),
+ Pair(Eq(MainFilePath),
+ EqualFileEdit(MainCode.code(), MainCode.ranges()))));
+}
+
+TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) {
+ // cross-file rename should work for function-local symbols, even there is no
+ // index provided.
+ Annotations Code("void f(int [[abc]]) { [[a^bc]] = 3; }");
+ auto TU = TestTU::withCode(Code.code());
+ auto Path = testPath(TU.Filename);
+ auto AST = TU.build();
+ auto Results = rename(AST, renameInputs(Code, "abcde", Path));
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(toVector(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(Eq(Path), EqualFileEdit(Code.code(), Code.ranges()))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -264,6 +264,16 @@
CommaSeparated,
};
+opt<bool> CrossFileRename{
+ "cross-file-rename",
+ cat(Features),
+ desc("Enable cross-file rename feature. Note that this feature is "
+ "experimental and may lead to broken code or incomplete rename "
+ "results"),
+ init(false),
+ Hidden,
+};
+
opt<unsigned> WorkerThreadsCount{
"j",
cat(Misc),
@@ -595,6 +605,7 @@
}
Opts.StaticIndex = StaticIdx.get();
Opts.AsyncThreadsCount = WorkerThreadsCount;
+ Opts.CrossFileRename = CrossFileRename;
clangd::CodeCompleteOptions CCOpts;
CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
Index: clang-tools-extra/clangd/refactor/Tweak.h
===================================================================
--- clang-tools-extra/clangd/refactor/Tweak.h
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -79,7 +79,7 @@
llvm::Optional<std::string> ShowMessage;
/// A mapping from file path(the one used for accessing the underlying VFS)
/// to edits.
- llvm::StringMap<Edit> ApplyEdits;
+ FileEdits ApplyEdits;
static Effect showMessage(StringRef S) {
Effect E;
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -9,7 +9,9 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H
+#include "Path.h"
#include "Protocol.h"
+#include "SourceCode.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -18,13 +20,27 @@
class ParsedAST;
class SymbolIndex;
+using DirtyBufferGetter =
+ std::function<llvm::Optional<std::string>(PathRef Path)>;
+
+struct RenameInputs {
+ Position Pos; // the position triggering the rename
+ llvm::StringRef NewName;
+
+ llvm::StringRef MainFilePath;
+ llvm::StringRef MainFileCode;
+
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS;
+ const SymbolIndex *Index = nullptr;
+
+ bool AllowCrossFile = false;
+ DirtyBufferGetter GetDirtyBuffer;
+};
+
/// Renames all occurrences of the symbol at \p Pos to \p NewName.
-/// Occurrences outside the current file are not modified.
-/// 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);
+/// If AllowCrossFile is false, returns an error if rename a symbol that's used
+/// in another file (per the index).
+llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs);
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -60,56 +60,83 @@
NoSymbolFound,
NoIndexProvided,
NonIndexable,
- UsedOutsideFile,
+ UsedOutsideFile, // for within-file rename only.
UnsupportedSymbol,
};
-// Check the symbol Decl is renameable (per the index) within the file.
-llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
- StringRef MainFile,
- const SymbolIndex *Index) {
+llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
+ StringRef MainFilePath,
+ const SymbolIndex *Index,
+ bool CrossFile) {
+ // Filter out symbols that are unsupported in both rename mode.
if (llvm::isa<NamespaceDecl>(&RenameDecl))
return ReasonToReject::UnsupportedSymbol;
if (const auto *FD = llvm::dyn_cast<FunctionDecl>(&RenameDecl)) {
if (FD->isOverloadedOperator())
return ReasonToReject::UnsupportedSymbol;
}
- auto &ASTCtx = RenameDecl.getASTContext();
- const auto &SM = ASTCtx.getSourceManager();
- bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
- bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
-
- if (!DeclaredInMainFile)
- // We are sure the symbol is used externally, bail out early.
- return UsedOutsideFile;
-
- // If the symbol is declared in the main file (which is not a header), we
- // rename it.
- if (!MainFileIsHeader)
- return None;
-
- // Below are cases where the symbol is declared in the header.
- // If the symbol is function-local, we rename it.
+ // function-local symbols is safe to rename.
if (RenameDecl.getParentFunctionOrMethod())
return None;
+ bool IsIndexable =
+ isa<NamedDecl>(RenameDecl) &&
+ SymbolCollector::shouldCollectSymbol(
+ cast<NamedDecl>(RenameDecl), RenameDecl.getASTContext(),
+ SymbolCollector::Options(), CrossFile);
+ if (!IsIndexable) // If the symbol is not indexable, we disallow rename.
+ return ReasonToReject::NonIndexable;
+
+ if (!CrossFile) {
+ // Within-file rename.
+ auto &ASTCtx = RenameDecl.getASTContext();
+ const auto &SM = ASTCtx.getSourceManager();
+ bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
+ bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
+
+ if (!DeclaredInMainFile)
+ // We are sure the symbol is used externally, bail out early.
+ return UsedOutsideFile;
+
+ // If the symbol is declared in the main file (which is not a header), we
+ // rename it.
+ if (!MainFileIsHeader)
+ return None;
+
+ if (!Index)
+ return NoIndexProvided;
+
+ auto OtherFile = getOtherRefFile(RenameDecl, MainFilePath, *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;
+ }
+
+ assert(CrossFile);
if (!Index)
- return ReasonToReject::NoIndexProvided;
+ return NoIndexProvided;
- bool IsIndexable = isa<NamedDecl>(RenameDecl) &&
- SymbolCollector::shouldCollectSymbol(
- cast<NamedDecl>(RenameDecl), ASTCtx, {}, false);
- // If the symbol is not indexable, we disallow rename.
- if (!IsIndexable)
- return ReasonToReject::NonIndexable;
- auto OtherFile = getOtherRefFile(RenameDecl, MainFile, *Index);
- // If the symbol is indexable and has no refs from other files in the index,
- // we rename it.
- if (!OtherFile)
+ // FIXME: renaming templates requries to rename all related specializations,
+ // which is not supported in our index.
+ if (RenameDecl.getDescribedTemplate())
+ return ReasonToReject::UnsupportedSymbol;
+
+ // A whitelist of renamable symbols.
+ if (llvm::isa<CXXRecordDecl>(RenameDecl))
+ return None;
+ if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
+ // FIXME: renaming virtual methods requires to rename all overridens in
+ // subclasses, which our index doesn't support yet.
+ if (!S->isVirtual())
+ return None;
+ } else if (isa<FunctionDecl>(&RenameDecl)) {
return None;
- // If the symbol is indexable and has refs from other files in the index,
- // we disallow rename.
- return ReasonToReject::UsedOutsideFile;
+ }
+ return UnsupportedSymbol;
}
llvm::Error makeError(ReasonToReject Reason) {
@@ -133,7 +160,7 @@
llvm::inconvertibleErrorCode());
}
-// Return all rename occurrences in the main file.
+// Find all rename occurrences in the main file based on the MainAST.
tooling::SymbolOccurrences
findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
const NamedDecl *CanonicalRenameDecl =
@@ -152,28 +179,10 @@
return Result;
}
-} // namespace
-
+// AST-based rename, it renames all occurrences in the main file.
llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
- llvm::StringRef NewName, const SymbolIndex *Index) {
- const SourceManager &SM = AST.getSourceManager();
- SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
- getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
- // FIXME: renaming macros is not supported yet, the macro-handling code should
- // be moved to rename tooling library.
- if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
- return makeError(UnsupportedSymbol);
-
- const auto *RenameDecl =
- tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg);
- if (!RenameDecl)
- return makeError(NoSymbolFound);
-
- if (auto Reject =
- renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
- return makeError(*Reject);
-
+renameWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl,
+ llvm::StringRef NewName) {
// Rename sometimes returns duplicate edits (which is a bug). A side-effect of
// adding them to a single Replacements object is these are deduplicated.
tooling::Replacements FilteredChanges;
@@ -194,5 +203,184 @@
return FilteredChanges;
}
+Range toRange(const SymbolLocation &L) {
+ Range R;
+ R.start.line = L.Start.line();
+ R.start.character = L.Start.column();
+ R.end.line = L.End.line();
+ R.end.character = L.End.column();
+ return R;
+};
+
+// Return all rename occurrences (per the index) outside of the main file,
+// grouped by the file name.
+llvm::StringMap<std::vector<Range>>
+findOccurrencesOutsideFile(const NamedDecl *RenameDecl,
+ llvm::StringRef MainFile, const SymbolIndex *Index) {
+ if (!Index)
+ return {};
+ RefsRequest RQuest;
+
+ if (auto ID = getSymbolID(RenameDecl))
+ RQuest.IDs.insert(*ID);
+
+ // Absolute file path => rename ocurrences in that file.
+ llvm::StringMap<std::vector<Range>> AffectedFiles;
+ Index->refs(RQuest, [&](const Ref &R) {
+ if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
+ if (*RefFilePath != MainFile)
+ AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
+ }
+ });
+ return AffectedFiles;
+}
+
+llvm::Expected<std::pair<size_t, size_t>> toRangeOffset(const clangd::Range &R,
+ llvm::StringRef Code) {
+ auto StartOffset = positionToOffset(Code, R.start);
+ if (!StartOffset)
+ return StartOffset.takeError();
+ auto EndOffset = positionToOffset(Code, R.end);
+ if (!EndOffset)
+ return EndOffset.takeError();
+ return std::make_pair<>(*StartOffset, *EndOffset);
+};
+
+llvm::Expected<Edit> buildRenameEdit(llvm::StringRef InitialCode,
+ const std::vector<Range> &Occurrences,
+ llvm::StringRef NewName) {
+ tooling::Replacements RenameEdit;
+ for (const Range &Occurrence : Occurrences) {
+ // !positionToOffset is O(N), it is okay at the moment since we only
+ // process at most 100 references.
+ auto RangeOffset = toRangeOffset(Occurrence, InitialCode);
+ if (!RangeOffset)
+ return RangeOffset.takeError();
+ auto ByteLength = RangeOffset->second - RangeOffset->first;
+ if (auto Err = RenameEdit.add(tooling::Replacement(
+ InitialCode, RangeOffset->first, ByteLength, NewName)))
+ return std::move(Err);
+ }
+ return Edit(InitialCode, std::move(RenameEdit));
+}
+
+// Index-based rename, it renames all occurrences outside of the main file.
+//
+// The cross-file rename is purely based on the index, as we don't want to
+// build all ASTs for affected files, which may cause a performance hit.
+// We choose to trade off some correctness for performance and scalability.
+//
+// Clangd builds a dynamic index for all opened files on top of the static
+// index of the whole codebase. Dynamic index is up-to-date (respects dirty
+// buffers) as long as clangd finishes processing opened files, while static
+// index (background index) is relatively stale. We choose the dirty buffers
+// as the file content we rename on, and fallback to file content on disk if
+// there is no dirty buffer.
+//
+// FIXME: add range patching heuristics to detect staleness of the index, and
+// report to users.
+// FIXME: our index may return implicit references, which are non-eligitble
+// for rename, we should filter out these references.
+llvm::Expected<FileEdits>
+renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath,
+ llvm::StringRef NewName, const SymbolIndex *Index,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+ const DirtyBufferGetter &GetDirtyBuffer) {
+ auto AffectedFiles =
+ findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+ // FIXME: make the limit customizable.
+ static constexpr size_t MaxLimitFiles = 50;
+ if (AffectedFiles.size() >= MaxLimitFiles)
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv(
+ "The number of affected files exceeds the max limit {0}: {1}",
+ MaxLimitFiles, AffectedFiles.size()),
+ llvm::inconvertibleErrorCode());
+
+ FileEdits Results;
+ std::string OldName = RenameDecl->getNameAsString();
+ for (const auto &FileAndOccurrences : AffectedFiles) {
+ llvm::StringRef FilePath = FileAndOccurrences.first();
+
+ std::string AffectedFileCode;
+ llvm::Optional<std::string> DirtyBuffer;
+ if (GetDirtyBuffer && (DirtyBuffer = GetDirtyBuffer(FilePath))) {
+ AffectedFileCode = std::move(*DirtyBuffer);
+ } else {
+ auto Content = VFS->getBufferForFile(FilePath);
+ if (!Content) {
+ elog("Fail to read file {0}: {1}", FilePath,
+ Content.getError().message());
+ continue;
+ }
+ if (!*Content)
+ continue;
+ AffectedFileCode = (*Content)->getBuffer().str();
+ }
+ auto RenameEdit = buildRenameEdit(AffectedFileCode,
+ FileAndOccurrences.getValue(), NewName);
+ if (!RenameEdit)
+ return RenameEdit.takeError();
+ if (!RenameEdit->Replacements.empty())
+ Results.insert({FilePath, std::move(*RenameEdit)});
+ }
+ return Results;
+}
+
+} // namespace
+
+llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs) {
+ const SourceManager &SM = AST.getSourceManager();
+ SourceLocation SourceLocationBeg =
+ SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+ RInputs.Pos, SM, AST.getASTContext().getLangOpts()));
+ // FIXME: renaming macros is not supported yet, the macro-handling code should
+ // be moved to rename tooling library.
+ if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
+ return makeError(UnsupportedSymbol);
+
+ const NamedDecl *RenameDecl = tooling::getCanonicalSymbolDeclaration(
+ tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg));
+ if (!RenameDecl)
+ return makeError(NoSymbolFound);
+
+ auto Reject =
+ renameable(*RenameDecl->getCanonicalDecl(), RInputs.MainFilePath,
+ RInputs.Index, RInputs.AllowCrossFile);
+ if (Reject)
+ return makeError(*Reject);
+
+ // We have two implemenations of the rename:
+ // - AST-based rename: used for renaming local symbols, e.g. variables
+ // defined in a function body;
+ // - index-based rename: used for renaming non-local symbols, and not
+ // feasible for local symbols (as by design our index don't index these
+ // symbols by design;
+ // To make cross-file rename work for local symbol, we use a hybrid solution:
+ // - run AST-based rename on the main file;
+ // - run index-based rename on other affected files;
+ auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
+ if (!MainFileRenameEdit)
+ return MainFileRenameEdit.takeError();
+
+ if (!RInputs.AllowCrossFile) {
+ // within-file rename, just return the main file results.
+ return FileEdits({std::make_pair<>(
+ RInputs.MainFilePath,
+ Edit{RInputs.MainFileCode, std::move(*MainFileRenameEdit)})});
+ }
+
+ // Cross-file rename.
+ auto Results =
+ renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
+ RInputs.Index, RInputs.VFS, RInputs.GetDirtyBuffer);
+ if (!Results)
+ return Results.takeError();
+ // Attach the rename edits for the main file.
+ Results->try_emplace(RInputs.MainFilePath, RInputs.MainFileCode,
+ std::move(*MainFileRenameEdit));
+ return Results;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -223,6 +223,7 @@
/// Checks whether the Replacements are applicable to given Code.
bool canApplyTo(llvm::StringRef Code) const;
};
+using FileEdits = llvm::StringMap<Edit>;
/// Formats the edits and code around it according to Style. Changes
/// Replacements to formatted ones if succeeds.
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -23,6 +23,7 @@
#include "index/Background.h"
#include "index/FileIndex.h"
#include "index/Index.h"
+#include "refactor/Rename.h"
#include "refactor/Tweak.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "clang/Tooling/Core/Replacement.h"
@@ -132,6 +133,9 @@
/// Enable semantic highlighting features.
bool SemanticHighlighting = false;
+ /// Enable cross-file rename feature.
+ bool CrossFileRename = false;
+
/// Returns true if the tweak should be enabled.
std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
return !T.hidden(); // only enable non-hidden tweaks.
@@ -251,7 +255,8 @@
/// embedders could use this method to get all occurrences of the symbol (e.g.
/// highlighting them in prepare stage).
void rename(PathRef File, Position Pos, llvm::StringRef NewName,
- bool WantFormat, Callback<std::vector<TextEdit>> CB);
+ bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+ Callback<FileEdits> CB);
struct TweakRef {
std::string ID; /// ID to pass for applyTweak.
@@ -326,6 +331,8 @@
// can be caused by missing includes (e.g. member access in incomplete type).
bool SuggestMissingIncludes = false;
+ bool CrossFileRename = false;
+
std::function<bool(const Tweak &)> TweakFilter;
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -119,7 +119,8 @@
: nullptr),
GetClangTidyOptions(Opts.GetClangTidyOptions),
SuggestMissingIncludes(Opts.SuggestMissingIncludes),
- TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
+ CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter),
+ WorkspaceRoot(Opts.WorkspaceRoot),
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
// is parsed.
@@ -308,16 +309,28 @@
if (!InpAST)
return CB(InpAST.takeError());
auto &AST = InpAST->AST;
- // Performing the rename isn't substantially more expensive than doing an
- // AST-based check, so we just rename and throw away the results. We may
- // have to revisit this when we support cross-file rename.
- auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
- if (!Changes) {
- // LSP says to return null on failure, but that will result in a generic
- // failure message. If we send an LSP error response, clients can surface
- // the message to users (VSCode does).
- return CB(Changes.takeError());
+ // FIXME: for cross-file rename, we presume prepareRename always succeeds,
+ // revisit this strategy.
+ if (!CrossFileRename) {
+ // Performing the local rename isn't substantially more expensive than
+ // doing an AST-based check, so we just rename and throw away the results.
+ RenameInputs RInputs;
+ RInputs.MainFileCode = InpAST->Inputs.Contents;
+ RInputs.MainFilePath = File;
+ RInputs.NewName = "dummy";
+ RInputs.Pos = Pos;
+ RInputs.AllowCrossFile = false;
+ RInputs.Index = Index;
+ RInputs.VFS = FSProvider.getFileSystem();
+ auto Changes = clangd::rename(AST, RInputs); // within-file rename.
+ if (!Changes) {
+ // LSP says to return null on failure, but that will result in a generic
+ // failure message. If we send an LSP error response, clients can
+ // surface the message to users (VSCode does).
+ return CB(Changes.takeError());
+ }
}
+
SourceLocation Loc = getBeginningOfIdentifier(
Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());
if (auto Range = getTokenRange(AST.getSourceManager(),
@@ -330,32 +343,36 @@
}
void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
- bool WantFormat, Callback<std::vector<TextEdit>> CB) {
+ bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+ Callback<FileEdits> CB) {
auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,
- CB = std::move(CB),
+ GetDirtyBuffer = std::move(GetDirtyBuffer), CB = std::move(CB),
this](llvm::Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
- auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
- if (!Changes)
- return CB(Changes.takeError());
+ RenameInputs RInputs;
+ RInputs.MainFileCode = InpAST->Inputs.Contents;
+ RInputs.MainFilePath = File;
+ RInputs.NewName = NewName;
+ RInputs.Pos = Pos;
+ RInputs.AllowCrossFile = CrossFileRename;
+ RInputs.Index = Index;
+ RInputs.VFS = FSProvider.getFileSystem();
+ RInputs.GetDirtyBuffer = std::move(GetDirtyBuffer);
+ auto Edits = clangd::rename(InpAST->AST, RInputs);
+ if (!Edits)
+ return CB(Edits.takeError());
if (WantFormat) {
auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
InpAST->Inputs.FS.get());
- if (auto Formatted =
- cleanupAndFormat(InpAST->Inputs.Contents, *Changes, Style))
- *Changes = std::move(*Formatted);
- else
- elog("Failed to format replacements: {0}", Formatted.takeError());
+ for (auto &E : *Edits) {
+ if (auto Err = reformatEdit(E.getValue(), Style))
+ elog("Failed to format replacements: {0}", std::move(Err));
+ }
}
-
- std::vector<TextEdit> Edits;
- for (const auto &Rep : *Changes)
- Edits.push_back(replacementToEdit(InpAST->Inputs.Contents, Rep));
- return CB(std::move(Edits));
+ CB(std::move(*Edits));
};
-
WorkScheduler.runWithAST("Rename", File, std::move(Action));
}
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -103,13 +103,13 @@
return LookupTable;
}
-// Makes sure edits in \p E are applicable to latest file contents reported by
+// Makes sure edits in \p FE are applicable to latest file contents reported by
// editor. If not generates an error message containing information about files
// that needs to be saved.
-llvm::Error validateEdits(const DraftStore &DraftMgr, const Tweak::Effect &E) {
+llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) {
size_t InvalidFileCount = 0;
llvm::StringRef LastInvalidFile;
- for (const auto &It : E.ApplyEdits) {
+ for (const auto &It : FE) {
if (auto Draft = DraftMgr.getDraft(It.first())) {
// If the file is open in user's editor, make sure the version we
// saw and current version are compatible as this is the text that
@@ -704,7 +704,7 @@
if (R->ApplyEdits.empty())
return Reply("Tweak applied.");
- if (auto Err = validateEdits(DraftMgr, *R))
+ if (auto Err = validateEdits(DraftMgr, R->ApplyEdits))
return Reply(std::move(Err));
WorkspaceEdit WE;
@@ -758,17 +758,24 @@
if (!Code)
return Reply(llvm::make_error<LSPError>(
"onRename called for non-added file", ErrorCode::InvalidParams));
-
- Server->rename(File, Params.position, Params.newName, /*WantFormat=*/true,
- [File, Code, Params, Reply = std::move(Reply)](
- llvm::Expected<std::vector<TextEdit>> Edits) mutable {
- if (!Edits)
- return Reply(Edits.takeError());
-
- WorkspaceEdit WE;
- WE.changes = {{Params.textDocument.uri.uri(), *Edits}};
- Reply(WE);
- });
+ Server->rename(
+ File, Params.position, Params.newName,
+ /*WantFormat=*/true,
+ [this](PathRef File) { return DraftMgr.getDraft(File); },
+ [File, Code, Params, Reply = std::move(Reply),
+ this](llvm::Expected<FileEdits> Edits) mutable {
+ if (!Edits)
+ return Reply(Edits.takeError());
+ if (auto Err = validateEdits(DraftMgr, *Edits))
+ return Reply(std::move(Err));
+ WorkspaceEdit Result;
+ Result.changes.emplace();
+ for (const auto &Rep : *Edits) {
+ (*Result.changes)[URI::createFile(Rep.first()).toString()] =
+ Rep.second.asTextEdits();
+ }
+ Reply(Result);
+ });
}
void ClangdLSPServer::onDocumentDidClose(
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
