[clang-tools-extra] f805c60 - Revert "[clangd] Implement rename by using SelectionTree and findExplicitReferences."
Wolfgang Pieb via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 18 15:49:18 PST 2019
Author: Wolfgang Pieb
Date: 2019-11-18T15:39:05-08:00
New Revision: f805c60a093325c16ce4200d2615ef48555d9cb8
URL: https://github.com/llvm/llvm-project/commit/f805c60a093325c16ce4200d2615ef48555d9cb8
DIFF: https://github.com/llvm/llvm-project/commit/f805c60a093325c16ce4200d2615ef48555d9cb8.diff
LOG: Revert "[clangd] Implement rename by using SelectionTree and findExplicitReferences."
This reverts commit 4f80fc2491cc35730a9a84b86975278b7daa8522.
Caused buildbot failure at
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/58251
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 fb83083384f9..3969f3e2e4e2 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -8,16 +8,14 @@
#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/AST/DeclCXX.h"
-#include "clang/AST/DeclTemplate.h"
-#include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
namespace clang {
namespace clangd {
@@ -36,17 +34,6 @@ 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) {
@@ -69,41 +56,12 @@ 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.
@@ -167,8 +125,6 @@ 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");
};
@@ -178,38 +134,22 @@ llvm::Error makeError(ReasonToReject Reason) {
}
// Return all rename occurrences in the main file.
-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;
+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;
for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
- 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);
- });
+ tooling::SymbolOccurrences RenameInDecl =
+ tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);
+ Result.insert(Result.end(), std::make_move_iterator(RenameInDecl.begin()),
+ std::make_move_iterator(RenameInDecl.end()));
}
-
- return Results;
+ return Result;
}
} // namespace
@@ -225,41 +165,30 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
return makeError(UnsupportedSymbol);
- 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());
+ const auto *RenameDecl =
+ tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg);
if (!RenameDecl)
- return makeError(UnsupportedSymbol);
+ return makeError(NoSymbolFound);
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 (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))
+ 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)
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(
- SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
+ AST.getASTContext().getSourceManager(),
+ CharSourceRange::getCharRange(Rename.getNameRanges()[0]), 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 3c3c60500f9d..212d9c089f92 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, WithinFileRename) {
- // rename is runnning on all "^" points, and "[[]]" ranges point to the
+TEST(RenameTest, SingleFile) {
+ // "^" points to the position of the rename, and "[[]]" ranges point to the
// identifier that is being renamed.
llvm::StringRef Tests[] = {
- // Function.
+ // Rename function.
R"cpp(
- void [[foo^]]() {
+ void [[foo]]() {
[[fo^o]]();
}
)cpp",
- // Type.
+ // Rename type.
R"cpp(
- struct [[foo^]] {};
+ struct [[foo]]{};
[[foo]] test() {
[[f^oo]] x;
return x;
}
)cpp",
- // Local variable.
+ // Rename variable.
R"cpp(
void bar() {
if (auto [[^foo]] = 5) {
@@ -66,313 +66,18 @@ TEST(RenameTest, WithinFileRename) {
}
}
)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";
- 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);
- }
+ 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);
}
}
@@ -443,32 +148,12 @@ 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) {
@@ -506,36 +191,6 @@ 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