Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code actions for attribute accessor creation #2739

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Code actions for attribute accessor creation #2739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions lib/ruby_lsp/requests/code_action_resolve.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def perform
refactor_method
when CodeActions::TOGGLE_BLOCK_STYLE_TITLE
switch_block_style
when CodeActions::CREATE_ATTRIBUTE_READER,
CodeActions::CREATE_ATTRIBUTE_WRITER,
CodeActions::CREATE_ATTRIBUTE_ACCESSOR
create_attribute_accessor
else
Error::UnknownCodeAction
end
Expand Down Expand Up @@ -329,6 +333,93 @@ def switch_block_body(body, indentation)

indentation ? body_content.gsub(";", "\n") : "#{body_content.gsub("\n", ";")} "
end

sig { returns(T.any(Interface::CodeAction, Error)) }
def create_attribute_accessor
source_range = @code_action.dig(:data, :range)
return Error::EmptySelection if source_range[:start] == source_range[:end]

node = @document.locate_first_within_range(
@code_action.dig(:data, :range),
node_types: [
Prism::InstanceVariableAndWriteNode,
Prism::InstanceVariableOperatorWriteNode,
Prism::InstanceVariableOrWriteNode,
Prism::InstanceVariableReadNode,
Prism::InstanceVariableTargetNode,
Prism::InstanceVariableWriteNode,
],
)
return Error::EmptySelection if node.nil?

node = T.cast(
node,
T.any(
Prism::InstanceVariableAndWriteNode,
Prism::InstanceVariableOperatorWriteNode,
Prism::InstanceVariableOrWriteNode,
Prism::InstanceVariableReadNode,
Prism::InstanceVariableTargetNode,
Prism::InstanceVariableWriteNode,
),
)

scanner = @document.create_scanner
start_index = scanner.find_char_position(
line: node.location.start_line,
character: node.location.start_character_column,
)
node_context = RubyDocument.locate(
@document.parse_result.value,
start_index,
node_types: [
Prism::ClassNode,
Prism::ModuleNode,
Prism::SingletonClassNode,
],
code_units_cache: @document.code_units_cache,
)
closest_node = node_context.node
return Error::InvalidTargetRange if closest_node.nil?

attribute_name = node.name[1..]
indentation = " " * (closest_node.location.start_column + 2)
attribute_accessor_source = T.must(
(
case @code_action[:title]
when CodeActions::CREATE_ATTRIBUTE_READER
"#{indentation}attr_reader :#{attribute_name}\n\n"
when CodeActions::CREATE_ATTRIBUTE_WRITER
"#{indentation}attr_writer :#{attribute_name}\n\n"
when CodeActions::CREATE_ATTRIBUTE_ACCESSOR
"#{indentation}attr_accessor :#{attribute_name}\n\n"
end
),
)

target_start_line = closest_node.location.start_line
target_range = {
start: { line: target_start_line, character: 0 },
end: { line: target_start_line, character: 0 },
}

Interface::CodeAction.new(
title: @code_action[:title],
edit: Interface::WorkspaceEdit.new(
document_changes: [
Interface::TextDocumentEdit.new(
text_document: Interface::OptionalVersionedTextDocumentIdentifier.new(
uri: @code_action.dig(:data, :uri),
version: nil,
),
edits: [
create_text_edit(target_range, attribute_accessor_source),
],
),
],
),
)
end
end
end
end
18 changes: 18 additions & 0 deletions lib/ruby_lsp/requests/code_actions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class CodeActions < Request
EXTRACT_TO_VARIABLE_TITLE = "Refactor: Extract Variable"
EXTRACT_TO_METHOD_TITLE = "Refactor: Extract Method"
TOGGLE_BLOCK_STYLE_TITLE = "Refactor: Toggle block style"
CREATE_ATTRIBUTE_READER = "Create Attribute Reader"
CREATE_ATTRIBUTE_WRITER = "Create Attribute Writer"
CREATE_ATTRIBUTE_ACCESSOR = "Create Attribute Accessor"

class << self
extend T::Sig
Expand Down Expand Up @@ -65,6 +68,21 @@ def perform
kind: Constant::CodeActionKind::REFACTOR_REWRITE,
data: { range: @range, uri: @uri.to_s },
)
code_actions << Interface::CodeAction.new(
title: CREATE_ATTRIBUTE_READER,
kind: Constant::CodeActionKind::EMPTY,
data: { range: @range, uri: @uri.to_s },
)
code_actions << Interface::CodeAction.new(
title: CREATE_ATTRIBUTE_WRITER,
kind: Constant::CodeActionKind::EMPTY,
data: { range: @range, uri: @uri.to_s },
)
code_actions << Interface::CodeAction.new(
title: CREATE_ATTRIBUTE_ACCESSOR,
kind: Constant::CodeActionKind::EMPTY,
data: { range: @range, uri: @uri.to_s },
)
Comment on lines +71 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we've been showing all refactoring code actions unconditionally because we only hand 2 or 3.

As this number grows, we are going to need to start being more selective about when to show things to the user.

What do you think about using the range with Document#locate_first_within_range and then we only make these refactors available if we found an instance variable inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good; we can proceed with that. For now, Document#locate_first_within_range should be sufficient for handling instance variables.

However, if we later need to display a toggle block-style action based on the presence of a block in the selected region, we’ll need to call Document#locate_first_within_range again to locate the call node.

