Skip to content

Commit

Permalink
[clangd] Abort rename when given the same name
Browse files Browse the repository at this point in the history
When user wants to rename the symbol to the same name we shouldn't do any work.
Bail out early and return error to save compute.

Resolves: clangd/clangd#580

Reviewed By: hokein

Differential Revision: https://reviews.llvm.org/D91134
  • Loading branch information
kirillbobyrev committed Nov 11, 2020
1 parent 898a81d commit 91ce6fb
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 3 deletions.
5 changes: 5 additions & 0 deletions clang-tools-extra/clangd/refactor/Rename.cpp
Expand Up @@ -123,6 +123,7 @@ enum class ReasonToReject {

// name validation.
RenameToKeywords,
SameName,
};

llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
Expand Down Expand Up @@ -213,6 +214,8 @@ llvm::Error makeError(ReasonToReject Reason) {
return "there are multiple symbols at the given location";
case ReasonToReject::RenameToKeywords:
return "the chosen name is a keyword";
case ReasonToReject::SameName:
return "new name is the same as the old name";
}
llvm_unreachable("unhandled reason kind");
};
Expand Down Expand Up @@ -556,6 +559,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return makeError(ReasonToReject::AmbiguousSymbol);
const auto &RenameDecl =
llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
if (RenameDecl.getName() == RInputs.NewName)
return makeError(ReasonToReject::SameName);
auto Invalid = checkName(RenameDecl, RInputs.NewName);
if (Invalid)
return makeError(*Invalid);
Expand Down
13 changes: 10 additions & 3 deletions clang-tools-extra/clangd/unittests/RenameTests.cpp
Expand Up @@ -774,6 +774,13 @@ TEST(RenameTest, Renameable) {
}
)cpp",
nullptr, !HeaderFile, nullptr, "Conflict"},

{R"cpp(// Trying to rename into the same name, SameName == SameName.
void func() {
int S^ameName;
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
};

for (const auto& Case : Cases) {
Expand Down Expand Up @@ -876,23 +883,23 @@ TEST(RenameTest, PrepareRename) {

auto Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
/*NewName=*/llvm::None, {/*CrossFile=*/true});
// verify that for multi-file rename, we only return main-file occurrences.
// Verify that for multi-file rename, we only return main-file occurrences.
ASSERT_TRUE(bool(Results)) << Results.takeError();
// We don't know the result is complete in prepareRename (passing a nullptr
// index internally), so GlobalChanges should be empty.
EXPECT_TRUE(Results->GlobalChanges.empty());
EXPECT_THAT(FooCC.ranges(),
testing::UnorderedElementsAreArray(Results->LocalChanges));

// verify name validation.
// Name validation.
Results =
runPrepareRename(Server, FooCCPath, FooCC.point(),
/*NewName=*/std::string("int"), {/*CrossFile=*/true});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("keyword"));

// single-file rename on global symbols, we should report an error.
// Single-file rename on global symbols, we should report an error.
Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
/*NewName=*/llvm::None, {/*CrossFile=*/false});
EXPECT_FALSE(Results);
Expand Down

0 comments on commit 91ce6fb

Please sign in to comment.