[clang-tools-extra] 852bafa - [clangd] Implement cross-file rename.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 26 01:12:50 PST 2019
Author: Haojian Wu
Date: 2019-11-26T10:04:31+01:00
New Revision: 852bafae2bb4d875e8d206168a57667f59c0f9a6
URL: https://github.com/llvm/llvm-project/commit/852bafae2bb4d875e8d206168a57667f59c0f9a6
DIFF: https://github.com/llvm/llvm-project/commit/852bafae2bb4d875e8d206168a57667f59c0f9a6.diff
LOG: [clangd] Implement cross-file rename.
Summary:
This is the initial version. The cross-file rename is purely based on
the index.
It is hidden under a command-line flag, and only available for a small set
of symbols.
Reviewers: ilya-biryukov, sammccall
Subscribers: merge_guards_bot, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D69263
Added:
Modified:
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/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 4fe815818074..57ed97f7a782 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -103,13 +103,13 @@ std::vector<std::vector<std::string>> buildHighlightScopeLookupTable() {
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 @@ void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
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,23 @@ void ClangdLSPServer::onRename(const RenameParams &Params,
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,
+ [File, 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(
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 5a9833d78b48..6c5fabdce5c3 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -119,7 +119,8 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
: 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,54 +309,68 @@ void ClangdServer::prepareRename(PathRef File, Position Pos,
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);
+ const auto &SM = AST.getSourceManager();
+ SourceLocation Loc =
+ SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+ Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()));
+ auto Range = getTokenRange(SM, AST.getASTContext().getLangOpts(), Loc);
+ if (!Range)
+ return CB(llvm::None); // "rename" is not valid at the position.
+
+ if (CrossFileRename)
+ // FIXME: we now assume cross-file rename always succeeds, revisit this.
+ return CB(*Range);
+
+ // Performing the local rename isn't substantially more expensive than
+ // doing an AST-based check, so we just rename and throw away the results.
+ auto Changes = clangd::rename({Pos, "dummy", AST, File, Index,
+ /*AllowCrossFile=*/false,
+ /*GetDirtyBuffer=*/nullptr});
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(),
- AST.getASTContext().getLangOpts(), Loc))
- return CB(*Range);
- // Return null if the "rename" is not valid at the position.
- CB(llvm::None);
+ return CB(*Range);
};
WorkScheduler.runWithAST("PrepareRename", File, std::move(Action));
}
void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
- bool WantFormat, Callback<std::vector<TextEdit>> CB) {
+ bool WantFormat, Callback<FileEdits> CB) {
+ // A snapshot of all file dirty buffers.
+ llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents();
auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,
- CB = std::move(CB),
+ CB = std::move(CB), Snapshot = std::move(Snapshot),
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());
+ auto GetDirtyBuffer =
+ [&Snapshot](PathRef AbsPath) -> llvm::Optional<std::string> {
+ auto It = Snapshot.find(AbsPath);
+ if (It == Snapshot.end())
+ return llvm::None;
+ return It->second;
+ };
+ auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index,
+ CrossFileRename, GetDirtyBuffer});
+ 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());
- }
+ llvm::Error Err = llvm::Error::success();
+ for (auto &E : *Edits)
+ Err =
+ llvm::joinErrors(reformatEdit(E.getValue(), Style), 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));
+ if (Err)
+ return CB(std::move(Err));
+ }
+ return CB(std::move(*Edits));
};
-
WorkScheduler.runWithAST("Rename", File, std::move(Action));
}
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index cd0b91c08f08..499340808765 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -24,6 +24,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"
@@ -133,6 +134,9 @@ class ClangdServer {
/// 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.
@@ -252,7 +256,7 @@ class ClangdServer {
/// 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, Callback<FileEdits> CB);
struct TweakRef {
std::string ID; /// ID to pass for applyTweak.
@@ -327,6 +331,8 @@ class ClangdServer {
// 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)
diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h
index 3b8aacef9bf1..f75be998dc2d 100644
--- a/clang-tools-extra/clangd/SourceCode.h
+++ b/clang-tools-extra/clangd/SourceCode.h
@@ -223,6 +223,9 @@ struct Edit {
/// Checks whether the Replacements are applicable to given Code.
bool canApplyTo(llvm::StringRef Code) const;
};
+/// A mapping from absolute file path (the one used for accessing the underlying
+/// VFS) to edits.
+using FileEdits = llvm::StringMap<Edit>;
/// Formats the edits and code around it according to Style. Changes
/// Replacements to formatted ones if succeeds.
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 6436e7a50c61..d740c3873695 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -916,6 +916,13 @@ llvm::StringRef TUScheduler::getContents(PathRef File) const {
return It->second->Contents;
}
+llvm::StringMap<std::string> TUScheduler::getAllFileContents() const {
+ llvm::StringMap<std::string> Results;
+ for (auto &It : Files)
+ Results.try_emplace(It.getKey(), It.getValue()->Contents);
+ return Results;
+}
+
void TUScheduler::run(llvm::StringRef Name,
llvm::unique_function<void()> Action) {
if (!PreambleTasks)
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index ff2d4d485047..de3b89549983 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -180,6 +180,9 @@ class TUScheduler {
/// The returned StringRef may be invalidated by any write to TUScheduler.
llvm::StringRef getContents(PathRef File) const;
+ /// Returns a snapshot of all file buffer contents, per last update().
+ llvm::StringMap<std::string> getAllFileContents() const;
+
/// Schedule an async task with no dependencies.
void run(llvm::StringRef Name, llvm::unique_function<void()> Action);
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index fb83083384f9..d4b186b4ca90 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,6 +18,8 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Error.h"
namespace clang {
namespace clangd {
@@ -55,8 +57,7 @@ llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
// 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);
+ Req.IDs.insert(*getSymbolID(&D));
llvm::Optional<std::string> OtherFile;
Index.refs(Req, [&](const Ref &R) {
if (OtherFile)
@@ -101,71 +102,95 @@ enum ReasonToReject {
NoSymbolFound,
NoIndexProvided,
NonIndexable,
- UsedOutsideFile,
+ UsedOutsideFile, // for within-file rename only.
UnsupportedSymbol,
AmbiguousSymbol,
};
-// 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 modes.
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 = isHeaderFile(MainFile, ASTCtx.getLangOpts());
- 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) {
+ auto &ASTCtx = RenameDecl.getASTContext();
+ const auto &SM = ASTCtx.getSourceManager();
+ bool MainFileIsHeader = isHeaderFile(MainFilePath, ASTCtx.getLangOpts());
+ bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
+
+ if (!DeclaredInMainFile)
+ // We are sure the symbol is used externally, bail out early.
+ return ReasonToReject::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 ReasonToReject::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;
- 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)
- return None;
- // If the symbol is indexable and has refs from other files in the index,
- // we disallow rename.
- return ReasonToReject::UsedOutsideFile;
+ // Blacklist symbols that are not supported yet in cross-file mode due to the
+ // limitations of our index.
+ // FIXME: renaming templates requries to rename all related specializations,
+ // our index doesn't have this information.
+ if (RenameDecl.getDescribedTemplate())
+ return ReasonToReject::UnsupportedSymbol;
+
+ // FIXME: renaming virtual methods requires to rename all overridens in
+ // subclasses, our index doesn't have this information.
+ // Note: within-file rename does support this through the AST.
+ if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
+ if (S->isVirtual())
+ return ReasonToReject::UnsupportedSymbol;
+ }
+ return None;
}
llvm::Error makeError(ReasonToReject Reason) {
auto Message = [](ReasonToReject Reason) {
switch (Reason) {
- case NoSymbolFound:
+ case ReasonToReject::NoSymbolFound:
return "there is no symbol at the given location";
- case NoIndexProvided:
+ case ReasonToReject::NoIndexProvided:
return "symbol may be used in other files (no index available)";
- case UsedOutsideFile:
+ case ReasonToReject::UsedOutsideFile:
return "the symbol is used outside main file";
- case NonIndexable:
+ case ReasonToReject::NonIndexable:
return "symbol may be used in other files (not eligible for indexing)";
- case UnsupportedSymbol:
+ case ReasonToReject::UnsupportedSymbol:
return "symbol is not a supported kind (e.g. namespace, macro)";
case AmbiguousSymbol:
return "there are multiple symbols at the given location";
@@ -212,35 +237,14 @@ std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
return Results;
}
-} // 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) {
+renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
+ llvm::StringRef NewName) {
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);
-
- auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
- if (DeclsUnderCursor.empty())
- return makeError(NoSymbolFound);
- if (DeclsUnderCursor.size() > 1)
- return makeError(AmbiguousSymbol);
-
- const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
- if (!RenameDecl)
- return makeError(UnsupportedSymbol);
-
- if (auto Reject =
- renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
- return makeError(*Reject);
tooling::Replacements FilteredChanges;
- for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
SourceLocation RenameLoc = Loc;
// We don't rename in any macro bodies, but we allow rename the symbol
// spelled in a top-level macro argument in the main file.
@@ -265,5 +269,201 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
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 absolute file path.
+llvm::StringMap<std::vector<Range>>
+findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
+ llvm::StringRef MainFile, const SymbolIndex &Index) {
+ RefsRequest RQuest;
+ RQuest.IDs.insert(*getSymbolID(&RenameDecl));
+
+ // 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) {
+ // FIXME: !positionToOffset is O(N), optimize it.
+ 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::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
+ 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;
+ for (const auto &FileAndOccurrences : AffectedFiles) {
+ llvm::StringRef FilePath = FileAndOccurrences.first();
+
+ auto AffectedFileCode = GetFileContent(FilePath);
+ if (!AffectedFileCode) {
+ elog("Fail to read file content: {0}", AffectedFileCode.takeError());
+ continue;
+ }
+
+ 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(const RenameInputs &RInputs) {
+ ParsedAST &AST = RInputs.AST;
+ const SourceManager &SM = AST.getSourceManager();
+ llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());
+ auto GetFileContent = [&RInputs,
+ &SM](PathRef AbsPath) -> llvm::Expected<std::string> {
+ llvm::Optional<std::string> DirtyBuffer;
+ if (RInputs.GetDirtyBuffer &&
+ (DirtyBuffer = RInputs.GetDirtyBuffer(AbsPath)))
+ return std::move(*DirtyBuffer);
+
+ auto Content =
+ SM.getFileManager().getVirtualFileSystem().getBufferForFile(AbsPath);
+ if (!Content)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ llvm::formatv("Fail to open file {0}: {1}", AbsPath,
+ Content.getError().message()));
+ if (!*Content)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ llvm::formatv("Got no buffer for file {0}", AbsPath));
+
+ return (*Content)->getBuffer().str();
+ };
+ 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(ReasonToReject::UnsupportedSymbol);
+
+ auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
+ if (DeclsUnderCursor.empty())
+ return makeError(ReasonToReject::NoSymbolFound);
+ if (DeclsUnderCursor.size() > 1)
+ return makeError(ReasonToReject::AmbiguousSymbol);
+
+ const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
+ if (!RenameDecl)
+ return makeError(ReasonToReject::UnsupportedSymbol);
+
+ 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{MainFileCode, std::move(*MainFileRenameEdit)})});
+ }
+
+ FileEdits Results;
+ // renameable safely guards us that at this point we are renaming a local
+ // symbol if we don't have index,
+ if (RInputs.Index) {
+ auto OtherFilesEdits =
+ renameOutsideFile(*RenameDecl, RInputs.MainFilePath, RInputs.NewName,
+ *RInputs.Index, GetFileContent);
+ if (!OtherFilesEdits)
+ return OtherFilesEdits.takeError();
+ Results = std::move(*OtherFilesEdits);
+ }
+ // Attach the rename edits for the main file.
+ Results.try_emplace(RInputs.MainFilePath, MainFileCode,
+ std::move(*MainFileRenameEdit));
+ return Results;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index 63a1ffe32150..1427d7042585 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/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,32 @@ namespace clangd {
class ParsedAST;
class SymbolIndex;
-/// 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);
+/// Gets dirty buffer for a given file \p AbsPath.
+/// Returns None if there is no dirty buffer for the given file.
+using DirtyBufferGetter =
+ llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
+
+struct RenameInputs {
+ Position Pos; // the position triggering the rename
+ llvm::StringRef NewName;
+
+ ParsedAST &AST;
+ llvm::StringRef MainFilePath;
+
+ const SymbolIndex *Index = nullptr;
+
+ bool AllowCrossFile = false;
+ // When set, used by the rename to get file content for all rename-related
+ // files.
+ // If there is no corresponding dirty buffer, we will use the file content
+ // from disk.
+ DirtyBufferGetter GetDirtyBuffer = nullptr;
+};
+
+/// Renames all occurrences of the symbol.
+/// If AllowCrossFile is false, returns an error if rename a symbol that's used
+/// in another file (per the index).
+llvm::Expected<FileEdits> rename(const RenameInputs &RInputs);
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/refactor/Tweak.h b/clang-tools-extra/clangd/refactor/Tweak.h
index de655abd98c7..69ac4ad612e9 100644
--- a/clang-tools-extra/clangd/refactor/Tweak.h
+++ b/clang-tools-extra/clangd/refactor/Tweak.h
@@ -77,9 +77,7 @@ class Tweak {
struct Effect {
/// A message to be displayed to the user.
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;
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 2639df31dbe8..608a2da68134 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -264,6 +264,16 @@ list<std::string> TweakList{
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 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
}
Opts.StaticIndex = StaticIdx.get();
Opts.AsyncThreadsCount = WorkerThreadsCount;
+ Opts.CrossFileRename = CrossFileRename;
clangd::CodeCompleteOptions CCOpts;
CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 8dedcf579fd3..75b15e735abf 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/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,8 +20,45 @@ namespace clang {
namespace clangd {
namespace {
-MATCHER_P2(RenameRange, Code, Range, "") {
- return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+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());
+}
+
+std::vector<
+ std::pair</*InitialCode*/ std::string, /*CodeAfterRename*/ std::string>>
+applyEdits(FileEdits FE) {
+ std::vector<std::pair<std::string, std::string>> Results;
+ for (auto &It : FE)
+ Results.emplace_back(
+ It.first().str(),
+ llvm::cantFail(tooling::applyAllReplacements(
+ It.getValue().InitialCode, It.getValue().Replacements)));
+ return Results;
}
// Generates an expected rename result by replacing all ranges in the given
@@ -363,11 +402,11 @@ TEST(RenameTest, WithinFileRename) {
llvm::StringRef NewName = "abcde";
for (const auto &RenamePos : Code.points()) {
auto RenameResult =
- renameWithinFile(AST, testPath(TU.Filename), RenamePos, NewName);
- ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
- auto ApplyResult = llvm::cantFail(
- tooling::applyAllReplacements(Code.code(), *RenameResult));
- EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ rename({RenamePos, NewName, AST, testPath(TU.Filename)});
+ ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+ ASSERT_EQ(1u, RenameResult->size());
+ EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+ expectedResult(Code, NewName));
}
}
}
@@ -480,23 +519,23 @@ TEST(RenameTest, Renameable) {
}
auto AST = TU.build();
llvm::StringRef NewName = "dummyNewName";
- auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
- NewName, Case.Index);
+ auto Results =
+ rename({T.point(), NewName, AST, 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();
+ << "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());
- auto ApplyResult =
- llvm::cantFail(tooling::applyAllReplacements(T.code(), *Results));
- EXPECT_EQ(expectedResult(T, NewName), ApplyResult);
+ ASSERT_EQ(1u, Results->size());
+ EXPECT_EQ(applyEdits(std::move(*Results)).front().second,
+ expectedResult(T, NewName));
}
}
}
@@ -522,11 +561,81 @@ TEST(RenameTest, MainFileReferencesOnly) {
llvm::StringRef NewName = "abcde";
auto RenameResult =
- renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+ rename({Code.point(), NewName, AST, testPath(TU.Filename)});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
- auto ApplyResult =
- llvm::cantFail(tooling::applyAllReplacements(Code.code(), *RenameResult));
- EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ ASSERT_EQ(1u, RenameResult->size());
+ EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+ expectedResult(Code, NewName));
+}
+
+TEST(RenameTests, CrossFile) {
+ Annotations FooCode("class [[Foo]] {};");
+ std::string FooPath = testPath("foo.cc");
+ Annotations FooDirtyBuffer("class [[Foo]] {};\n// this is dirty buffer");
+ 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);
+
+ Annotations MainCode("class [[Fo^o]] {};");
+ auto MainFilePath = testPath("main.cc");
+ // Dirty buffer for foo.cc.
+ auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
+ if (Path == FooPath)
+ return FooDirtyBuffer.code().str();
+ return llvm::None;
+ };
+
+ // 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();
+ llvm::StringRef NewName = "newName";
+ auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
+ Index.get(), /*CrossFile=*/true, GetDirtyBuffer});
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(
+ applyEdits(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(Eq(FooPath), Eq(expectedResult(FooDirtyBuffer, NewName))),
+ Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
+
+ // 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]](); }");
+ TU = TestTU::withCode(MainCode.code());
+ // Set a file "bar.cc" on disk.
+ TU.AdditionalFiles["bar.cc"] = BarCode.code();
+ AST = TU.build();
+ Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(),
+ /*CrossFile=*/true, GetDirtyBuffer});
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(
+ applyEdits(std::move(*Results)),
+ UnorderedElementsAre(
+ Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
+ Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
+}
+
+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();
+ llvm::StringRef NewName = "newName";
+ auto Results = rename({Code.point(), NewName, AST, Path});
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(
+ applyEdits(std::move(*Results)),
+ UnorderedElementsAre(Pair(Eq(Path), Eq(expectedResult(Code, NewName)))));
}
} // namespace
diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.cpp b/clang-tools-extra/clangd/unittests/SyncAPI.cpp
index 812fa7a0f2ec..085eacd42fee 100644
--- a/clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ b/clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@ runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos) {
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;
+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, capture(Result));
return std::move(*Result);
}
diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.h b/clang-tools-extra/clangd/unittests/SyncAPI.h
index 5ffed1fbb120..55a538ef6a97 100644
--- a/clang-tools-extra/clangd/unittests/SyncAPI.h
+++ b/clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@ runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos);
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);
More information about the cfe-commits
mailing list