diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -318,6 +318,12 @@ // the message to users (VSCode does). return CB(Changes.takeError()); } + // FIXME: this is not always correct with SelectionTree behavior, e.g. + // t^emplate + // class Foo {}; + // When we trigger rename on the above cursor, we will rename the template + // class Foo, but the token under the cursor is not corresponding to the + // "Foo" range, though the final result is correct. SourceLocation Loc = getBeginningOfIdentifier( Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()); if (auto Range = getTokenRange(AST.getSourceManager(), diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -8,14 +8,15 @@ #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/Tooling/Refactoring/Rename/USRFindingAction.h" -#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h" namespace clang { namespace clangd { @@ -56,12 +57,31 @@ return OtherFile; } +llvm::DenseSet locateDeclAt(ParsedAST &AST, Position P) { + const auto &SM = AST.getSourceManager(); + llvm::StringRef SourceCode = SM.getBufferData(SM.getMainFileID()); + auto Offset = positionToOffset(SourceCode, P); + if (!Offset) { + elog("Fail to convert position to offset: {0}", Offset.takeError()); + return {}; + } + SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset); + llvm::DenseSet Result; + if (const SelectionTree::Node *N = Selection.commonAncestor()) { + for (const auto *D : targetDecl( + N->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 +145,8 @@ 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"); }; @@ -133,23 +155,43 @@ llvm::inconvertibleErrorCode()); } +const NamedDecl &getPrimaryTemplateOrThis(const NamedDecl &ND) { + if (const auto *T = ND.getDescribedTemplate()) + return *T; + return ND; +} + // 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 RenameUSRs = - tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext()); - std::string OldName = CanonicalRenameDecl->getNameAsString(); - tooling::SymbolOccurrences Result; +std::vector 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 = getPrimaryTemplateOrThis(ND); + // FIXME: get rid of the remaining tooling APIs. + std::vector RenameUSRs = tooling::getUSRsForDeclaration( + tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext()); + llvm::DenseSet TargetIDs; + for (auto &USR : RenameUSRs) + TargetIDs.insert(SymbolID(USR)); + + std::vector 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); + assert(ID); + if (TargetIDs.find(*ID) == TargetIDs.end()) + return; + } + Results.push_back(Ref.NameLoc); + }); } - return Result; + + return Results; } } // namespace @@ -165,30 +207,28 @@ if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) return makeError(UnsupportedSymbol); - const auto *RenameDecl = - tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg); - if (!RenameDecl) + auto DeclsUnderCursor = locateDeclAt(AST, Pos); + if (DeclsUnderCursor.empty()) return makeError(NoSymbolFound); + if (DeclsUnderCursor.size() > 1) + return makeError(AmbiguousSymbol); + + const auto *RenameDecl = llvm::dyn_cast(*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) - 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. + for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) { + // We shouldn't have conflicting replacements or replacements from different + // files. If there are conflicts, it means that we have bugs either in + // clangd or in findExplicitReferences, 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(Loc), 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 --- a/clang-tools-extra/clangd/unittests/RenameTests.cpp +++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -38,8 +38,8 @@ 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. @@ -66,18 +66,313 @@ } } )cpp", + + R"cpp( + class [[F^oo]] {}; + template void func() {} + template class Baz {}; + int main() { + func<[[F^oo]]>(); + Baz<[[F^oo]]> obj; + return 0; + } + )cpp", + + // class simple rename + R"cpp( + class [[F^oo]] { + void foo(int x); + }; + void [[Foo]]::foo(int x) {} + )cpp", + + // class constructor/destructor. + R"cpp( + class [[Foo^]] { + [[F^oo]](); + ~[[F^oo]](); + }; + )cpp", + + // class 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", + + // 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(BazReference).getValue(); + dynamic_cast(BazPointer)->getValue(); + reinterpret_cast(BazPointer)->getValue(); + static_cast(BazReference).getValue(); + static_cast(BazPointer)->getValue(); + } + )cpp", + + // class constructors + R"cpp( + class [[^Foo]] { + public: + [[Foo]](); + }; + + [[Foo]]::[[Fo^o]]() {} + )cpp", + + // constructor initializer list + R"cpp( + class Baz {}; + class Qux { + Baz [[F^oo]]; + public: + Qux(); + }; + // FIXME: selection tree on initializer list Foo below will returns + // CXXConstructExpr node, which ends up with renaming class "Baze" + // instead of "Foo". + Qux::Qux() : [[Foo]]() {} + )cpp", + + // DeclRefExpr + R"cpp( + class C { + public: + static int [[F^oo]]; + }; + + int foo(int x) { return 0; } + #define MACRO(a) foo(a) + + void func() { + C::[[F^oo]] = 1; + MACRO(C::[[Foo]]); + int y = C::[[F^oo]]; + } + )cpp", + + // Forward declaration + R"cpp( + class [[F^oo]]; + [[Foo]] *f(); + )cpp", + + // function marco + R"cpp( + #define moo [[foo]] + int [[fo^o]]() { return 42; } + void boo(int value) {} + + void qoo() { + [[foo]](); + boo([[foo]]()); + moo(); + boo(moo()); + } + )cpp", + + // MemberExpr in macro + R"cpp( + class Baz { + public: + int [[F^oo]]; + }; + int qux(int x) { return 0; } + #define MACRO(a) qux(a) + + int main() { + Baz baz; + baz.[[Foo]] = 1; + MACRO(baz.[[Foo]]); + int y = baz.[[Foo]]; + } + )cpp", + + // template class instantiations + R"cpp( + template + class [[F^oo]] { + public: + T foo(T arg, T& ref, T* ptr) { + T value; + int number = 42; + value = (T)number; + value = static_cast(number); + return value; + } + static void foo(T value) {} + T member; + }; + + template + void func() { + [[F^oo]] obj; + obj.member = T(); + [[Foo]]::foo(); + } + + int main() { + [[F^oo]] i; + i.member = 0; + [[F^oo]]::foo(0); + + [[F^oo]] b; + b.member = false; + [[Foo]]::foo(false); + + return 0; + } + )cpp", + + // template arguments + R"cpp( + template + 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", + + // template class methods + R"cpp( + template + class A { + public: + void [[f^oo]]() {} + }; + + void func() { + A a; + A b; + A c; + a.[[f^oo]](); + b.[[f^oo]](); + c.[[f^oo]](); + } + )cpp", + + // typedef + R"cpp( + namespace std { + class basic_string {}; + typedef basic_string [[s^tring]]; + } // namespace std + + std::[[s^tring]] foo(); + )cpp", + + // variable + R"cpp( + #define NAMESPACE namespace A + NAMESPACE { + 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", + + // references in template argument lists + R"cpp( + template + class [[Fo^o]] {}; + template 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(); 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,21 @@ {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 void f(T t) { + fo^o(t); + })cpp", + "multiple symbols", !HeaderFile, nullptr /*no index*/}, }; for (const auto& Case : Cases) {