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 @@ -273,12 +273,12 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, Callback> CB) { - auto Action = [Pos](Path File, std::string NewName, - Callback> CB, - llvm::Expected InpAST) { + auto Action = [Pos, this](Path File, std::string NewName, + Callback> CB, + llvm::Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); - auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName); + auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index); if (!Changes) return CB(Changes.takeError()); std::vector Edits; diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h --- a/clang-tools-extra/clangd/refactor/Rename.h +++ b/clang-tools-extra/clangd/refactor/Rename.h @@ -18,10 +18,10 @@ /// Renames all occurrences of the symbol at \p Pos to \p NewName. /// Occurrences outside the current file are not modified. -llvm::Expected renameWithinFile(ParsedAST &AST, - llvm::StringRef File, - Position Pos, - llvm::StringRef NewName); +/// Only support renaming symbols not being used outside the file. +llvm::Expected +renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, + llvm::StringRef NewName, const SymbolIndex *Index = nullptr); } // namespace clangd } // namespace clang 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 @@ -7,8 +7,11 @@ //===----------------------------------------------------------------------===// #include "refactor/Rename.h" +#include "AST.h" +#include "Logger.h" #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h" #include "clang/Tooling/Refactoring/Rename/RenamingAction.h" +#include "clang/Tooling/Refactoring/Rename/USRFinder.h" namespace clang { namespace clangd { @@ -46,11 +49,54 @@ return Err; } +llvm::Optional filePath(const SymbolLocation &Loc, + llvm::StringRef HintFilePath) { + if (!Loc) + return None; + auto Uri = URI::parse(Loc.FileURI); + if (!Uri) { + elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError()); + return None; + } + auto U = URIForFile::fromURI(*Uri, HintFilePath); + if (!U) { + elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError()); + return None; + } + return U->file().str(); +} + +// Query the index to get other file where the Decl is referenced. +llvm::Optional getOtherRefFile(const NamedDecl &Decl, + StringRef MainFile, + const SymbolIndex *Index) { + if (!Index) + return None; + + RefsRequest Req; + // We limit the number of results, this is a correctness/performance + // tradeoff. We expect the number of symbol references in the current file + // is smaller than the limit. + Req.Limit = 100; + if (auto ID = getSymbolID(&Decl)) + Req.IDs.insert(*ID); + llvm::Optional OtherFile; + Index->refs(Req, [&](const Ref &R) { + if (OtherFile) + return; + if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) { + if (*RefFilePath != MainFile) + OtherFile = *RefFilePath; + } + }); + return OtherFile; +} + } // namespace llvm::Expected renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, - llvm::StringRef NewName) { + llvm::StringRef NewName, const SymbolIndex *Index) { RefactoringResultCollector ResultCollector; ASTContext &ASTCtx = AST.getASTContext(); SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier( @@ -62,6 +108,16 @@ if (!Rename) return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics()); + const auto *RenameDecl = Rename->getRenameDecl(); + assert(RenameDecl && "symbol must be found at this point"); + // FIXME: we may skip querying the index if RenameDecl is function-local. + if (auto OtherFile = getOtherRefFile(*RenameDecl, File, Index)) + return llvm::make_error( + llvm::formatv( + "renaming symbol used outside of the file is not supported: {0}", + *OtherFile), + llvm::inconvertibleErrorCode()); + Rename->invoke(ResultCollector, Context); assert(ResultCollector.Result.hasValue()); diff --git a/clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp b/clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp @@ -94,7 +94,7 @@ Intent intent() const override { return Info; } bool hidden() const override { return true; } }; -REGISTER_TWEAK(ShowSelectionTree); +REGISTER_TWEAK(ShowSelectionTree) /// Shows the layout of the RecordDecl under the cursor. /// Input: @@ -132,7 +132,7 @@ private: const RecordDecl *Record = nullptr; }; -REGISTER_TWEAK(DumpRecordLayout); +REGISTER_TWEAK(DumpRecordLayout) } // namespace } // namespace clangd 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 @@ -18,6 +18,10 @@ namespace clangd { namespace { +MATCHER_P2(RenameRange, Code, Range, "") { + return replacementToEdit(Code, arg).range == Range; +} + TEST(RenameTest, SingleFile) { struct Test { const char* Before; @@ -87,6 +91,69 @@ } } +TEST(RenameTest, WithIndex) { + const char *Tests[] = { + R"cpp( + class [[Foo]] {}; + void f() { + [[Fo^o]] b; + } + )cpp", + + R"cpp( + void f(int [[Lo^cal]]) { + [[Local]] = 2; + } + )cpp", + + // Cases where we reject to do the rename. + R"cpp( // no symbol at the cursor + // This is in comme^nt. + )cpp", + + R"cpp( + void f() { // Outside main file. + Out^side s; + } + )cpp", + }; + const char *Header = "class Outside {};"; + + TestTU OtherFile = TestTU::withCode("Outside s;"); + OtherFile.HeaderCode = Header; + OtherFile.Filename = "other.cc"; + + for (const char *Test : Tests) { + Annotations T(Test); + TestTU MainFile; + MainFile.Code = T.code(); + MainFile.HeaderCode = Header; + auto AST = MainFile.build(); + // Build a fake index containing refs of MainFile and OtherFile + auto OtherFileIndex = OtherFile.index(); + auto MainFileIndex = MainFile.index(); + auto Index = MergedIndex(MainFileIndex.get(), OtherFileIndex.get()); + + auto Results = renameWithinFile(AST, testPath(MainFile.Filename), T.point(), + "dummyNewName", &Index); + bool WantRename = true; + if (T.ranges().empty()) + WantRename = false; + if (!WantRename) { + EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: " + << T.code(); + llvm::consumeError(Results.takeError()); + } else { + EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: " + << llvm::toString(Results.takeError()); + std::vector> Expected; + for (const auto &R : T.ranges()) + Expected.push_back(RenameRange(MainFile.Code, R)); + EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected)); + } + } +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h --- a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h +++ b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h @@ -54,6 +54,8 @@ static const RefactoringDescriptor &describe(); + const NamedDecl *getRenameDecl() const; + private: RenameOccurrences(const NamedDecl *ND, std::string NewName) : ND(ND), NewName(std::move(NewName)) {} diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp --- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp +++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp @@ -74,6 +74,8 @@ std::move(NewName)); } +const NamedDecl *RenameOccurrences::getRenameDecl() const { return ND; } + Expected RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) { Expected Occurrences = findSymbolOccurrences(ND, Context);