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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
