From 5a0f6a6c9a6bc9254ccbaaf56c03de5951ce3c82 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 26 Jul 2024 09:44:15 -0400 Subject: [PATCH 1/3] Add the ability to locate the first node within a range --- lib/ruby_lsp/document.rb | 34 ++++++++++++++++++++++++++++++++++ test/ruby_document_test.rb | 23 +++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index e57ad3de8..f17d439a3 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -223,6 +223,40 @@ def locate(node, char_position, node_types: []) NodeContext.new(closest, parent, nesting_nodes, call_node) end + sig do + params( + range: T::Hash[Symbol, T.untyped], + node_types: T::Array[T.class_of(Prism::Node)], + ).returns(T.nilable(Prism::Node)) + end + def locate_first_within_range(range, node_types: []) + scanner = create_scanner + start_position = scanner.find_char_position(range[:start]) + end_position = scanner.find_char_position(range[:end]) + desired_range = (start_position...end_position) + queue = T.let(@parse_result.value.child_nodes.compact, T::Array[T.nilable(Prism::Node)]) + + until queue.empty? + candidate = queue.shift + + # Skip nil child nodes + next if candidate.nil? + + # Add the next child_nodes to the queue to be processed. The order here is important! We want to move in the + # same order as the visiting mechanism, which means searching the child nodes before moving on to the next + # sibling + T.unsafe(queue).unshift(*candidate.child_nodes) + + # Skip if the current node doesn't cover the desired position + loc = candidate.location + + if desired_range.cover?(loc.start_offset...loc.end_offset) && + (node_types.empty? || node_types.any? { |type| candidate.class == type }) + return candidate + end + end + end + sig { returns(SorbetLevel) } def sorbet_level sigil = parse_result.magic_comments.find do |comment| diff --git a/test/ruby_document_test.rb b/test/ruby_document_test.rb index 1aca63b30..7571cd6a0 100644 --- a/test/ruby_document_test.rb +++ b/test/ruby_document_test.rb @@ -716,6 +716,29 @@ def qux assert_equal("qux", node_context.surrounding_method) end + def test_locate_first_within_range + document = RubyLsp::RubyDocument.new(source: +<<~RUBY, version: 1, uri: URI("file:///foo/bar.rb")) + method_call(other_call).each do |a| + nested_call(fourth_call).each do |b| + end + end + RUBY + + target = document.locate_first_within_range( + { start: { line: 0, character: 0 }, end: { line: 3, character: 3 } }, + node_types: [Prism::CallNode], + ) + + assert_equal("each", T.cast(target, Prism::CallNode).message) + + target = document.locate_first_within_range( + { start: { line: 1, character: 2 }, end: { line: 2, character: 5 } }, + node_types: [Prism::CallNode], + ) + + assert_equal("each", T.cast(target, Prism::CallNode).message) + end + private def assert_error_edit(actual, error_range) From 469e38cfaaf731e85236b50805d0b92d37666666 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 26 Jul 2024 09:44:48 -0400 Subject: [PATCH 2/3] Add the switch block style refactor --- lib/ruby_lsp/requests/code_action_resolve.rb | 63 ++++++++++++++++++- lib/ruby_lsp/requests/code_actions.rb | 6 ++ .../aref_call_aref_assign.exp.json | 47 ++++++++++++++ .../nested_block_calls.exp.json | 47 ++++++++++++++ .../code_actions/aref_field.exp.json | 17 +++++ test/fixtures/nested_block_calls.rb | 4 ++ .../code_actions_expectations_test.rb | 2 +- 7 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 test/expectations/code_action_resolve/aref_call_aref_assign.exp.json create mode 100644 test/expectations/code_action_resolve/nested_block_calls.exp.json create mode 100644 test/fixtures/nested_block_calls.rb diff --git a/lib/ruby_lsp/requests/code_action_resolve.rb b/lib/ruby_lsp/requests/code_action_resolve.rb index 962c3f2a5..c4e384a51 100644 --- a/lib/ruby_lsp/requests/code_action_resolve.rb +++ b/lib/ruby_lsp/requests/code_action_resolve.rb @@ -23,6 +23,8 @@ module Requests # class CodeActionResolve < Request extend T::Sig + include Support::Common + NEW_VARIABLE_NAME = "new_variable" NEW_METHOD_NAME = "new_method" @@ -50,11 +52,70 @@ def perform refactor_variable when CodeActions::EXTRACT_TO_METHOD_TITLE refactor_method + when CodeActions::SWITCH_BLOCK_STYLE_TITLE + switch_block_style else Error::UnknownCodeAction end end + private + + sig { returns(T.any(Interface::CodeAction, Error)) } + def switch_block_style + return Error::EmptySelection if @document.source.empty? + + source_range = @code_action.dig(:data, :range) + return Error::EmptySelection if source_range[:start] == source_range[:end] + + target = @document.locate_first_within_range( + @code_action.dig(:data, :range), + node_types: [Prism::CallNode], + ) + + return Error::InvalidTargetRange unless target.is_a?(Prism::CallNode) + + node = target.block + return Error::InvalidTargetRange unless node.is_a?(Prism::BlockNode) + + parameters = node.parameters + body = node.body + + # If the block is using `do...end` style, we change it to a single line brace block. Newlines are turned into + # semi colons, so that the result is valid Ruby code and still a one liner. If the block is using brace style, + # we do the opposite and turn it into a `do...end` block, making all semi colons into newlines. + new_source = if node.opening_loc.slice == "do" + source = +"{ " + source << "#{parameters.slice} " if parameters + source << "#{body.slice.gsub("\n", ";")} " if body + source << "}" + else + indentation = " " * target.location.start_column + source = +"do" + source << " #{parameters.slice}" if parameters + source << "\n#{indentation} " + source << body.slice.gsub(";", "\n") if body + source << "\n#{indentation}end" + end + + Interface::CodeAction.new( + title: CodeActions::SWITCH_BLOCK_STYLE_TITLE, + edit: Interface::WorkspaceEdit.new( + document_changes: [ + Interface::TextDocumentEdit.new( + text_document: Interface::OptionalVersionedTextDocumentIdentifier.new( + uri: @code_action.dig(:data, :uri), + version: nil, + ), + edits: [ + Interface::TextEdit.new(range: range_from_location(node.location), new_text: new_source), + ], + ), + ], + ), + ) + end + sig { returns(T.any(Interface::CodeAction, Error)) } def refactor_variable return Error::EmptySelection if @document.source.empty? @@ -206,8 +267,6 @@ def refactor_method ) end - private - sig { params(range: T::Hash[Symbol, T.untyped], new_text: String).returns(Interface::TextEdit) } def create_text_edit(range, new_text) Interface::TextEdit.new( diff --git a/lib/ruby_lsp/requests/code_actions.rb b/lib/ruby_lsp/requests/code_actions.rb index d710501fd..f60aec7cf 100644 --- a/lib/ruby_lsp/requests/code_actions.rb +++ b/lib/ruby_lsp/requests/code_actions.rb @@ -21,6 +21,7 @@ class CodeActions < Request EXTRACT_TO_VARIABLE_TITLE = "Refactor: Extract Variable" EXTRACT_TO_METHOD_TITLE = "Refactor: Extract Method" + SWITCH_BLOCK_STYLE_TITLE = "Refactor: Switch block style" class << self extend T::Sig @@ -69,6 +70,11 @@ def perform kind: Constant::CodeActionKind::REFACTOR_EXTRACT, data: { range: @range, uri: @uri.to_s }, ) + code_actions << Interface::CodeAction.new( + title: SWITCH_BLOCK_STYLE_TITLE, + kind: Constant::CodeActionKind::REFACTOR_REWRITE, + data: { range: @range, uri: @uri.to_s }, + ) end code_actions diff --git a/test/expectations/code_action_resolve/aref_call_aref_assign.exp.json b/test/expectations/code_action_resolve/aref_call_aref_assign.exp.json new file mode 100644 index 000000000..d9c668996 --- /dev/null +++ b/test/expectations/code_action_resolve/aref_call_aref_assign.exp.json @@ -0,0 +1,47 @@ +{ + "params": { + "kind": "refactor.rewrite", + "title": "Refactor: Switch block style", + "data": { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 58 + } + }, + "uri": "file:///fake" + } + }, + "result": { + "title": "Refactor: Switch block style", + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///fake", + "version": null + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 26 + }, + "end": { + "line": 0, + "character": 58 + } + }, + "newText": "do |a|\n a[\"field\"] == \"expected\"\nend" + } + ] + } + ] + } + } +} diff --git a/test/expectations/code_action_resolve/nested_block_calls.exp.json b/test/expectations/code_action_resolve/nested_block_calls.exp.json new file mode 100644 index 000000000..4c0cad78a --- /dev/null +++ b/test/expectations/code_action_resolve/nested_block_calls.exp.json @@ -0,0 +1,47 @@ +{ + "params": { + "kind": "refactor.rewrite", + "title": "Refactor: Switch block style", + "data": { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 3, + "character": 3 + } + }, + "uri": "file:///fake" + } + }, + "result": { + "title": "Refactor: Switch block style", + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///fake", + "version": null + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 29 + }, + "end": { + "line": 3, + "character": 3 + } + }, + "newText": "{ |a| nested_call(fourth_call).each do |b|; end }" + } + ] + } + ] + } + } +} diff --git a/test/expectations/code_actions/aref_field.exp.json b/test/expectations/code_actions/aref_field.exp.json index 77e89b68f..4e7478c73 100644 --- a/test/expectations/code_actions/aref_field.exp.json +++ b/test/expectations/code_actions/aref_field.exp.json @@ -52,6 +52,23 @@ }, "uri": "file:///fake" } + }, + { + "title": "Refactor: Switch block style", + "kind": "refactor.rewrite", + "data": { + "range": { + "start": { + "line": 2, + "character": 9 + }, + "end": { + "line": 2, + "character": 13 + } + }, + "uri": "file:///fake" + } } ] } diff --git a/test/fixtures/nested_block_calls.rb b/test/fixtures/nested_block_calls.rb new file mode 100644 index 000000000..43de2c3b0 --- /dev/null +++ b/test/fixtures/nested_block_calls.rb @@ -0,0 +1,4 @@ +method_call(other_call).each do |a| + nested_call(fourth_call).each do |b| + end +end diff --git a/test/requests/code_actions_expectations_test.rb b/test/requests/code_actions_expectations_test.rb index 4cf9376b3..f0c6be9c6 100644 --- a/test/requests/code_actions_expectations_test.rb +++ b/test/requests/code_actions_expectations_test.rb @@ -76,7 +76,7 @@ def code_action_for_diagnostic(diagnostic) def code_action_for_refactor(refactor) LanguageServer::Protocol::Interface::CodeAction.new( title: refactor["title"], - kind: LanguageServer::Protocol::Constant::CodeActionKind::REFACTOR_EXTRACT, + kind: refactor["kind"], data: { range: refactor.dig("data", "range"), uri: refactor.dig("data", "uri"), From c273e3c08b9513c661f987f0a78b35d493187ee2 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 8 Aug 2024 10:27:23 -0400 Subject: [PATCH 3/3] Apply consistent block style to all nested blocks --- lib/ruby_lsp/requests/code_action_resolve.rb | 91 +++++++++++++------ .../nested_block_calls.exp.json | 2 +- .../nested_oneline_blocks.exp.json | 47 ++++++++++ test/fixtures/nested_oneline_blocks.rb | 1 + 4 files changed, 114 insertions(+), 27 deletions(-) create mode 100644 test/expectations/code_action_resolve/nested_oneline_blocks.exp.json create mode 100644 test/fixtures/nested_oneline_blocks.rb diff --git a/lib/ruby_lsp/requests/code_action_resolve.rb b/lib/ruby_lsp/requests/code_action_resolve.rb index c4e384a51..68101d4a5 100644 --- a/lib/ruby_lsp/requests/code_action_resolve.rb +++ b/lib/ruby_lsp/requests/code_action_resolve.rb @@ -47,6 +47,8 @@ def initialize(document, code_action) sig { override.returns(T.any(Interface::CodeAction, Error)) } def perform + return Error::EmptySelection if @document.source.empty? + case @code_action[:title] when CodeActions::EXTRACT_TO_VARIABLE_TITLE refactor_variable @@ -63,8 +65,6 @@ def perform sig { returns(T.any(Interface::CodeAction, Error)) } def switch_block_style - return Error::EmptySelection if @document.source.empty? - source_range = @code_action.dig(:data, :range) return Error::EmptySelection if source_range[:start] == source_range[:end] @@ -78,25 +78,7 @@ def switch_block_style node = target.block return Error::InvalidTargetRange unless node.is_a?(Prism::BlockNode) - parameters = node.parameters - body = node.body - - # If the block is using `do...end` style, we change it to a single line brace block. Newlines are turned into - # semi colons, so that the result is valid Ruby code and still a one liner. If the block is using brace style, - # we do the opposite and turn it into a `do...end` block, making all semi colons into newlines. - new_source = if node.opening_loc.slice == "do" - source = +"{ " - source << "#{parameters.slice} " if parameters - source << "#{body.slice.gsub("\n", ";")} " if body - source << "}" - else - indentation = " " * target.location.start_column - source = +"do" - source << " #{parameters.slice}" if parameters - source << "\n#{indentation} " - source << body.slice.gsub(";", "\n") if body - source << "\n#{indentation}end" - end + indentation = " " * target.location.start_column unless node.opening_loc.slice == "do" Interface::CodeAction.new( title: CodeActions::SWITCH_BLOCK_STYLE_TITLE, @@ -108,7 +90,10 @@ def switch_block_style version: nil, ), edits: [ - Interface::TextEdit.new(range: range_from_location(node.location), new_text: new_source), + Interface::TextEdit.new( + range: range_from_location(node.location), + new_text: recursively_switch_nested_block_styles(node, indentation), + ), ], ), ], @@ -118,8 +103,6 @@ def switch_block_style sig { returns(T.any(Interface::CodeAction, Error)) } def refactor_variable - return Error::EmptySelection if @document.source.empty? - source_range = @code_action.dig(:data, :range) return Error::EmptySelection if source_range[:start] == source_range[:end] @@ -214,8 +197,6 @@ def refactor_variable sig { returns(T.any(Interface::CodeAction, Error)) } def refactor_method - return Error::EmptySelection if @document.source.empty? - source_range = @code_action.dig(:data, :range) return Error::EmptySelection if source_range[:start] == source_range[:end] @@ -277,6 +258,64 @@ def create_text_edit(range, new_text) new_text: new_text, ) end + + sig { params(node: Prism::BlockNode, indentation: T.nilable(String)).returns(String) } + def recursively_switch_nested_block_styles(node, indentation) + parameters = node.parameters + body = node.body + + # We use the indentation to differentiate between do...end and brace style blocks because only the do...end + # style requires the indentation to build the edit. + # + # If the block is using `do...end` style, we change it to a single line brace block. Newlines are turned into + # semi colons, so that the result is valid Ruby code and still a one liner. If the block is using brace style, + # we do the opposite and turn it into a `do...end` block, making all semi colons into newlines. + source = +"" + + if indentation + source << "do" + source << " #{parameters.slice}" if parameters + source << "\n#{indentation} " + source << switch_block_body(body, indentation) if body + source << "\n#{indentation}end" + else + source << "{ " + source << "#{parameters.slice} " if parameters + source << switch_block_body(body, nil) if body + source << "}" + end + + source + end + + sig { params(body: Prism::Node, indentation: T.nilable(String)).returns(String) } + def switch_block_body(body, indentation) + # Check if there are any nested blocks inside of the current block + body_loc = body.location + nested_block = @document.locate_first_within_range( + { + start: { line: body_loc.start_line - 1, character: body_loc.start_column }, + end: { line: body_loc.end_line - 1, character: body_loc.end_column }, + }, + node_types: [Prism::BlockNode], + ) + + body_content = body.slice.dup + + # If there are nested blocks, then we change their style too and we have to mutate the string using the + # relative position in respect to the beginning of the body + if nested_block.is_a?(Prism::BlockNode) + location = nested_block.location + correction_start = location.start_offset - body_loc.start_offset + correction_end = location.end_offset - body_loc.start_offset + next_indentation = indentation ? "#{indentation} " : nil + + body_content[correction_start...correction_end] = + recursively_switch_nested_block_styles(nested_block, next_indentation) + end + + indentation ? body_content.gsub(";", "\n") : "#{body_content.gsub("\n", ";")} " + end end end end diff --git a/test/expectations/code_action_resolve/nested_block_calls.exp.json b/test/expectations/code_action_resolve/nested_block_calls.exp.json index 4c0cad78a..ef1a59da8 100644 --- a/test/expectations/code_action_resolve/nested_block_calls.exp.json +++ b/test/expectations/code_action_resolve/nested_block_calls.exp.json @@ -37,7 +37,7 @@ "character": 3 } }, - "newText": "{ |a| nested_call(fourth_call).each do |b|; end }" + "newText": "{ |a| nested_call(fourth_call).each { |b| } }" } ] } diff --git a/test/expectations/code_action_resolve/nested_oneline_blocks.exp.json b/test/expectations/code_action_resolve/nested_oneline_blocks.exp.json new file mode 100644 index 000000000..dfbe92ded --- /dev/null +++ b/test/expectations/code_action_resolve/nested_oneline_blocks.exp.json @@ -0,0 +1,47 @@ +{ + "params": { + "kind": "refactor.rewrite", + "title": "Refactor: Switch block style", + "data": { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 74 + } + }, + "uri": "file:///fake" + } + }, + "result": { + "title": "Refactor: Switch block style", + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": "file:///fake", + "version": null + }, + "edits": [ + { + "range": { + "start": { + "line": 0, + "character": 29 + }, + "end": { + "line": 0, + "character": 74 + } + }, + "newText": "do |a|\n nested_call(fourth_call).each do |b|\n \n end\nend" + } + ] + } + ] + } + } +} diff --git a/test/fixtures/nested_oneline_blocks.rb b/test/fixtures/nested_oneline_blocks.rb new file mode 100644 index 000000000..62caf97a7 --- /dev/null +++ b/test/fixtures/nested_oneline_blocks.rb @@ -0,0 +1 @@ +method_call(other_call).each { |a| nested_call(fourth_call).each { |b| } }