diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -800,8 +800,13 @@ void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params, Callback> Reply) { - Server->prepareRename(Params.textDocument.uri.file(), Params.position, - Opts.Rename, std::move(Reply)); + Server->prepareRename( + Params.textDocument.uri.file(), Params.position, Opts.Rename, + [Reply = std::move(Reply)](llvm::Expected Result) mutable { + if (!Result) + return Reply(Result.takeError()); + return Reply(std::move(Result->Target)); + }); } void ClangdLSPServer::onRename(const RenameParams &Params, @@ -813,14 +818,14 @@ Server->rename( File, Params.position, Params.newName, Opts.Rename, [File, Params, Reply = std::move(Reply), - this](llvm::Expected Edits) mutable { - if (!Edits) - return Reply(Edits.takeError()); - if (auto Err = validateEdits(DraftMgr, *Edits)) + this](llvm::Expected R) mutable { + if (!R) + return Reply(R.takeError()); + if (auto Err = validateEdits(DraftMgr, R->GlobalChanges)) return Reply(std::move(Err)); WorkspaceEdit Result; Result.changes.emplace(); - for (const auto &Rep : *Edits) { + for (const auto &Rep : R->GlobalChanges) { (*Result.changes)[URI::createFile(Rep.first()).toString()] = Rep.second.asTextEdits(); } diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -273,9 +273,12 @@ StringRef TriggerText, Callback> CB); /// Test the validity of a rename operation. + /// + /// The returned result describes edits in the main-file only (all + /// occurrences of the renamed symbol are simply deleted. void prepareRename(PathRef File, Position Pos, const RenameOptions &RenameOpts, - Callback> CB); + Callback CB); /// Rename all occurrences of the symbol at the \p Pos in \p File to /// \p NewName. @@ -283,7 +286,7 @@ /// embedders could use this method to get all occurrences of the symbol (e.g. /// highlighting them in prepare stage). void rename(PathRef File, Position Pos, llvm::StringRef NewName, - const RenameOptions &Opts, Callback CB); + const RenameOptions &Opts, Callback CB); struct TweakRef { std::string ID; /// ID to pass for applyTweak. 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 @@ -400,46 +400,35 @@ void ClangdServer::prepareRename(PathRef File, Position Pos, const RenameOptions &RenameOpts, - Callback> CB) { + Callback CB) { auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts, this](llvm::Expected InpAST) mutable { if (!InpAST) return CB(InpAST.takeError()); - auto &AST = InpAST->AST; - const auto &SM = AST.getSourceManager(); - auto Loc = sourceLocationInMainFile(SM, Pos); - if (!Loc) - return CB(Loc.takeError()); - const auto *TouchingIdentifier = - spelledIdentifierTouching(*Loc, AST.getTokens()); - if (!TouchingIdentifier) - return CB(llvm::None); // no rename on non-identifiers. - - auto Range = halfOpenToRange( - SM, CharSourceRange::getCharRange(TouchingIdentifier->location(), - TouchingIdentifier->endLocation())); - - if (RenameOpts.AllowCrossFile) - // FIXME: we now assume cross-file rename always succeeds, revisit this. - return CB(Range); - - // Performing the local rename isn't substantially more expensive than - // doing an AST-based check, so we just rename and throw away the results. - auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts, - /*GetDirtyBuffer=*/nullptr}); - if (!Changes) { + // prepareRename is latency-sensitive: + // - for single-file rename, performing rename isn't substantially more + // expensive than doing an AST-based check (the index is used to see if + // the rename is complete); + // - for cross-file rename, we deliberately pass a nullptr index to save + // the cost, thus the result may be incomplete as it only contains + // main-file occurrences; + auto Results = clangd::rename({Pos, /*NewName*/ "", InpAST->AST, File, + RenameOpts.AllowCrossFile ? nullptr : Index, + RenameOpts}); + if (!Results) { // LSP says to return null on failure, but that will result in a generic // failure message. If we send an LSP error response, clients can surface // the message to users (VSCode does). - return CB(Changes.takeError()); + return CB(Results.takeError()); } - return CB(Range); + return CB(*Results); }; WorkScheduler.runWithAST("PrepareRename", File, std::move(Action)); } void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, - const RenameOptions &Opts, Callback CB) { + const RenameOptions &Opts, + Callback CB) { // A snapshot of all file dirty buffers. llvm::StringMap Snapshot = WorkScheduler.getAllFileContents(); auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts, @@ -457,24 +446,24 @@ return llvm::None; return It->second; }; - auto Edits = clangd::rename( + auto R = clangd::rename( {Pos, NewName, InpAST->AST, File, Index, Opts, GetDirtyBuffer}); - if (!Edits) - return CB(Edits.takeError()); + if (!R) + return CB(R.takeError()); if (Opts.WantFormat) { auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents, *InpAST->Inputs.TFS); llvm::Error Err = llvm::Error::success(); - for (auto &E : *Edits) + for (auto &E : R->GlobalChanges) Err = llvm::joinErrors(reformatEdit(E.getValue(), Style), std::move(Err)); if (Err) return CB(std::move(Err)); } - RenameFiles.record(Edits->size()); - return CB(std::move(*Edits)); + RenameFiles.record(R->GlobalChanges.size()); + return CB(*R); }; WorkScheduler.runWithAST("Rename", File, std::move(Action)); } diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -181,6 +181,8 @@ tooling::Replacements Replacements; std::string InitialCode; + Edit() = default; + Edit(llvm::StringRef Code, tooling::Replacements Reps) : Replacements(std::move(Reps)), InitialCode(Code) {} 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 @@ -55,10 +55,20 @@ DirtyBufferGetter GetDirtyBuffer = nullptr; }; +struct RenameResult { + // The range of the symbol that the user can attempt to rename. + Range Target; + // Rename occurrences for the current main file. + std::vector LocalChanges; + // Complete edits for the rename, including LocalChanges. + // If the full set of changes is unknown, this field is empty. + FileEdits GlobalChanges; +}; + /// Renames all occurrences of the symbol. The result edits are unformatted. /// If AllowCrossFile is false, returns an error if rename a symbol that's used /// in another file (per the index). -llvm::Expected rename(const RenameInputs &RInputs); +llvm::Expected rename(const RenameInputs &RInputs); /// Generates rename edits that replaces all given occurrences with the /// NewName. 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 @@ -182,8 +182,6 @@ } assert(CrossFile); - if (!Index) - return ReasonToReject::NoIndexProvided; // FIXME: Renaming virtual methods requires to rename all overridens in // subclasses, our index doesn't have this information. @@ -427,7 +425,7 @@ } // namespace -llvm::Expected rename(const RenameInputs &RInputs) { +llvm::Expected rename(const RenameInputs &RInputs) { trace::Span Tracer("Rename flow"); const auto &Opts = RInputs.Opts; ParsedAST &AST = RInputs.AST; @@ -456,9 +454,13 @@ return Loc.takeError(); const syntax::Token *IdentifierToken = spelledIdentifierTouching(*Loc, AST.getTokens()); + // Renames should only triggered on identifiers. if (!IdentifierToken) return makeError(ReasonToReject::NoSymbolFound); + Range CurrentIdentifier = halfOpenToRange( + SM, CharSourceRange::getCharRange(IdentifierToken->location(), + IdentifierToken->endLocation())); // FIXME: Renaming macros is not supported yet, the macro-handling code should // be moved to rename tooling library. if (locateMacroAt(*IdentifierToken, AST.getPreprocessor())) @@ -489,32 +491,40 @@ auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName); if (!MainFileRenameEdit) return MainFileRenameEdit.takeError(); + RenameResult Result; + Result.Target = CurrentIdentifier; + Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit)); + llvm::for_each(MainFileEdits.asTextEdits(), [&Result](const TextEdit &TE) { + Result.LocalChanges.push_back(TE.range); + }); // return the main file edit if this is a within-file rename or the symbol // being renamed is function local. if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) { - return FileEdits( - {std::make_pair(RInputs.MainFilePath, - Edit{MainFileCode, std::move(*MainFileRenameEdit)})}); + Result.GlobalChanges = FileEdits( + {std::make_pair(RInputs.MainFilePath, std::move(MainFileEdits))}); + return Result; } - FileEdits Results; - // Renameable safely guards us that at this point we are renaming a local - // symbol if we don't have index. - if (RInputs.Index) { - auto OtherFilesEdits = renameOutsideFile( - RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index, - Opts.LimitFiles == 0 ? std::numeric_limits::max() - : Opts.LimitFiles, - GetFileContent); - if (!OtherFilesEdits) - return OtherFilesEdits.takeError(); - Results = std::move(*OtherFilesEdits); + // If the index is nullptr, we don't know the completeness of the result, so + // we don't populate the field GlobalChanges. + if (!RInputs.Index) { + assert(Result.GlobalChanges.empty() && Opts.AllowCrossFile); + return Result; } + + auto OtherFilesEdits = renameOutsideFile( + RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index, + Opts.LimitFiles == 0 ? std::numeric_limits::max() + : Opts.LimitFiles, + GetFileContent); + if (!OtherFilesEdits) + return OtherFilesEdits.takeError(); + Result.GlobalChanges = *OtherFilesEdits; // Attach the rename edits for the main file. - Results.try_emplace(RInputs.MainFilePath, MainFileCode, - std::move(*MainFileRenameEdit)); - return Results; + Result.GlobalChanges.try_emplace(RInputs.MainFilePath, + std::move(MainFileEdits)); + return Result; } llvm::Expected buildRenameEdit(llvm::StringRef AbsFilePath, diff --git a/clang-tools-extra/clangd/test/rename.test b/clang-tools-extra/clangd/test/rename.test --- a/clang-tools-extra/clangd/test/rename.test +++ b/clang-tools-extra/clangd/test/rename.test @@ -21,9 +21,12 @@ # CHECK-NEXT: } --- {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}} -# CHECK: "id": 2, -# CHECK-NEXT: "jsonrpc": "2.0", -# CHECK-NEXT: "result": null +# CHECK: "error": { +# CHECK-NEXT: "code": -32001, +# CHECK-NEXT: "message": "Cannot rename symbol: there is no symbol at the given location" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 2, +# CHECK-NEXT: "jsonrpc": "2.0" --- {"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}} # CHECK: "error": { 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 @@ -502,9 +502,10 @@ auto RenameResult = rename({RenamePos, NewName, AST, testPath(TU.Filename)}); ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError(); - ASSERT_EQ(1u, RenameResult->size()); - EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second, - expectedResult(Code, NewName)); + ASSERT_EQ(1u, RenameResult->GlobalChanges.size()); + EXPECT_EQ( + applyEdits(std::move(RenameResult->GlobalChanges)).front().second, + expectedResult(Code, NewName)); } } } @@ -653,8 +654,8 @@ } else { EXPECT_TRUE(bool(Results)) << "rename returned an error: " << llvm::toString(Results.takeError()); - ASSERT_EQ(1u, Results->size()); - EXPECT_EQ(applyEdits(std::move(*Results)).front().second, + ASSERT_EQ(1u, Results->GlobalChanges.size()); + EXPECT_EQ(applyEdits(std::move(Results->GlobalChanges)).front().second, expectedResult(T, NewName)); } } @@ -683,8 +684,8 @@ auto RenameResult = rename({Code.point(), NewName, AST, testPath(TU.Filename)}); ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point(); - ASSERT_EQ(1u, RenameResult->size()); - EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second, + ASSERT_EQ(1u, RenameResult->GlobalChanges.size()); + EXPECT_EQ(applyEdits(std::move(RenameResult->GlobalChanges)).front().second, expectedResult(Code, NewName)); } @@ -703,6 +704,44 @@ testing::HasSubstr("not a supported kind")); } +TEST(RenameTest, PrepareRename) { + Annotations FooH("void func();"); + Annotations FooCC(R"cpp( + #include "foo.h" + void [[fu^nc]]() {} + )cpp"); + std::string FooHPath = testPath("foo.h"); + std::string FooCCPath = testPath("foo.cc"); + MockFS FS; + FS.Files[FooHPath] = std::string(FooH.code()); + FS.Files[FooCCPath] = std::string(FooCC.code()); + + auto ServerOpts = ClangdServer::optsForTest(); + ServerOpts.BuildDynamicSymbolIndex = true; + + MockCompilationDatabase CDB; + ClangdServer Server(CDB, FS, ServerOpts); + runAddDocument(Server, FooHPath, FooH.code()); + runAddDocument(Server, FooCCPath, FooCC.code()); + + auto Results = + runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/true}); + // verify that for multi-file rename, we only return main-file occurrences. + ASSERT_TRUE(bool(Results)) << Results.takeError(); + // We don't know the result is complete in prepareRename (passing a nullptr + // index internally), so GlobalChanges should be empty. + EXPECT_TRUE(Results->GlobalChanges.empty()); + EXPECT_THAT(FooCC.ranges(), + testing::UnorderedElementsAreArray(Results->LocalChanges)); + + // single-file rename on global symbols, we should report an error. + Results = + runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/false}); + EXPECT_FALSE(Results); + EXPECT_THAT(llvm::toString(Results.takeError()), + testing::HasSubstr("is used outside")); +} + TEST(CrossFileRenameTests, DirtyBuffer) { Annotations FooCode("class [[Foo]] {};"); std::string FooPath = testPath("foo.cc"); @@ -741,7 +780,7 @@ GetDirtyBuffer}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( - applyEdits(std::move(*Results)), + applyEdits(std::move(Results->GlobalChanges)), UnorderedElementsAre( Pair(Eq(FooPath), Eq(expectedResult(FooDirtyBuffer, NewName))), Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName))))); @@ -762,7 +801,7 @@ GetDirtyBuffer}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( - applyEdits(std::move(*Results)), + applyEdits(std::move(Results->GlobalChanges)), UnorderedElementsAre( Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))), Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName))))); @@ -847,7 +886,7 @@ {/*CrossFile=*/true}}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( - applyEdits(std::move(*Results)), + applyEdits(std::move(Results->GlobalChanges)), UnorderedElementsAre( Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))), Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName))))); @@ -1047,7 +1086,7 @@ Server, FooHPath, RenamePos, NewName, {/*CrossFile=*/true})); EXPECT_THAT(Tracer.takeMetric("rename_files"), ElementsAre(2)); EXPECT_THAT( - applyEdits(std::move(FileEditsList)), + applyEdits(std::move(FileEditsList.GlobalChanges)), UnorderedElementsAre( Pair(Eq(FooHPath), Eq(expectedResult(T.FooH, NewName))), Pair(Eq(FooCCPath), Eq(expectedResult(T.FooCC, NewName))))); @@ -1066,7 +1105,7 @@ auto Results = rename({Code.point(), NewName, AST, Path}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( - applyEdits(std::move(*Results)), + applyEdits(std::move(Results->GlobalChanges)), UnorderedElementsAre(Pair(Eq(Path), Eq(expectedResult(Code, NewName))))); } diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.h b/clang-tools-extra/clangd/unittests/SyncAPI.h --- a/clang-tools-extra/clangd/unittests/SyncAPI.h +++ b/clang-tools-extra/clangd/unittests/SyncAPI.h @@ -40,9 +40,13 @@ llvm::Expected> runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos); -llvm::Expected runRename(ClangdServer &Server, PathRef File, - Position Pos, StringRef NewName, - const clangd::RenameOptions &RenameOpts); +llvm::Expected runRename(ClangdServer &Server, PathRef File, + Position Pos, StringRef NewName, + const clangd::RenameOptions &RenameOpts); + +llvm::Expected +runPrepareRename(ClangdServer &Server, PathRef File, Position Pos, + const clangd::RenameOptions &RenameOpts); llvm::Expected runFormatFile(ClangdServer &Server, PathRef File, StringRef Code); diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.cpp b/clang-tools-extra/clangd/unittests/SyncAPI.cpp --- a/clang-tools-extra/clangd/unittests/SyncAPI.cpp +++ b/clang-tools-extra/clangd/unittests/SyncAPI.cpp @@ -97,14 +97,22 @@ return std::move(*Result); } -llvm::Expected runRename(ClangdServer &Server, PathRef File, - Position Pos, llvm::StringRef NewName, - const RenameOptions &RenameOpts) { - llvm::Optional> Result; +llvm::Expected runRename(ClangdServer &Server, PathRef File, + Position Pos, llvm::StringRef NewName, + const RenameOptions &RenameOpts) { + llvm::Optional> Result; Server.rename(File, Pos, NewName, RenameOpts, capture(Result)); return std::move(*Result); } +llvm::Expected runPrepareRename(ClangdServer &Server, + PathRef File, Position Pos, + const RenameOptions &RenameOpts) { + llvm::Optional> Result; + Server.prepareRename(File, Pos, RenameOpts, capture(Result)); + return std::move(*Result); +} + llvm::Expected runFormatFile(ClangdServer &Server, PathRef File, StringRef Code) { llvm::Optional> Result;