[clang-tools-extra] 4f80fc2 - [clangd] Implement rename by using SelectionTree and findExplicitReferences.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 18 07:18:36 PST 2019
Author: Haojian Wu
Date: 2019-11-18T16:16:47+01:00
New Revision: 4f80fc2491cc35730a9a84b86975278b7daa8522
URL: https://github.com/llvm/llvm-project/commit/4f80fc2491cc35730a9a84b86975278b7daa8522
DIFF: https://github.com/llvm/llvm-project/commit/4f80fc2491cc35730a9a84b86975278b7daa8522.diff
LOG: [clangd] Implement rename by using SelectionTree and findExplicitReferences.
Summary:
With the new implemenation, we will have better coverage of various AST
nodes, and fix some known/potential bugs.
Also added the existing clang-renamae tests. Known changed behavior:
- "~Fo^o()" will not trigger the rename, will fix afterwards
- references in macro bodies are not renamed now
This fixes:
- https://github.com/clangd/clangd/issues/167
- https://github.com/clangd/clangd/issues/169
- https://github.com/clangd/clangd/issues/171
Reviewers: ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D69934
Added:
Modified:
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 3969f3e2e4e2..fb83083384f9 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -8,14 +8,16 @@
#include "refactor/Rename.h"
#include "AST.h"
+#include "FindTarget.h"
#include "Logger.h"
#include "ParsedAST.h"
+#include "Selection.h"
#include "SourceCode.h"
#include "index/SymbolCollector.h"
-#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
-#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
-#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
namespace clang {
namespace clangd {
@@ -34,6 +36,17 @@ llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
return *Path;
}
+// Returns true if the given location is expanded from any macro body.
+bool isInMacroBody(const SourceManager &SM, SourceLocation Loc) {
+ while (Loc.isMacroID()) {
+ if (SM.isMacroBodyExpansion(Loc))
+ return true;
+ Loc = SM.getImmediateMacroCallerLoc(Loc);
+ }
+
+ return false;
+}
+
// Query the index to find some other files where the Decl is referenced.
llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
const SymbolIndex &Index) {
@@ -56,12 +69,41 @@ llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
return OtherFile;
}
+llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
+ SourceLocation TokenStartLoc) {
+ unsigned Offset =
+ AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
+
+ SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
+ const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
+ if (!SelectedNode)
+ return {};
+
+ // If the location points to a Decl, we check it is actually on the name
+ // range of the Decl. This would avoid allowing rename on unrelated tokens.
+ // ^class Foo {} // SelectionTree returns CXXRecordDecl,
+ // // we don't attempt to trigger rename on this position.
+ // FIXME: make this work on destructors, e.g. "~F^oo()".
+ if (const auto *D = SelectedNode->ASTNode.get<Decl>()) {
+ if (D->getLocation() != TokenStartLoc)
+ return {};
+ }
+
+ llvm::DenseSet<const Decl *> Result;
+ for (const auto *D :
+ targetDecl(SelectedNode->ASTNode,
+ DeclRelation::Alias | DeclRelation::TemplatePattern))
+ Result.insert(D);
+ return Result;
+}
+
enum ReasonToReject {
NoSymbolFound,
NoIndexProvided,
NonIndexable,
UsedOutsideFile,
UnsupportedSymbol,
+ AmbiguousSymbol,
};
// Check the symbol Decl is renameable (per the index) within the file.
@@ -125,6 +167,8 @@ llvm::Error makeError(ReasonToReject Reason) {
return "symbol may be used in other files (not eligible for indexing)";
case UnsupportedSymbol:
return "symbol is not a supported kind (e.g. namespace, macro)";
+ case AmbiguousSymbol:
+ return "there are multiple symbols at the given location";
}
llvm_unreachable("unhandled reason kind");
};
@@ -134,22 +178,38 @@ llvm::Error makeError(ReasonToReject Reason) {
}
// Return all rename occurrences in the main file.
-tooling::SymbolOccurrences
-findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
- const NamedDecl *CanonicalRenameDecl =
- tooling::getCanonicalSymbolDeclaration(RenameDecl);
- assert(CanonicalRenameDecl && "RenameDecl must be not null");
- std::vector<std::string> RenameUSRs =
- tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext());
- std::string OldName = CanonicalRenameDecl->getNameAsString();
- tooling::SymbolOccurrences Result;
+std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
+ const NamedDecl &ND) {
+ // In theory, locateDeclAt should return the primary template. However, if the
+ // cursor is under the underlying CXXRecordDecl of the ClassTemplateDecl, ND
+ // will be the CXXRecordDecl, for this case, we need to get the primary
+ // template maunally.
+ const auto &RenameDecl =
+ ND.getDescribedTemplate() ? *ND.getDescribedTemplate() : ND;
+ // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
+ // overriddens, primary template and all explicit specializations.
+ // FIXME: get rid of the remaining tooling APIs.
+ std::vector<std::string> RenameUSRs = tooling::getUSRsForDeclaration(
+ tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
+ llvm::DenseSet<SymbolID> TargetIDs;
+ for (auto &USR : RenameUSRs)
+ TargetIDs.insert(SymbolID(USR));
+
+ std::vector<SourceLocation> Results;
for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
- tooling::SymbolOccurrences RenameInDecl =
- tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);
- Result.insert(Result.end(), std::make_move_iterator(RenameInDecl.begin()),
- std::make_move_iterator(RenameInDecl.end()));
+ findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
+ if (Ref.Targets.empty())
+ return;
+ for (const auto *Target : Ref.Targets) {
+ auto ID = getSymbolID(Target);
+ if (!ID || TargetIDs.find(*ID) == TargetIDs.end())
+ return;
+ }
+ Results.push_back(Ref.NameLoc);
+ });
}
- return Result;
+
+ return Results;
}
} // namespace
@@ -165,30 +225,41 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
return makeError(UnsupportedSymbol);
- const auto *RenameDecl =
- tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg);
- if (!RenameDecl)
+ auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
+ if (DeclsUnderCursor.empty())
return makeError(NoSymbolFound);
+ if (DeclsUnderCursor.size() > 1)
+ return makeError(AmbiguousSymbol);
+
+ const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
+ if (!RenameDecl)
+ return makeError(UnsupportedSymbol);
if (auto Reject =
renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
return makeError(*Reject);
- // Rename sometimes returns duplicate edits (which is a bug). A side-effect of
- // adding them to a single Replacements object is these are deduplicated.
tooling::Replacements FilteredChanges;
- for (const tooling::SymbolOccurrence &Rename :
- findOccurrencesWithinFile(AST, RenameDecl)) {
- // Currently, we only support normal rename (one range) for C/C++.
- // FIXME: support multiple-range rename for objective-c methods.
- if (Rename.getNameRanges().size() > 1)
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ SourceLocation RenameLoc = Loc;
+ // We don't rename in any macro bodies, but we allow rename the symbol
+ // spelled in a top-level macro argument in the main file.
+ if (RenameLoc.isMacroID()) {
+ if (isInMacroBody(SM, RenameLoc))
+ continue;
+ RenameLoc = SM.getSpellingLoc(Loc);
+ }
+ // Filter out locations not from main file.
+ // We traverse only main file decls, but locations could come from an
+ // non-preamble #include file e.g.
+ // void test() {
+ // int f^oo;
+ // #include "use_foo.inc"
+ // }
+ if (!isInsideMainFile(RenameLoc, SM))
continue;
- // We shouldn't have conflicting replacements. If there are conflicts, it
- // means that we have bugs either in clangd or in Rename library, therefore
- // we refuse to perform the rename.
if (auto Err = FilteredChanges.add(tooling::Replacement(
- AST.getASTContext().getSourceManager(),
- CharSourceRange::getCharRange(Rename.getNameRanges()[0]), NewName)))
+ SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
return std::move(Err);
}
return FilteredChanges;
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 212d9c089f92..3c3c60500f9d 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -38,27 +38,27 @@ std::string expectedResult(Annotations Test, llvm::StringRef NewName) {
return Result;
}
-TEST(RenameTest, SingleFile) {
- // "^" points to the position of the rename, and "[[]]" ranges point to the
+TEST(RenameTest, WithinFileRename) {
+ // rename is runnning on all "^" points, and "[[]]" ranges point to the
// identifier that is being renamed.
llvm::StringRef Tests[] = {
- // Rename function.
+ // Function.
R"cpp(
- void [[foo]]() {
+ void [[foo^]]() {
[[fo^o]]();
}
)cpp",
- // Rename type.
+ // Type.
R"cpp(
- struct [[foo]]{};
+ struct [[foo^]] {};
[[foo]] test() {
[[f^oo]] x;
return x;
}
)cpp",
- // Rename variable.
+ // Local variable.
R"cpp(
void bar() {
if (auto [[^foo]] = 5) {
@@ -66,18 +66,313 @@ TEST(RenameTest, SingleFile) {
}
}
)cpp",
+
+ // Rename class, including constructor/destructor.
+ R"cpp(
+ class [[F^oo]] {
+ [[F^oo]]();
+ ~[[Foo]]();
+ void foo(int x);
+ };
+ [[Foo]]::[[Fo^o]]() {}
+ void [[Foo]]::foo(int x) {}
+ )cpp",
+
+ // Class in template argument.
+ R"cpp(
+ class [[F^oo]] {};
+ template <typename T> void func();
+ template <typename T> class Baz {};
+ int main() {
+ func<[[F^oo]]>();
+ Baz<[[F^oo]]> obj;
+ return 0;
+ }
+ )cpp",
+
+ // Forward class declaration without definition.
+ R"cpp(
+ class [[F^oo]];
+ [[Foo]] *f();
+ )cpp",
+
+ // Class methods overrides.
+ R"cpp(
+ struct A {
+ virtual void [[f^oo]]() {}
+ };
+ struct B : A {
+ void [[f^oo]]() override {}
+ };
+ struct C : B {
+ void [[f^oo]]() override {}
+ };
+
+ void func() {
+ A().[[f^oo]]();
+ B().[[f^oo]]();
+ C().[[f^oo]]();
+ }
+ )cpp",
+
+ // Template class (partial) specializations.
+ R"cpp(
+ template <typename T>
+ class [[F^oo]] {};
+
+ template<>
+ class [[F^oo]]<bool> {};
+ template <typename T>
+ class [[F^oo]]<T*> {};
+
+ void test() {
+ [[Foo]]<int> x;
+ [[Foo]]<bool> y;
+ [[Foo]]<int*> z;
+ }
+ )cpp",
+
+ // Template class instantiations.
+ R"cpp(
+ template <typename T>
+ class [[F^oo]] {
+ public:
+ T foo(T arg, T& ref, T* ptr) {
+ T value;
+ int number = 42;
+ value = (T)number;
+ value = static_cast<T>(number);
+ return value;
+ }
+ static void foo(T value) {}
+ T member;
+ };
+
+ template <typename T>
+ void func() {
+ [[F^oo]]<T> obj;
+ obj.member = T();
+ [[Foo]]<T>::foo();
+ }
+
+ void test() {
+ [[F^oo]]<int> i;
+ i.member = 0;
+ [[F^oo]]<int>::foo(0);
+
+ [[F^oo]]<bool> b;
+ b.member = false;
+ [[Foo]]<bool>::foo(false);
+ }
+ )cpp",
+
+ // Template class methods.
+ R"cpp(
+ template <typename T>
+ class A {
+ public:
+ void [[f^oo]]() {}
+ };
+
+ void func() {
+ A<int>().[[f^oo]]();
+ A<double>().[[f^oo]]();
+ A<float>().[[f^oo]]();
+ }
+ )cpp",
+
+ // Complicated class type.
+ R"cpp(
+ // Forward declaration.
+ class [[Fo^o]];
+ class Baz {
+ virtual int getValue() const = 0;
+ };
+
+ class [[F^oo]] : public Baz {
+ public:
+ [[Foo]](int value = 0) : x(value) {}
+
+ [[Foo]] &operator++(int);
+
+ bool operator<([[Foo]] const &rhs);
+ int getValue() const;
+ private:
+ int x;
+ };
+
+ void func() {
+ [[Foo]] *Pointer = 0;
+ [[Foo]] Variable = [[Foo]](10);
+ for ([[Foo]] it; it < Variable; it++);
+ const [[Foo]] *C = new [[Foo]]();
+ const_cast<[[Foo]] *>(C)->getValue();
+ [[Foo]] foo;
+ const Baz &BazReference = foo;
+ const Baz *BazPointer = &foo;
+ dynamic_cast<const [[^Foo]] &>(BazReference).getValue();
+ dynamic_cast<const [[^Foo]] *>(BazPointer)->getValue();
+ reinterpret_cast<const [[^Foo]] *>(BazPointer)->getValue();
+ static_cast<const [[^Foo]] &>(BazReference).getValue();
+ static_cast<const [[^Foo]] *>(BazPointer)->getValue();
+ }
+ )cpp",
+
+ // CXXConstructor initializer list.
+ R"cpp(
+ class Baz {};
+ class Qux {
+ Baz [[F^oo]];
+ public:
+ Qux();
+ };
+ Qux::Qux() : [[F^oo]]() {}
+ )cpp",
+
+ // DeclRefExpr.
+ R"cpp(
+ class C {
+ public:
+ static int [[F^oo]];
+ };
+
+ int foo(int x);
+ #define MACRO(a) foo(a)
+
+ void func() {
+ C::[[F^oo]] = 1;
+ MACRO(C::[[Foo]]);
+ int y = C::[[F^oo]];
+ }
+ )cpp",
+
+ // Macros.
+ R"cpp(
+ // no rename inside macro body.
+ #define M1 foo
+ #define M2(x) x
+ int [[fo^o]]();
+ void boo(int);
+
+ void qoo() {
+ [[foo]]();
+ boo([[foo]]());
+ M1();
+ boo(M1());
+ M2([[foo]]());
+ M2(M1()); // foo is inside the nested macro body.
+ }
+ )cpp",
+
+ // MemberExpr in macros
+ R"cpp(
+ class Baz {
+ public:
+ int [[F^oo]];
+ };
+ int qux(int x);
+ #define MACRO(a) qux(a)
+
+ int main() {
+ Baz baz;
+ baz.[[Foo]] = 1;
+ MACRO(baz.[[Foo]]);
+ int y = baz.[[Foo]];
+ }
+ )cpp",
+
+ // Template parameters.
+ R"cpp(
+ template <typename [[^T]]>
+ class Foo {
+ [[T]] foo([[T]] arg, [[T]]& ref, [[^T]]* ptr) {
+ [[T]] value;
+ int number = 42;
+ value = ([[T]])number;
+ value = static_cast<[[^T]]>(number);
+ return value;
+ }
+ static void foo([[T]] value) {}
+ [[T]] member;
+ };
+ )cpp",
+
+ // Typedef.
+ R"cpp(
+ namespace std {
+ class basic_string {};
+ typedef basic_string [[s^tring]];
+ } // namespace std
+
+ std::[[s^tring]] foo();
+ )cpp",
+
+ // Variable.
+ R"cpp(
+ namespace A {
+ int [[F^oo]];
+ }
+ int Foo;
+ int Qux = Foo;
+ int Baz = A::[[^Foo]];
+ void fun() {
+ struct {
+ int Foo;
+ } b = {100};
+ int Foo = 100;
+ Baz = Foo;
+ {
+ extern int Foo;
+ Baz = Foo;
+ Foo = A::[[F^oo]] + Baz;
+ A::[[Fo^o]] = b.Foo;
+ }
+ Foo = b.Foo;
+ }
+ )cpp",
+
+ // Namespace alias.
+ R"cpp(
+ namespace a { namespace b { void foo(); } }
+ namespace [[^x]] = a::b;
+ void bar() {
+ [[x]]::foo();
+ }
+ )cpp",
+
+ // Scope enums.
+ R"cpp(
+ enum class [[K^ind]] { ABC };
+ void ff() {
+ [[K^ind]] s;
+ s = [[Kind]]::ABC;
+ }
+ )cpp",
+
+ // template class in template argument list.
+ R"cpp(
+ template<typename T>
+ class [[Fo^o]] {};
+ template <template<typename> class Z> struct Bar { };
+ template <> struct Bar<[[Foo]]> {};
+ )cpp",
};
for (const auto T : Tests) {
Annotations Code(T);
auto TU = TestTU::withCode(Code.code());
auto AST = TU.build();
+ EXPECT_TRUE(AST.getDiagnostics().empty())
+ << AST.getDiagnostics().front() << Code.code();
+
llvm::StringRef NewName = "abcde";
- auto RenameResult =
- renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
- ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
- auto ApplyResult = llvm::cantFail(
- tooling::applyAllReplacements(Code.code(), *RenameResult));
- EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ for (const auto &RenamePos : Code.points()) {
+ auto RenameResult =
+ renameWithinFile(AST, testPath(TU.Filename), RenamePos, NewName);
+ ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
+ auto ApplyResult = llvm::cantFail(
+ tooling::applyAllReplacements(Code.code(), *RenameResult));
+ EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ }
}
}
@@ -148,12 +443,32 @@ TEST(RenameTest, Renameable) {
{R"cpp(// foo is declared outside the file.
void fo^o() {}
- )cpp", "used outside main file", !HeaderFile /*cc file*/, Index},
+ )cpp",
+ "used outside main file", !HeaderFile /*cc file*/, Index},
{R"cpp(
// We should detect the symbol is used outside the file from the AST.
void fo^o() {})cpp",
"used outside main file", !HeaderFile, nullptr /*no index*/},
+
+ {R"cpp(
+ void foo(int);
+ void foo(char);
+ template <typename T> void f(T t) {
+ fo^o(t);
+ })cpp",
+ "multiple symbols", !HeaderFile, nullptr /*no index*/},
+
+ {R"cpp(// disallow rename on unrelated token.
+ cl^ass Foo {};
+ )cpp",
+ "no symbol", !HeaderFile, nullptr},
+
+ {R"cpp(// disallow rename on unrelated token.
+ temp^late<typename T>
+ class Foo {};
+ )cpp",
+ "no symbol", !HeaderFile, nullptr},
};
for (const auto& Case : Cases) {
@@ -191,6 +506,36 @@ TEST(RenameTest, Renameable) {
}
}
+TEST(RenameTest, MainFileReferencesOnly) {
+ // filter out references not from main file.
+ llvm::StringRef Test =
+ R"cpp(
+ void test() {
+ int [[fo^o]] = 1;
+ // rename references not from main file are not included.
+ #include "foo.inc"
+ })cpp";
+
+ Annotations Code(Test);
+ auto TU = TestTU::withCode(Code.code());
+ TU.AdditionalFiles["foo.inc"] = R"cpp(
+ #define Macro(X) X
+ &Macro(foo);
+ &foo;
+ )cpp";
+ auto AST = TU.build();
+ EXPECT_TRUE(AST.getDiagnostics().empty())
+ << AST.getDiagnostics().front() << Code.code();
+ llvm::StringRef NewName = "abcde";
+
+ auto RenameResult =
+ renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+ ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
+ auto ApplyResult =
+ llvm::cantFail(tooling::applyAllReplacements(Code.code(), *RenameResult));
+ EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list