Skip to content

Commit

Permalink
[clangd] Fix rename for symbol introduced by UsingDecl
Browse files Browse the repository at this point in the history
Fixes clangd/clangd#170

This patch actually consists of 2 fixes:

1) Add handling for UsingShadowDecl to canonicalRenameDecl().
   This fixes the issue described in clangd/clangd#170.

2) Avoid the "there are multiple symbols under the cursor error" by applying similar
   logic as in https://reviews.llvm.org/D133664.
   This also partly fixes clangd/clangd#586.

Differential Revision: https://reviews.llvm.org/D135489
  • Loading branch information
tom-anders committed Oct 9, 2022
1 parent 5593d36 commit ac21938
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
21 changes: 21 additions & 0 deletions clang-tools-extra/clangd/refactor/Rename.cpp
Expand Up @@ -133,6 +133,10 @@ const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
return canonicalRenameDecl(OriginalVD);
}
if (const auto *UD = dyn_cast<UsingShadowDecl>(D)) {
if (const auto *TargetDecl = UD->getTargetDecl())
return canonicalRenameDecl(TargetDecl);
}
return dyn_cast<NamedDecl>(D->getCanonicalDecl());
}

Expand All @@ -157,6 +161,22 @@ llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
return Result;
}

void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
// For something like
// namespace ns { void foo(); }
// void bar() { using ns::f^oo; foo(); }
// locateDeclAt() will return a UsingDecl and foo's actual declaration.
// For renaming, we're only interested in foo's declaration, so drop the other
// one. There should never be more than one UsingDecl here, otherwise the
// rename would be ambiguos anyway.
auto UD = std::find_if(Decls.begin(), Decls.end(), [](const NamedDecl *D) {
return llvm::isa<UsingDecl>(D);
});
if (UD != Decls.end()) {
Decls.erase(UD);
}
}

// By default, we exclude symbols from system headers and protobuf symbols as
// renaming these symbols would change system/generated files which are unlikely
// to be good candidates for modification.
Expand Down Expand Up @@ -737,6 +757,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return makeError(ReasonToReject::UnsupportedSymbol);

auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
filterRenameTargets(DeclsUnderCursor);
if (DeclsUnderCursor.empty())
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
Expand Down
31 changes: 31 additions & 0 deletions clang-tools-extra/clangd/unittests/RenameTests.cpp
Expand Up @@ -817,6 +817,29 @@ TEST(RenameTest, WithinFileRename) {
char [[da^ta]];
} @end
)cpp",

// Issue 170: Rename symbol introduced by UsingDecl
R"cpp(
namespace ns { void [[f^oo]](); }
using ns::[[f^oo]];
void f() {
[[f^oo]]();
auto p = &[[f^oo]];
}
)cpp",

// Issue 170: using decl that imports multiple overloads
// -> Only the overload under the cursor is renamed
R"cpp(
namespace ns { int [[^foo]](int); char foo(char); }
using ns::[[foo]];
void f() {
[[^foo]](42);
foo('x');
}
)cpp",
};
llvm::StringRef NewName = "NewName";
for (llvm::StringRef T : Tests) {
Expand Down Expand Up @@ -1062,6 +1085,14 @@ TEST(RenameTest, Renameable) {
};
)cpp",
"no symbol", false},

{R"cpp(// FIXME we probably want to rename both overloads here,
// but renaming currently assumes there's only a
// single canonical declaration.
namespace ns { int foo(int); char foo(char); }
using ns::^foo;
)cpp",
"there are multiple symbols at the given location", !HeaderFile},
};

for (const auto& Case : Cases) {
Expand Down

0 comments on commit ac21938

Please sign in to comment.