From 7c3de47f5d8ff604353bb4e762fa0a72c3142f8a Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Tue, 28 Jan 2025 16:18:51 -0800 Subject: [PATCH 1/6] scaffolding --- main/lsp/BUILD | 1 + main/lsp/ExtractMethod.cc | 15 +++++++++++++++ main/lsp/ExtractMethod.h | 22 ++++++++++++++++++++++ main/lsp/requests/code_action.cc | 19 +++++++++++++++++++ main/options/options.cc | 4 ++++ main/options/options.h | 1 + test/helpers/position_assertions.cc | 1 + test/lsp_test_runner.cc | 2 ++ website/docs/cli-ref.md | 2 ++ 9 files changed, 67 insertions(+) create mode 100644 main/lsp/ExtractMethod.cc create mode 100644 main/lsp/ExtractMethod.h diff --git a/main/lsp/BUILD b/main/lsp/BUILD index b72c2394e70..9ee0dfb0b5a 100644 --- a/main/lsp/BUILD +++ b/main/lsp/BUILD @@ -31,6 +31,7 @@ cc_library( ], hdrs = [ "ConvertToSingletonClassMethod.h", + "ExtractMethod.h", "ExtractVariable.h", "LSPConfiguration.h", "LSPInput.h", diff --git a/main/lsp/ExtractMethod.cc b/main/lsp/ExtractMethod.cc new file mode 100644 index 00000000000..e1f257c875c --- /dev/null +++ b/main/lsp/ExtractMethod.cc @@ -0,0 +1,15 @@ +#include "main/lsp/ExtractMethod.h" +#include "absl/strings/escaping.h" +#include "ast/treemap/treemap.h" +#include "common/sort/sort.h" + +using namespace std; + +namespace sorbet::realmain::lsp { + +vector> MethodExtractor::getExtractEdits(const LSPTypecheckerDelegate &typechecker, + const LSPConfiguration &config) { + return {}; +} + +} // namespace sorbet::realmain::lsp diff --git a/main/lsp/ExtractMethod.h b/main/lsp/ExtractMethod.h new file mode 100644 index 00000000000..a87d670aa21 --- /dev/null +++ b/main/lsp/ExtractMethod.h @@ -0,0 +1,22 @@ +#ifndef SORBET_EXTRACT_METHOD_H +#define SORBET_EXTRACT_METHOD_H + +#include "main/lsp/LSPConfiguration.h" +#include "main/lsp/LSPTypechecker.h" +#include "main/lsp/json_types.h" + +namespace sorbet::realmain::lsp { + +class MethodExtractor { + const core::Loc selectionLoc; + ast::ExpressionPtr matchingNode; + +public: + MethodExtractor(const core::Loc selectionLoc) : selectionLoc(selectionLoc) {} + std::vector> getExtractEdits(const LSPTypecheckerDelegate &typechecker, + const LSPConfiguration &config); +}; + +} // namespace sorbet::realmain::lsp + +#endif diff --git a/main/lsp/requests/code_action.cc b/main/lsp/requests/code_action.cc index 27b3a021546..f401ac3ccb6 100644 --- a/main/lsp/requests/code_action.cc +++ b/main/lsp/requests/code_action.cc @@ -4,6 +4,7 @@ #include "common/sort/sort.h" #include "core/lsp/QueryResponse.h" #include "main/lsp/ConvertToSingletonClassMethod.h" +#include "main/lsp/ExtractMethod.h" #include "main/lsp/ExtractVariable.h" #include "main/lsp/LSPLoop.h" #include "main/lsp/LSPQuery.h" @@ -314,6 +315,24 @@ unique_ptr CodeActionTask::runRequest(LSPTypecheckerDelegate &t // TODO(neil): trigger a rename for newVariable } } + if (config.opts.lspExtractToMethodEnabled) { + MethodExtractor methodExtractor(loc); + vector> documentEdits; + { + Timer timeit(gs.tracer(), "Extract to Method"); + documentEdits = methodExtractor.getExtractEdits(typechecker, config); + } + if (!documentEdits.empty()) { + auto action = make_unique("Extract Method"); + action->kind = CodeActionKind::RefactorExtract; + + auto workspaceEdit = make_unique(); + workspaceEdit->documentChanges = move(documentEdits); + + action->edit = move(workspaceEdit); + result.emplace_back(move(action)); + } + } } response->result = move(result); diff --git a/main/options/options.cc b/main/options/options.cc index 70041886cd9..38ae79c622c 100644 --- a/main/options/options.cc +++ b/main/options/options.cc @@ -549,6 +549,8 @@ buildOptions(const vector(); opts.lspExtractToVariableEnabled = opts.lspAllBetaFeaturesEnabled || raw["enable-experimental-lsp-extract-to-variable"].as(); + opts.lspExtractToMethodEnabled = + opts.lspAllBetaFeaturesEnabled || raw["enable-experimental-lsp-extract-to-method"].as(); opts.lspDocumentHighlightEnabled = enableAllLSPFeatures || raw["enable-experimental-lsp-document-highlight"].as(); opts.lspSignatureHelpEnabled = enableAllLSPFeatures || raw["enable-experimental-lsp-signature-help"].as(); diff --git a/main/options/options.h b/main/options/options.h index b130e77fe17..c1e59e75771 100644 --- a/main/options/options.h +++ b/main/options/options.h @@ -283,6 +283,7 @@ struct Options { bool lspDocumentFormatRubyfmtEnabled = false; bool lspSignatureHelpEnabled = false; bool lspExtractToVariableEnabled = false; + bool lspExtractToMethodEnabled = false; bool forciblySilenceLspMultipleDirError = false; // Enables out-of-order reference checking bool outOfOrderReferenceChecksEnabled = false; diff --git a/test/helpers/position_assertions.cc b/test/helpers/position_assertions.cc index 54bbf218851..033b21fab0c 100644 --- a/test/helpers/position_assertions.cc +++ b/test/helpers/position_assertions.cc @@ -261,6 +261,7 @@ const UnorderedMap< {"typed-super", BooleanPropertyAssertion::make}, {"enable-suggest-unsafe", BooleanPropertyAssertion::make}, {"enable-experimental-lsp-extract-to-variable", BooleanPropertyAssertion::make}, + {"enable-experimental-lsp-extract-to-method", BooleanPropertyAssertion::make}, {"selective-apply-code-action", StringPropertyAssertions::make}, {"use-code-action-resolve", BooleanPropertyAssertion::make}, {"assert-no-code-action", StringPropertyAssertions::make}, diff --git a/test/lsp_test_runner.cc b/test/lsp_test_runner.cc index 0cc479803bf..d31ad96a578 100644 --- a/test/lsp_test_runner.cc +++ b/test/lsp_test_runner.cc @@ -548,6 +548,8 @@ TEST_CASE("LSPTest") { opts.lspExtractToVariableEnabled = BooleanPropertyAssertion::getValue("enable-experimental-lsp-extract-to-variable", assertions) .value_or(false); + opts.lspExtractToMethodEnabled = + BooleanPropertyAssertion::getValue("enable-experimental-lsp-extract-to-method", assertions).value_or(false); opts.disableWatchman = true; opts.rubyfmtPath = "test/testdata/lsp/rubyfmt-stub/rubyfmt"; diff --git a/website/docs/cli-ref.md b/website/docs/cli-ref.md index e70344a6aa9..53fa06ac112 100644 --- a/website/docs/cli-ref.md +++ b/website/docs/cli-ref.md @@ -244,6 +244,8 @@ Usage: Enable experimental LSP feature: Signature Help --enable-experimental-lsp-extract-to-variable Enable experimental LSP feature: Extract To Variable + --enable-experimental-lsp-extract-to-method + Enable experimental LSP feature: Extract To Method --enable-all-experimental-lsp-features Enable every experimental LSP feature. (WARNING: can be crashy; for developer use only. End users should prefer From 51a9fa5ffa54e58f5a2371036701405488959910 Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Wed, 5 Feb 2025 22:55:31 -0800 Subject: [PATCH 2/6] comment in tree sanity checks --- ast/TreeSanityChecks.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/ast/TreeSanityChecks.cc b/ast/TreeSanityChecks.cc index 2ab98852203..738dffb86c7 100644 --- a/ast/TreeSanityChecks.cc +++ b/ast/TreeSanityChecks.cc @@ -115,6 +115,7 @@ void Literal::_sanityCheck() { case core::TypePtr::Tag::FloatLiteralType: case core::TypePtr::Tag::NamedLiteralType: case core::TypePtr::Tag::ClassType: + // If you add a new valid tag here, make sure to also update the code in TreeEquality.cc to handle that tag break; default: ENFORCE(false, "unexpected TypePtr::Tag: {}", fmt::underlying(tag)); From 59017002a89e79be66423ba71dc536a6916f245b Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Wed, 5 Feb 2025 22:56:07 -0800 Subject: [PATCH 3/6] add a todo in extract variable --- main/lsp/ExtractVariable.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/main/lsp/ExtractVariable.cc b/main/lsp/ExtractVariable.cc index ff0b7b634be..1fae32f11c9 100644 --- a/main/lsp/ExtractVariable.cc +++ b/main/lsp/ExtractVariable.cc @@ -106,6 +106,7 @@ core::LocOffsets findWhereToInsert(const ast::ExpressionPtr &scope, const core:: // This tree walk takes a Loc and looks for nodes that have that Loc exactly class LocSearchWalk { // The selection loc + // TODO(neil): change this to LocOffsets core::Loc targetLoc; // At the end of this walk, we want to return what class/method the matching expression was part of. // To do that, we can maintain a stack of classes/method, so that when we get a match, we can capture From ea9015266868186251e1aa2941e6fde02ada807d Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Wed, 30 Apr 2025 11:02:52 -0500 Subject: [PATCH 4/6] first prototype --- main/lsp/ExtractMethod.cc | 204 +++++++++++++++++++++++++++++++++++++- 1 file changed, 201 insertions(+), 3 deletions(-) diff --git a/main/lsp/ExtractMethod.cc b/main/lsp/ExtractMethod.cc index e1f257c875c..bf7247a6d7c 100644 --- a/main/lsp/ExtractMethod.cc +++ b/main/lsp/ExtractMethod.cc @@ -1,15 +1,213 @@ #include "main/lsp/ExtractMethod.h" -#include "absl/strings/escaping.h" +#include "absl/strings/str_join.h" #include "ast/treemap/treemap.h" -#include "common/sort/sort.h" using namespace std; namespace sorbet::realmain::lsp { +class Walk { + // The selection loc + core::LocOffsets targetLoc; + +public: + // After the walk is complete, this should point to the deepest insSeq that contains targetLoc + const ast::InsSeq *enclosingInsSeq; + const ast::MethodDef *enclosingMethodDef; + Walk(core::LocOffsets targetLoc) : targetLoc(targetLoc), enclosingInsSeq(nullptr) {} + + void preTransformInsSeq(core::Context ctx, const ast::ExpressionPtr &tree) { + auto &insSeq = ast::cast_tree_nonnull(tree); + if (insSeq.loc.exists() && insSeq.loc.contains(targetLoc)) { + if (!enclosingInsSeq || enclosingInsSeq->loc.contains(insSeq.loc)) { + enclosingInsSeq = &insSeq; + } + } + } + + void preTransformMethodDef(core::Context ctx, const ast::ExpressionPtr &tree) { + auto &methodDef = ast::cast_tree_nonnull(tree); + if (methodDef.loc.exists() && methodDef.loc.contains(targetLoc)) { + enclosingMethodDef = &methodDef; + } + } +}; + +class FindLocalsWalk { + absl::flat_hash_set &localsAssigned; + absl::flat_hash_set &localsReferenced; + + absl::flat_hash_set lhsLocs; + +public: + FindLocalsWalk(absl::flat_hash_set &localsAssigned, + absl::flat_hash_set &localsReferenced) + : localsAssigned(localsAssigned), localsReferenced(localsReferenced) {} + void preTransformAssign(core::Context ctx, const ast::Assign &assign) { + if (auto local = ast::cast_tree(assign.lhs)) { + localsAssigned.insert(local->localVariable._name); + lhsLocs.insert(local->loc); + } + return; + } + + void postTransformLocal(core::Context ctx, const ast::Local &local) { + if (!lhsLocs.contains(local.loc)) { + localsReferenced.insert(local.localVariable._name); + } + } +}; + +void findLocals(const core::GlobalState &gs, const core::FileRef file, + absl::flat_hash_set &localsAssigned, + absl::flat_hash_set &localsReferenced, const ast::ExpressionPtr *insn) { + FindLocalsWalk walk(localsAssigned, localsReferenced); + core::Context ctx(gs, core::Symbols::root(), file); + ast::ConstTreeWalk::apply(ctx, walk, *insn); +} + +const ast::ExpressionPtr *getElem(const ast::InsSeq *insSeq, int i) { + if (i > insSeq->stats.size()) { + return nullptr; + } + + if (i == insSeq->stats.size()) { + return &insSeq->expr; + } + + return &insSeq->stats[i]; +} + vector> MethodExtractor::getExtractEdits(const LSPTypecheckerDelegate &typechecker, const LSPConfiguration &config) { - return {}; + const auto file = selectionLoc.file(); + const auto &gs = typechecker.state(); + // Steps: + // - Find enclosing InsSeq (insSeq.contains(selectionLoc)) + Walk walk(selectionLoc.offsets()); + // TODO(neil): do we need local Vars? + auto afterLocalVars = typechecker.getLocalVarTrees(file); + core::Context ctx(gs, core::Symbols::root(), file); + ast::TreeWalk::apply(ctx, walk, afterLocalVars); + auto *enclosingInsSeq = walk.enclosingInsSeq; + auto *enclosingMethodDef = walk.enclosingMethodDef; + + if (!enclosingInsSeq || !enclosingMethodDef) { + return {}; + } + // + // - Step through each insn in InsSeq, and see if selection corrosponds to full insns + // - start of selection == start of some insn and end of selection == end of some insn + int start_idx = -1; + int end_idx = -1; + // TODO: use vector instead of set so it's ordered? or btree_set? + absl::flat_hash_set localsAssignedBefore; + // TODO: replace with std::transform/absl::c_transform? + for (auto ¶m : enclosingMethodDef->args) { + if (auto local = ast::cast_tree(param)) { + localsAssignedBefore.insert(local->localVariable._name); + } + } + absl::flat_hash_set localsReferencedIn; + absl::flat_hash_set localsAssignedIn; + absl::flat_hash_set localsReferencedAfter; + // size() + 1 to account for InsSeq#expr + // TODO: we need to find all the locals from before the enclosing insSeq too + for (int i = 0; i < enclosingInsSeq->stats.size() + 1; i++) { + auto *insn = getElem(enclosingInsSeq, i); + if (start_idx == -1) { + // TODO: move the start of selectionLoc to the first non-whitespace character, so we can ignore the + // indentation at the start + if (insn->loc().beginPos() == selectionLoc.offsets().beginPos()) { + // First insn in the user's selection + start_idx = i; + + findLocals(gs, file, localsAssignedIn, localsReferencedIn, insn); + } else { + // Before the user's selection + absl::flat_hash_set localsReferencedBefore_; + findLocals(gs, file, localsAssignedBefore, localsReferencedBefore_, insn); + } + } else if (end_idx == -1) { + // Inside the user's selection + findLocals(gs, file, localsAssignedIn, localsReferencedIn, insn); + + // TODO: move the end of selectionLoc to the last non-whitespace character, so we can ignore the + // indentation at the end + if (insn->loc().endPos() == selectionLoc.offsets().endPos()) { + // Last insn in the user's selection + end_idx = i; + } + } else { + // After the user's selection + absl::flat_hash_set localsAssignedAfter_; + findLocals(gs, file, localsAssignedAfter_, localsReferencedAfter, insn); + } + } + + if (start_idx == -1 || end_idx == -1) { + return {}; + } + /* for (auto name : localsAssignedBefore) { */ + /* fmt::print("assignedBefore: {}\n", name.toString(gs)); */ + /* } */ + /* for (auto name : localsAssignedIn) { */ + /* fmt::print("assignedIn: {}\n", name.toString(gs)); */ + /* } */ + /* for (auto name : localsReferencedIn) { */ + /* fmt::print("referencedIn: {}\n", name.toString(gs)); */ + /* } */ + /* for (auto name : localsReferencedAfter) { */ + /* fmt::print("referencedAfter: {}\n", name.toString(gs)); */ + /* } */ + // TODO: if the selection includes a return, don't allow extracting it for now? + + std::vector methodParams; + std::vector methodReturns; + // args for new method are those that are (referencedIn and (assignedBefore or !assignedIn)) + for (auto name : localsReferencedIn) { + if (name == core::Names::selfLocal()) { + continue; + } + if (localsAssignedBefore.contains(name) || !localsAssignedIn.contains(name)) { + methodParams.push_back(name.show(gs)); + } + } + // new method will return those are (assignedIn and referencedAfter) + for (auto name : localsAssignedIn) { + /* if (localsReferencedAfter.contains(name)) { */ + methodReturns.push_back(name.show(gs)); + /* } */ + } + + string firstLine = fmt::format("def newMethod({})", absl::StrJoin(methodParams, ", ")); + string lastLine = fmt::format("return {}", absl::StrJoin(methodReturns, ", ")); + auto [_, numSpaces] = selectionLoc.findStartOfIndentation(gs); + config.logger->info("{}\n", numSpaces); + auto bodyIndentation = std::string(numSpaces, ' '); + auto defEndIndentation = std::string(numSpaces - 2, ' '); + auto newMethod = + fmt::format("{}\n{}{}\n{}{}\n{}end\n\n{}", firstLine, bodyIndentation, selectionLoc.source(gs).value(), + bodyIndentation, lastLine, defEndIndentation, defEndIndentation); + + vector> edits; + edits.emplace_back(make_unique( + Range::fromLoc(gs, core::Loc(file, enclosingMethodDef->loc.copyWithZeroLength())), newMethod)); + if (methodReturns.size() > 0) { + edits.emplace_back(make_unique( + Range::fromLoc(gs, selectionLoc), + fmt::format("{} = newMethod({})", absl::StrJoin(methodReturns, ", "), absl::StrJoin(methodParams, ", ")))); + } else { + edits.emplace_back(make_unique(Range::fromLoc(gs, selectionLoc), + fmt::format("newMethod({})", absl::StrJoin(methodParams, ", ")))); + } + + auto docEdit = make_unique( + make_unique(config.fileRef2Uri(gs, file), JSONNullObject()), move(edits)); + + vector> res; + res.emplace_back(move(docEdit)); + return res; } } // namespace sorbet::realmain::lsp From 971998c7bc370d131f9aae03be6a9b547248e708 Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Wed, 30 Apr 2025 11:03:03 -0500 Subject: [PATCH 5/6] add a simple test --- .../extract_method/test.A.rbedited | 19 +++++++++++++++++++ .../lsp/code_actions/extract_method/test.rb | 14 ++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 test/testdata/lsp/code_actions/extract_method/test.A.rbedited create mode 100644 test/testdata/lsp/code_actions/extract_method/test.rb diff --git a/test/testdata/lsp/code_actions/extract_method/test.A.rbedited b/test/testdata/lsp/code_actions/extract_method/test.A.rbedited new file mode 100644 index 00000000000..8216d0bb609 --- /dev/null +++ b/test/testdata/lsp/code_actions/extract_method/test.A.rbedited @@ -0,0 +1,19 @@ +# typed: true +# selective-apply-code-action: refactor.extract +# enable-experimental-lsp-extract-to-method: true + +class A + def y + 1 + end + + def newMethod(x, a) + b = x * y; c = x + y + a + b + return c, b + end + + def foo(x) + a = x + y; c, b = newMethod(x, a); return c + # ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ apply-code-action: [A] Extract Method + end +end diff --git a/test/testdata/lsp/code_actions/extract_method/test.rb b/test/testdata/lsp/code_actions/extract_method/test.rb new file mode 100644 index 00000000000..b0e5cb250be --- /dev/null +++ b/test/testdata/lsp/code_actions/extract_method/test.rb @@ -0,0 +1,14 @@ +# typed: true +# selective-apply-code-action: refactor.extract +# enable-experimental-lsp-extract-to-method: true + +class A + def y + 1 + end + + def foo(x) + a = x + y; b = x * y; c = x + y + a + b; return c + # ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ apply-code-action: [A] Extract Method + end +end From 84d4509e5a405cf69b7da1cfd428d7bd2d2bc27d Mon Sep 17 00:00:00 2001 From: Neil Parikh Date: Wed, 30 Apr 2025 11:14:55 -0500 Subject: [PATCH 6/6] todo --- main/lsp/ExtractMethod.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/main/lsp/ExtractMethod.cc b/main/lsp/ExtractMethod.cc index bf7247a6d7c..567803f7229 100644 --- a/main/lsp/ExtractMethod.cc +++ b/main/lsp/ExtractMethod.cc @@ -179,6 +179,7 @@ vector> MethodExtractor::getExtractEdits(const LSPT methodReturns.push_back(name.show(gs)); /* } */ } + // TODO: sort methodParams and methodReturns for deterministic behaviour string firstLine = fmt::format("def newMethod({})", absl::StrJoin(methodParams, ", ")); string lastLine = fmt::format("return {}", absl::StrJoin(methodReturns, ", "));