hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.
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).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63426
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Rename.h
clang-tools-extra/clangd/unittests/RenameTests.cpp
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -18,6 +18,10 @@
namespace clangd {
namespace {
+MATCHER_P2(RenameRange, Code, Range, "") {
+ return replacementToEdit(Code, arg).range == Range;
+}
+
TEST(RenameTest, SingleFile) {
struct Test {
const char* Before;
@@ -87,6 +91,69 @@
}
}
+TEST(RenameTest, WithIndex) {
+ const char *Tests[] = {
+ R"cpp(
+ class [[Foo]] {};
+ void f() {
+ [[Fo^o]] b;
+ }
+ )cpp",
+
+ R"cpp(
+ void f(int [[Lo^cal]]) {
+ [[Local]] = 2;
+ }
+ )cpp",
+
+ // Cases where we reject to do the rename.
+ R"cpp( // no symbol at the cursor
+ // This is in comme^nt.
+ )cpp",
+
+ R"cpp(
+ void f() { // Outside main file.
+ Out^side s;
+ }
+ )cpp",
+ };
+ const char *Header = "class Outside {};";
+
+ TestTU OtherFile = TestTU::withCode("Outside s;");
+ OtherFile.HeaderCode = Header;
+ OtherFile.Filename = "other.cc";
+
+ for (const char *Test : Tests) {
+ Annotations T(Test);
+ TestTU MainFile;
+ MainFile.Code = T.code();
+ MainFile.HeaderCode = Header;
+ auto AST = MainFile.build();
+ // Build a fake index containing refs of MainFile and OtherFile
+ auto OtherFileIndex = OtherFile.index();
+ auto MainFileIndex = MainFile.index();
+ auto Index = MergedIndex(MainFileIndex.get(), OtherFileIndex.get());
+
+ auto Results = renameWithinFile(AST, testPath(MainFile.Filename), T.point(),
+ "dummyNewName", &Index);
+ bool WantRename = true;
+ if (T.ranges().empty())
+ WantRename = false;
+ if (!WantRename) {
+ EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
+ << T.code();
+ llvm::consumeError(Results.takeError());
+ } else {
+ EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+ << llvm::toString(Results.takeError());
+ std::vector<testing::Matcher<tooling::Replacement>> Expected;
+ for (const auto &R : T.ranges())
+ Expected.push_back(RenameRange(MainFile.Code, R));
+ EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
+ }
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -18,10 +18,10 @@
/// Renames all occurrences of the symbol at \p Pos to \p NewName.
/// Occurrences outside the current file are not modified.
-llvm::Expected<tooling::Replacements> renameWithinFile(ParsedAST &AST,
- llvm::StringRef File,
- Position Pos,
- llvm::StringRef NewName);
+/// Only support renaming symbols not being used outside the file.
+llvm::Expected<tooling::Replacements>
+renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
+ llvm::StringRef NewName, const SymbolIndex *Index = nullptr);
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -7,8 +7,11 @@
//===----------------------------------------------------------------------===//
#include "refactor/Rename.h"
+#include "AST.h"
+#include "Logger.h"
#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
namespace clang {
namespace clangd {
@@ -46,15 +49,70 @@
return Err;
}
+static llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
+ llvm::StringRef HintFilePath) {
+ if (!Loc)
+ return None;
+ auto Uri = URI::parse(Loc.FileURI);
+ if (!Uri) {
+ elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
+ return None;
+ }
+ auto U = URIForFile::fromURI(*Uri, HintFilePath);
+ if (!U) {
+ elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError());
+ return None;
+ }
+ return U->file().str();
+}
+
} // namespace
llvm::Expected<tooling::Replacements>
renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
- llvm::StringRef NewName) {
+ llvm::StringRef NewName, const SymbolIndex *Index) {
RefactoringResultCollector ResultCollector;
ASTContext &ASTCtx = AST.getASTContext();
SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier(
AST, Pos, AST.getSourceManager().getMainFileID());
+
+ if (Index) {
+ // Consult the index to determine whether the symbol is used outside of
+ // the current file.
+ // FIXME: we may skip querying the index if D is function-local.
+ const NamedDecl *D =
+ clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg);
+ if (!D)
+ return llvm::make_error<llvm::StringError>(
+ "there is no symbol at the given location",
+ llvm::inconvertibleErrorCode());
+ RefsRequest Req;
+ // We limit the number of results, this is a correctness/performance
+ // tradeoff. We expect the number of symbol references in the current file
+ // is smaller than the limit.
+ Req.Limit = 100;
+ if (auto ID = getSymbolID(D))
+ Req.IDs.insert(*ID);
+ bool OutsideMainFile = false;
+ std::string OtherFile;
+ Index->refs(Req, [&](const Ref &R) {
+ if (OutsideMainFile)
+ return;
+ if (auto RefFilePath = filePath(R.Location, File)) {
+ if (*RefFilePath != File) {
+ OtherFile = *RefFilePath;
+ OutsideMainFile = true;
+ }
+ }
+ });
+ if (OutsideMainFile)
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv(
+ "renaming symbol used outside of the file is not supported: ",
+ OtherFile),
+ llvm::inconvertibleErrorCode());
+ }
+
tooling::RefactoringRuleContext Context(AST.getSourceManager());
Context.setASTContext(ASTCtx);
auto Rename = clang::tooling::RenameOccurrences::initiate(
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -272,12 +272,12 @@
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) {
+ auto Action = [Pos, this](Path File, std::string NewName,
+ Callback<std::vector<TextEdit>> CB,
+ llvm::Expected<InputsAndAST> InpAST) {
if (!InpAST)
return CB(InpAST.takeError());
- auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName);
+ auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
if (!Changes)
return CB(Changes.takeError());
std::vector<TextEdit> Edits;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
