kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay.
Herald added a project: clang.
In some cases the candidate ranges for rename final stage (textual
replacements) are invalid and do not contain references to identifier being
renamed. Examples of such behavior include implicit references that are
currently not filtered out (though in the future they should be dealt with
during the references collection stage).
This patch addresses this issue by explicitly checking whether the text in each
candidate range is equivalent to the renamed identifier's name. It does not make
index-based rename absolutely correct, but it is a cheap way to filter out some
replacements that are clearly incorrect.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72071
Files:
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
@@ -795,6 +795,7 @@
void func() {
[[Foo]] foo;
+ foo.~[[Foo]];
}
)cpp",
},
@@ -868,6 +869,20 @@
}
)cpp",
},
+ {
+ // Macros and implicit references.
+ R"cpp(
+ class [[Fo^o]] {};
+ #define FooFoo Foo
+ )cpp",
+ R"cpp(
+ #include "foo.h"
+ void bar() {
+ [[Foo]] foo;
+ FooFoo z;
+ }
+ )cpp",
+ },
};
for (const auto& T : Cases) {
@@ -920,7 +935,7 @@
auto LSPRange = Code.range();
llvm::StringRef FilePath = "/test/TestTU.cpp";
llvm::StringRef NewName = "abc";
- auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+ auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName, "😂");
ASSERT_TRUE(bool(Edit)) << Edit.takeError();
ASSERT_EQ(1UL, Edit->Replacements.size());
EXPECT_EQ(FilePath, Edit->Replacements.begin()->getFilePath());
@@ -928,7 +943,7 @@
// Test invalid range.
LSPRange.end = {10, 0}; // out of range
- Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+ Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName, "😂");
EXPECT_FALSE(Edit);
EXPECT_THAT(llvm::toString(Edit.takeError()),
testing::HasSubstr("fail to convert"));
@@ -939,7 +954,7 @@
[[range]]
[[range]]
)cpp");
- Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName);
+ Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName, "range");
ASSERT_TRUE(bool(Edit)) << Edit.takeError();
EXPECT_EQ(applyEdits(FileEdits{{T.code(), std::move(*Edit)}}).front().second,
expectedResult(T, NewName));
@@ -1013,11 +1028,6 @@
llvm::StringRef IndexedCode;
llvm::StringRef LexedCode;
} Tests[] = {
- {
- // no lexed ranges.
- "[[]]",
- "",
- },
{
// both line and column are changed, not a near miss.
R"([[]])",
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -55,7 +55,8 @@
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
llvm::StringRef InitialCode,
std::vector<Range> Occurrences,
- llvm::StringRef NewName);
+ llvm::StringRef NewName,
+ llvm::StringRef OldName);
/// Adjusts indexed occurrences to match the current state of the file.
///
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -374,7 +374,8 @@
llvm::inconvertibleErrorCode());
}
auto RenameEdit =
- buildRenameEdit(FilePath, *AffectedFileCode, *RenameRanges, NewName);
+ buildRenameEdit(FilePath, *AffectedFileCode, *RenameRanges, NewName,
+ RenameDecl.getNameAsString());
if (!RenameEdit) {
return llvm::make_error<llvm::StringError>(
llvm::formatv("fail to build rename edit for file {0}: {1}", FilePath,
@@ -522,7 +523,8 @@
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
llvm::StringRef InitialCode,
std::vector<Range> Occurrences,
- llvm::StringRef NewName) {
+ llvm::StringRef NewName,
+ llvm::StringRef OldName) {
assert(std::is_sorted(Occurrences.begin(), Occurrences.end()));
assert(std::unique(Occurrences.begin(), Occurrences.end()) ==
Occurrences.end() &&
@@ -557,7 +559,12 @@
auto EndOffset = Offset(R.end);
if (!EndOffset)
return EndOffset.takeError();
- OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
+ // FIXME: We should not generate invalid replacements in the first place,
+ // but it currently happens for implicit references and in some complicated
+ // cases where the might be some incorrect captured tokens.
+ if (*EndOffset > *StartOffset &&
+ InitialCode.slice(*StartOffset, *EndOffset) == OldName)
+ OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
}
tooling::Replacements RenameEdit;
@@ -600,10 +607,6 @@
assert(std::is_sorted(Indexed.begin(), Indexed.end()));
assert(std::is_sorted(Lexed.begin(), Lexed.end()));
- if (Indexed.size() > Lexed.size()) {
- vlog("The number of lexed occurrences is less than indexed occurrences");
- return llvm::None;
- }
// Fast check for the special subset case.
if (std::includes(Indexed.begin(), Indexed.end(), Lexed.begin(), Lexed.end()))
return Indexed.vec();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
