Author: hokein Date: Tue Jun 25 01:43:17 2019 New Revision: 364283 URL: http://llvm.org/viewvc/llvm-project?rev=364283&view=rev Log: [clangd] Narrow rename to local symbols.
Summary: Previously, we performed rename for all kinds of symbols (local, global). This patch narrows the scope by only renaming symbols not being used outside of the main file (with index asisitance). Renaming global symbols is not supported at the moment (return an error). Reviewers: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D63426 Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/refactor/Rename.cpp clang-tools-extra/trunk/clangd/refactor/Rename.h clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=364283&r1=364282&r2=364283&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Jun 25 01:43:17 2019 @@ -277,12 +277,12 @@ ClangdServer::formatOnType(llvm::StringR void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, Callback<std::vector<TextEdit>> CB) { - auto Action = [Pos](Path File, std::string NewName, - Callback<std::vector<TextEdit>> CB, - llvm::Expected<InputsAndAST> InpAST) { + auto Action = [Pos, this](Path File, std::string NewName, + Callback<std::vector<TextEdit>> CB, + llvm::Expected<InputsAndAST> 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()); Modified: clang-tools-extra/trunk/clangd/refactor/Rename.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/Rename.cpp?rev=364283&r1=364282&r2=364283&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/refactor/Rename.cpp (original) +++ clang-tools-extra/trunk/clangd/refactor/Rename.cpp Tue Jun 25 01:43:17 2019 @@ -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 @@ llvm::Error expandDiagnostics(llvm::Erro return Err; } +llvm::Optional<std::string> 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<std::string> 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<std::string> 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<ReasonToReject> 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<tooling::Replacements> 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 @@ renameWithinFile(ParsedAST &AST, llvm::S 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::StringError>( + llvm::formatv("Cannot rename symbol: {0}", Message(*Reject)), + llvm::inconvertibleErrorCode()); + } + Rename->invoke(ResultCollector, Context); assert(ResultCollector.Result.hasValue()); Modified: clang-tools-extra/trunk/clangd/refactor/Rename.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/Rename.h?rev=364283&r1=364282&r2=364283&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/refactor/Rename.h (original) +++ clang-tools-extra/trunk/clangd/refactor/Rename.h Tue Jun 25 01:43:17 2019 @@ -18,10 +18,11 @@ namespace clangd { /// Renames all occurrences of the symbol at \p Pos to \p NewName. /// Occurrences outside the current file are not modified. -llvm::Expected<tooling::Replacements> 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<tooling::Replacements> +renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, + llvm::StringRef NewName, const SymbolIndex *Index = nullptr); } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp?rev=364283&r1=364282&r2=364283&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp (original) +++ clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp Tue Jun 25 01:43:17 2019 @@ -18,6 +18,10 @@ namespace clang { 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, SingleFile) { } } +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<testing::Matcher<tooling::Replacement>> Expected; + for (const auto &R : T.ranges()) + Expected.push_back(RenameRange(TU.Code, R)); + EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected)); + } + } +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
