Index: cfe/trunk/include/clang/Tooling/Refactoring/Rename/RenamingAction.h =================================================================== --- cfe/trunk/include/clang/Tooling/Refactoring/Rename/RenamingAction.h +++ cfe/trunk/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)) {} Index: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp =================================================================== --- cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp +++ cfe/trunk/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); Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp @@ -277,12 +277,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()); Index: clang-tools-extra/trunk/clangd/refactor/Rename.h =================================================================== --- clang-tools-extra/trunk/clangd/refactor/Rename.h +++ clang-tools-extra/trunk/clangd/refactor/Rename.h @@ -18,10 +18,11 @@ /// 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); +/// Returns an error if rename a symbol that's used in another file (per the +/// index). +llvm::Expected +renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, + llvm::StringRef NewName, const SymbolIndex *Index = nullptr); } // namespace clangd } // namespace clang Index: clang-tools-extra/trunk/clangd/refactor/Rename.cpp =================================================================== --- clang-tools-extra/trunk/clangd/refactor/Rename.cpp +++ clang-tools-extra/trunk/clangd/refactor/Rename.cpp @@ -7,6 +7,9 @@ //===----------------------------------------------------------------------===// #include "refactor/Rename.h" +#include "AST.h" +#include "Logger.h" +#include "index/SymbolCollector.h" #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h" #include "clang/Tooling/Refactoring/Rename/RenamingAction.h" @@ -46,11 +49,95 @@ 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 find some other files where the Decl is referenced. +llvm::Optional getOtherRefFile(const NamedDecl &Decl, + StringRef MainFile, + const SymbolIndex &Index) { + 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; +} + +enum ReasonToReject { + NoIndexProvided, + NonIndexable, + UsedOutsideFile, +}; + +// Check the symbol Decl is renameable (per the index) within the file. +llvm::Optional renamableWithinFile(const NamedDecl &Decl, + StringRef MainFile, + const SymbolIndex *Index) { + auto &ASTCtx = Decl.getASTContext(); + const auto &SM = ASTCtx.getSourceManager(); + bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile; + bool DeclaredInMainFile = + SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation())); + + // If the symbol is declared in the main file (which is not a header), we + // rename it. + if (DeclaredInMainFile && !MainFileIsHeader) + return None; + + // Below are cases where the symbol is declared in the header. + // If the symbol is function-local, we rename it. + if (Decl.getParentFunctionOrMethod()) + return None; + + if (!Index) + return ReasonToReject::NoIndexProvided; + + bool IsIndexable = + SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false); + // If the symbol is not indexable, we disallow rename. + if (!IsIndexable) + return ReasonToReject::NonIndexable; + auto OtherFile = getOtherRefFile(Decl, MainFile, *Index); + // If the symbol is indexable and has no refs from other files in the index, + // we rename it. + if (!OtherFile) + return None; + // If the symbol is indexable and has refs from other files in the index, + // we disallow rename. + return ReasonToReject::UsedOutsideFile; +} + } // 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 +149,25 @@ if (!Rename) return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics()); + const auto *RenameDecl = Rename->getRenameDecl(); + assert(RenameDecl && "symbol must be found at this point"); + if (auto Reject = renamableWithinFile(*RenameDecl, File, Index)) { + auto Message = [](ReasonToReject Reason) { + switch (Reason) { + case NoIndexProvided: + return "symbol may be used in other files (no index available)"; + case UsedOutsideFile: + return "the symbol is used outside main file"; + case NonIndexable: + return "symbol may be used in other files (not eligible for indexing)"; + } + llvm_unreachable("unhandled reason kind"); + }; + return llvm::make_error( + llvm::formatv("Cannot rename symbol: {0}", Message(*Reject)), + llvm::inconvertibleErrorCode()); + } + Rename->invoke(ResultCollector, Context); assert(ResultCollector.Result.hasValue()); Index: clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp =================================================================== --- clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/trunk/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,67 @@ } } +TEST(RenameTest, Renameable) { + // Test cases where the symbol is declared in header. + const char *Headers[] = { + R"cpp(// allow -- function-local + void f(int [[Lo^cal]]) { + [[Local]] = 2; + } + )cpp", + + R"cpp(// allow -- symbol is indexable and has no refs in index. + void [[On^lyInThisFile]](); + )cpp", + + R"cpp(// disallow -- symbol is indexable and has other refs in index. + void f() { + Out^side s; + } + )cpp", + + R"cpp(// disallow -- symbol is not indexable. + namespace { + class Unin^dexable {}; + } + )cpp", + }; + const char *CommonHeader = "class Outside {};"; + TestTU OtherFile = TestTU::withCode("Outside s;"); + OtherFile.HeaderCode = CommonHeader; + OtherFile.Filename = "other.cc"; + // The index has a "Outside" reference. + auto OtherFileIndex = OtherFile.index(); + + for (const char *Header : Headers) { + Annotations T(Header); + // We open the .h file as the main file. + TestTU TU = TestTU::withCode(T.code()); + TU.Filename = "test.h"; + TU.HeaderCode = CommonHeader; + TU.ExtraArgs.push_back("-xc++"); + auto AST = TU.build(); + + auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(), + "dummyNewName", OtherFileIndex.get()); + 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(TU.Code, R)); + EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected)); + } + } +} + } // namespace } // namespace clangd } // namespace clang