If we create a method that accepts a list of desired nodes and returns only those within a specified range (similar to Document#locate_first_within_range but designed to find all specified options rather than just one). we could then use this list to determine which code actions to provide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinistock Which approach should we take to move forward for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just getting back to this now. Let's use locate_first_within_range to only show the code actions if we're on an instance variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider this user behavior: we have an InstanceVariableWriteNode like @foo = 1, and the user's selection includes only @foo. Using locate_first_within_range would return nil since the selection range does not cover the entire node, preventing us from providing attribute-related actions. The same applies to all types of write nodes.

Is there an alternative approach or implementation in our library to detect such cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I think you will need to account for both the case where the whole node is selected or for when the cursor is sitting on the node. Something like this:

# Try to find an instance variable node if it's sitting under the cursor
node_context = RubyDocument.locate(
  @document.parse_result.value,
  start_index,
  node_types: [Prism::InstanceVariableWriteNode],
  code_units_cache: @document.code_units_cache,
)

# If we didn't find it, try to check if there are any instance variable nodes
# inside the selection
node_context ||= @document.locate_first_within_range(
  range,
  node_types: [Prism::InstanceVariableWriteNode],
)

end

code_actions
Expand Down
47 changes: 47 additions & 0 deletions test/expectations/code_action_resolve/create_attribute_accessor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"params": {
"kind": "",
"title": "Create Attribute Accessor",
"data": {
"range": {
"start": {
"line": 2,
"character": 0
},
"end": {
"line": 2,
"character": 16
}
},
"uri": "file:///fake"
}
},
"result": {
"title": "Create Attribute Accessor",
"edit": {
"documentChanges": [
{
"textDocument": {
"uri": "file:///fake",
"version": null
},
"edits": [
{
"range": {
"start": {
"line": 1,
"character": 0
},
"end": {
"line": 1,
"character": 0
}
},
"newText": " attr_accessor :foo\n\n"
}
]
}
]
}
}
}
47 changes: 47 additions & 0 deletions test/expectations/code_action_resolve/create_attribute_reader.exp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"params": {
"kind": "",
"title": "Create Attribute Reader",
"data": {
"range": {
"start": {
"line": 2,
"character": 0
},
"end": {
"line": 2,
"character": 16
}
},
"uri": "file:///fake"
}
},
"result": {
"title": "Create Attribute Reader",
"edit": {
"documentChanges": [
{
"textDocument": {
"uri": "file:///fake",
"version": null
},
"edits": [
{
"range": {
"start": {
"line": 1,
"character": 0
},
"end": {
"line": 1,
"character": 0
}
},
"newText": " attr_reader :foo\n\n"
}
]
}
]
}
}
}
47 changes: 47 additions & 0 deletions test/expectations/code_action_resolve/create_attributer_writer.exp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"params": {
"kind": "",
"title": "Create Attribute Writer",
"data": {
"range": {
"start": {
"line": 3,
"character": 0
},
"end": {
"line": 4,
"character": 0
}
},
"uri": "file:///fake"
}
},
"result": {
"title": "Create Attribute Writer",
"edit": {
"documentChanges": [
{
"textDocument": {
"uri": "file:///fake",
"version": null
},
"edits": [
{
"range": {
"start": {
"line": 2,
"character": 0
},
"end": {
"line": 2,
"character": 0
}
},
"newText": " attr_accessor :foo\n\n"
}
]
}
]
}
}
}
51 changes: 51 additions & 0 deletions test/expectations/code_actions/aref_field.exp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,57 @@
},
"uri": "file:///fake"
}
},
{
"title": "Create Attribute Reader",
"kind": "",
"data": {
"range": {
"start": {
"line": 2,
"character": 9
},
"end": {
"line": 2,
"character": 13
}
},
"uri": "file:///fake"
}
},
{
"title": "Create Attribute Writer",
"kind": "",
"data": {
"range": {
"start": {
"line": 2,
"character": 9
},
"end": {
"line": 2,
"character": 13
}
},
"uri": "file:///fake"
}
},
{
"title": "Create Attribute Accessor",
"kind": "",
"data": {
"range": {
"start": {
"line": 2,
"character": 9
},
"end": {
"line": 2,
"character": 13
}
},
"uri": "file:///fake"
}
}
]
}
5 changes: 5 additions & 0 deletions test/fixtures/create_attribute_accessor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module Foo
def bar
@foo = "foo"
end
end
5 changes: 5 additions & 0 deletions test/fixtures/create_attribute_reader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Foo
def bar
@foo = "foo"
end
end
7 changes: 7 additions & 0 deletions test/fixtures/create_attribute_writer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class Foo
class << self
def bar
@foo = "foo"
end
end
end
16 changes: 15 additions & 1 deletion test/requests/code_actions_expectations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ def map_actions(expectation)
refactors = expectation
.select { |action| action["kind"].start_with?("refactor") }
.map { |action| code_action_for_refactor(action) }
result = [*quickfixes, *refactors]
empty_kind = expectation
.select { |action| action["kind"] == "" }
.map { |action| code_action_for_refactor(action) }
result = [*quickfixes, *refactors, *empty_kind]

JSON.parse(result.to_json)
end
Expand Down Expand Up @@ -83,4 +86,15 @@ def code_action_for_refactor(refactor)
},
)
end

def code_action_for_empty_kind(expectations)
LanguageServer::Protocol::Interface::CodeAction.new(
title: expectations["title"],
kind: expectations["kind"],
data: {
range: expectations.dig("data", "range"),
uri: expectations.dig("data", "uri"),
},
)
end
end
Loading