hokein updated this revision to Diff 205963.
hokein added a comment.
Improve the tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63426/new/
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
clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
Index: clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -74,6 +74,8 @@
std::move(NewName));
}
+const NamedDecl *RenameOccurrences::getRenameDecl() const { return ND; }
+
Expected<AtomicChanges>
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
Expected<SymbolOccurrences> Occurrences = findSymbolOccurrences(ND, Context);
Index: clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
+++ clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
@@ -54,6 +54,8 @@
static const RefactoringDescriptor &describe();
+ const NamedDecl *getRenameDecl() const;
+
private:
RenameOccurrences(const NamedDecl *ND, std::string NewName)
: ND(ND), NewName(std::move(NewName)) {}
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,67 @@
}
}
+TEST(RenameTest, Renameable) {
+ // Test cases where the symbol is declared in header.
+ const char *Headers[] = {
+ R"cpp(// allow -- function-local
+ void f(int [[Lo^cal]]) {
+ [[Local]] = 2;
+ }
+ )cpp",
+
+ R"cpp(// allow -- symbol is indexable and has no refs in index.
+ void [[On^lyInThisFile]]();
+ )cpp",
+
+ R"cpp(// disallow -- symbol is indexable and has other refs in index.
+ void f() {
+ Out^side s;
+ }
+ )cpp",
+
+ R"cpp(// disallow -- symbol is not indexable.
+ namespace {
+ class Unin^dexable {};
+ }
+ )cpp",
+ };
+ const char *CommonHeader = "class Outside {};";
+ TestTU OtherFile = TestTU::withCode("Outside s;");
+ OtherFile.HeaderCode = CommonHeader;
+ OtherFile.Filename = "other.cc";
+ // The index has a "Outside" reference.
+ auto OtherFileIndex = OtherFile.index();
+
+ for (const char *Header : Headers) {
+ Annotations T(Header);
+ // We open the .h file as the main file.
+ TestTU TU = TestTU::withCode(T.code());
+ TU.Filename = "test.h";
+ TU.HeaderCode = CommonHeader;
+ TU.ExtraArgs.push_back("-xc++");
+ auto AST = TU.build();
+
+ auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
+ "dummyNewName", OtherFileIndex.get());
+ bool WantRename = true;
+ if (T.ranges().empty())
+ WantRename = false;
+ if (!WantRename) {
+ EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
+ << T.code();
+ llvm::consumeError(Results.takeError());
+ } else {
+ EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+ << llvm::toString(Results.takeError());
+ std::vector<testing::Matcher<tooling::Replacement>> Expected;
+ for (const auto &R : T.ranges())
+ Expected.push_back(RenameRange(TU.Code, R));
+ EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
+ }
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
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,6 +7,9 @@
//===----------------------------------------------------------------------===//
#include "refactor/Rename.h"
+#include "AST.h"
+#include "Logger.h"
+#include "index/SymbolCollector.h"
#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
@@ -46,11 +49,95 @@
return Err;
}
+llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
+ llvm::StringRef HintFilePath) {
+ if (!Loc)
+ return None;
+ auto Uri = URI::parse(Loc.FileURI);
+ if (!Uri) {
+ elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
+ return None;
+ }
+ auto U = URIForFile::fromURI(*Uri, HintFilePath);
+ if (!U) {
+ elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError());
+ return None;
+ }
+ return U->file().str();
+}
+
+// Query the index to get other file where the Decl is referenced.
+llvm::Optional<std::string> getOtherRefFile(const NamedDecl &Decl,
+ StringRef MainFile,
+ const SymbolIndex &Index) {
+ RefsRequest Req;
+ // We limit the number of results, this is a correctness/performance
+ // tradeoff. We expect the number of symbol references in the current file
+ // is smaller than the limit.
+ Req.Limit = 100;
+ if (auto ID = getSymbolID(&Decl))
+ Req.IDs.insert(*ID);
+ llvm::Optional<std::string> OtherFile;
+ Index.refs(Req, [&](const Ref &R) {
+ if (OtherFile)
+ return;
+ if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
+ if (*RefFilePath != MainFile)
+ OtherFile = *RefFilePath;
+ }
+ });
+ return OtherFile;
+}
+
+enum ReasonToReject {
+ NoIndexProvided,
+ NonIndexable,
+ UsedOutsideFile,
+};
+
+// Check the symbol Decl is renameable, details are:
+// 1. symbol declared in the main file (not a header) => allow
+// 2. symbol declared in the header
+// 1) symbol is function-local => allow
+// 2) symbol is not indexable => disallow
+// 3) symbol is indexable and has no refs in index => allow
+// 4) symbol is indexable and has refs from other files => disallow (for
+// now)
+llvm::Optional<ReasonToReject>
+renamable(const NamedDecl &Decl, StringRef MainFile, const SymbolIndex *Index) {
+ auto &ASTCtx = Decl.getASTContext();
+ const auto &SM = ASTCtx.getSourceManager();
+ bool MainFileIsHeader = llvm::sys::path::extension(MainFile) == ".h";
+ bool DeclaredInMainFile =
+ SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation()));
+
+ bool DeclaredInHeader =
+ !DeclaredInMainFile || (DeclaredInMainFile && MainFileIsHeader);
+ if (!DeclaredInHeader)
+ return None; // Case 1
+
+ if (Decl.getParentFunctionOrMethod())
+ return None; // Case 2.1: function-local
+
+ if (!Index)
+ return ReasonToReject::NoIndexProvided;
+
+ bool IsIndexable =
+ SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false);
+ if (!IsIndexable)
+ return ReasonToReject::NonIndexable; // Case 2.2
+ // query index for xrefs. (not support for now)
+ auto OtherFile = getOtherRefFile(Decl, MainFile, *Index);
+ if (!OtherFile)
+ return None; // Case 2.3
+ return ReasonToReject::UsedOutsideFile; // Case 2.4
+}
+
} // 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(
@@ -62,6 +149,25 @@
if (!Rename)
return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics());
+ const auto *RenameDecl = Rename->getRenameDecl();
+ assert(RenameDecl && "symbol must be found at this point");
+ if (auto Reject = renamable(*RenameDecl, File, Index)) {
+ auto Message = [](ReasonToReject Reason) {
+ switch (Reason) {
+ case NoIndexProvided:
+ return "no index is provided";
+ case UsedOutsideFile:
+ return "the symbol is used outside main file";
+ case NonIndexable:
+ return "the symbol is not indexable";
+ }
+ llvm_unreachable("unhandled reason kind");
+ };
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("reject to rename: {0}", Message(*Reject)),
+ llvm::inconvertibleErrorCode());
+ }
+
Rename->invoke(ResultCollector, Context);
assert(ResultCollector.Result.hasValue());
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -277,12 +277,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());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
