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 @@ -272,12 +272,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,15 +49,70 @@ return Err; } +static 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(); +} + } // 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( AST, Pos, AST.getSourceManager().getMainFileID()); + + if (Index) { + // Consult the index to determine whether the symbol is used outside of + // the current file. + // FIXME: we may skip querying the index if D is function-local. + const NamedDecl *D = + clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg); + if (!D) + return llvm::make_error( + "there is no symbol at the given location", + llvm::inconvertibleErrorCode()); + 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(D)) + Req.IDs.insert(*ID); + bool OutsideMainFile = false; + std::string OtherFile; + Index->refs(Req, [&](const Ref &R) { + if (OutsideMainFile) + return; + if (auto RefFilePath = filePath(R.Location, File)) { + if (*RefFilePath != File) { + OtherFile = *RefFilePath; + OutsideMainFile = true; + } + } + }); + if (OutsideMainFile) + return llvm::make_error( + llvm::formatv( + "renaming symbol used outside of the file is not supported: ", + OtherFile), + llvm::inconvertibleErrorCode()); + } + tooling::RefactoringRuleContext Context(AST.getSourceManager()); Context.setASTContext(ASTCtx); auto Rename = clang::tooling::RenameOccurrences::initiate( 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