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.

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

Allow skipping autocorrect from within autocorrect block #336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 20 additions & 5 deletions spec/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ module Ameba
return unless name.includes?("A")

issue_for(node.name, message: "A to AA") do |corrector|
corrector.replace(node.name, name.sub("A", "AA"))
next unless location = node.name.location
next unless end_location = node.name.end_location

corrector.replace(location, end_location, name.sub("A", "AA"))
end
end
end
Expand All @@ -126,7 +129,10 @@ module Ameba
return unless name.includes?("A")

issue_for(node.name, message: "A to B") do |corrector|
corrector.replace(node.name, name.tr("A", "B"))
next unless location = node.name.location
next unless end_location = node.name.end_location

corrector.replace(location, end_location, name.tr("A", "B"))
end
end
end
Expand All @@ -143,7 +149,10 @@ module Ameba
return unless name.includes?("B")

issue_for(node.name, message: "B to A") do |corrector|
corrector.replace(node.name, name.tr("B", "A"))
next unless location = node.name.location
next unless end_location = node.name.end_location

corrector.replace(location, end_location, name.tr("B", "A"))
end
end
end
Expand All @@ -160,7 +169,10 @@ module Ameba
return unless name.includes?("B")

issue_for(node.name, message: "B to C") do |corrector|
corrector.replace(node.name, name.tr("B", "C"))
next unless location = node.name.location
next unless end_location = node.name.end_location

corrector.replace(location, end_location, name.tr("B", "C"))
end
end
end
Expand All @@ -177,7 +189,10 @@ module Ameba
return unless name.includes?("C")

issue_for(node.name, message: "C to A") do |corrector|
corrector.replace(node.name, name.tr("C", "A"))
next unless location = node.name.location
next unless end_location = node.name.end_location

corrector.replace(location, end_location, name.tr("C", "A"))
end
end
end
Expand Down
6 changes: 4 additions & 2 deletions src/ameba/issue.cr
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ module Ameba
rule.is_a?(Rule::Lint::Syntax)
end

def correctable?
[email protected]?
getter? correctable : Bool do
corrector = Source::Corrector.new(code)
correct(corrector)
!corrector.empty?
end

def correct(corrector)
Expand Down
21 changes: 12 additions & 9 deletions src/ameba/rule/lint/comparison_to_boolean.cr
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,21 @@ module Ameba::Rule::Lint
end

return unless bool && exp
return unless exp_code = node_source(exp, source.lines)

not =
case node.name
when "==", "===" then !bool.value # foo == false
when "!=" then bool.value # foo != true
end
issue_for node, MSG do |corrector|
next unless location = node.location
next unless end_location = node.end_location
next unless exp_code = node_source(exp, source.lines)

exp_code = "!#{exp_code}" if not
not =
case node.name
when "==", "===" then !bool.value # foo == false
when "!=" then bool.value # foo != true
end

issue_for node, MSG do |corrector|
corrector.replace(node, exp_code)
exp_code = "!#{exp_code}" if not

corrector.replace(location, end_location, exp_code)
end
end
end
Expand Down
11 changes: 7 additions & 4 deletions src/ameba/rule/lint/not_nil_after_no_bang.cr
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,17 @@ module Ameba::Rule::Lint
return unless obj.name.in?(obj.block ? BLOCK_CALL_NAMES : CALL_NAMES)

return unless name_location = obj.name_location
return unless name_location_end = name_end_location(obj)
return unless end_location = name_end_location(node)
return unless name_end_location = name_end_location(node)

msg = MSG % {obj.name, obj.name}

issue_for name_location, end_location, msg do |corrector|
issue_for name_location, name_end_location, msg do |corrector|
next unless location = node.location
next unless end_location = node.end_location
next unless name_location_end = name_end_location(obj)

corrector.insert_after(name_location_end, '!')
corrector.remove_trailing(node, {{ ".not_nil!".size }})
corrector.remove_trailing(location, end_location, {{ ".not_nil!".size }})
end
end
end
Expand Down
5 changes: 4 additions & 1 deletion src/ameba/rule/style/is_a_nil.cr
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ module Ameba::Rule::Style
return unless path_named?(const, "Nil")

issue_for const, MSG do |corrector|
corrector.replace(node, "#{node.obj}.nil?")
next unless location = node.location
next unless end_location = node.end_location

corrector.replace(location, end_location, "#{node.obj}.nil?")
end
end
end
Expand Down
12 changes: 9 additions & 3 deletions src/ameba/rule/style/parentheses_around_condition.cr
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ module Ameba::Rule::Style

if cond.is_a?(Crystal::Assign) && allow_safe_assignment?
issue_for cond, MSG_MISSING do |corrector|
corrector.wrap(cond, '(', ')')
next unless location = cond.location
next unless end_location = cond.end_location

corrector.wrap(location, end_location, '(', ')')
end
return
end
Expand All @@ -73,8 +76,11 @@ module Ameba::Rule::Style
return unless strip_parentheses?(exp, is_ternary)

issue_for cond, MSG_REDUNDANT do |corrector|
corrector.remove_trailing(cond, 1)
corrector.remove_leading(cond, 1)
next unless location = cond.location
next unless end_location = cond.end_location

corrector.remove_trailing(location, end_location, 1)
corrector.remove_leading(location, end_location, 1)
end
end
end
Expand Down
12 changes: 6 additions & 6 deletions src/ameba/rule/style/redundant_next.cr
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,12 @@ module Ameba::Rule::Style
return if allow_multi_next? && node.exp.is_a?(Crystal::TupleLiteral)
return if allow_empty_next? && (node.exp.nil? || node.exp.try(&.nop?))

if exp_code = control_exp_code(node, source.lines)
issue_for node, MSG do |corrector|
corrector.replace(node, exp_code)
end
else
issue_for node, MSG
issue_for node, MSG do |corrector|
next unless location = node.location
next unless end_location = node.end_location
next unless exp_code = control_exp_code(node, source.lines)

corrector.replace(location, end_location, exp_code)
end
end
end
Expand Down
12 changes: 6 additions & 6 deletions src/ameba/rule/style/redundant_return.cr
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,12 @@ module Ameba::Rule::Style
return if allow_multi_return? && node.exp.is_a?(Crystal::TupleLiteral)
return if allow_empty_return? && (node.exp.nil? || node.exp.try(&.nop?))

if exp_code = control_exp_code(node, source.lines)
issue_for node, MSG do |corrector|
corrector.replace(node, exp_code)
end
else
issue_for node, MSG
issue_for node, MSG do |corrector|
next unless location = node.location
next unless end_location = node.end_location
next unless exp_code = control_exp_code(node, source.lines)

corrector.replace(location, end_location, exp_code)
end
end
end
Expand Down
15 changes: 5 additions & 10 deletions src/ameba/rule/style/unless_else.cr
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,12 @@ module Ameba::Rule::Style
def test(source, node : Crystal::Unless)
return if node.else.nop?

location = node.location
cond_end_location = node.cond.end_location
else_location = node.else_location
end_location = node.end_location

unless location && cond_end_location && else_location && end_location
issue_for node, MSG
return
end

issue_for node, MSG do |corrector|
next unless location = node.location
next unless cond_end_location = node.cond.end_location
next unless else_location = node.else_location
next unless end_location = node.end_location

keyword_begin_pos = source.pos(location)
keyword_end_pos = keyword_begin_pos + {{ "unless".size }}
keyword_range = keyword_begin_pos...keyword_end_pos
Expand Down
57 changes: 5 additions & 52 deletions src/ameba/source/corrector.cr
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ class Ameba::Source
@rewriter = Rewriter.new(code)
end

# Returns `true` if no (non trivial) update has been recorded
def empty?
@rewriter.empty?
end

# Replaces the code of the given range with *content*.
def replace(location, end_location, content)
@rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, content)
Expand Down Expand Up @@ -140,58 +145,6 @@ class Ameba::Source
@line_sizes[0...line - 1].sum + (column - 1)
end

# Replaces the code of the given node with *content*.
def replace(node : Crystal::ASTNode, content)
replace(location(node), end_location(node), content)
end

# Inserts the given strings before and after the given node.
def wrap(node : Crystal::ASTNode, insert_before, insert_after)
wrap(location(node), end_location(node), insert_before, insert_after)
end

# Shortcut for `replace(node, "")`
def remove(node : Crystal::ASTNode)
remove(location(node), end_location(node))
end

# Shortcut for `wrap(node, content, nil)`
def insert_before(node : Crystal::ASTNode, content)
insert_before(location(node), content)
end

# Shortcut for `wrap(node, nil, content)`
def insert_after(node : Crystal::ASTNode, content)
insert_after(end_location(node), content)
end

# Removes *size* characters prior to the given node.
def remove_preceding(node : Crystal::ASTNode, size)
remove_preceding(location(node), end_location(node), size)
end

# Removes *size* characters from the beginning of the given node.
# If *size* is greater than the size of the node, the removed region can
# overrun the end of the node.
def remove_leading(node : Crystal::ASTNode, size)
remove_leading(location(node), end_location(node), size)
end

# Removes *size* characters from the end of the given node.
# If *size* is greater than the size of the node, the removed region can
# overrun the beginning of the node.
def remove_trailing(node : Crystal::ASTNode, size)
remove_trailing(location(node), end_location(node), size)
end

private def location(node : Crystal::ASTNode)
node.location || raise "Missing location"
end

private def end_location(node : Crystal::ASTNode)
node.end_location || raise "Missing end location"
end

Comment on lines -143 to -194
Copy link
Member

Choose a reason for hiding this comment

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

Why? Removing these will increase the amount of boilerplate code.

Copy link
Contributor Author

@FnControlOption FnControlOption Dec 23, 2022

Choose a reason for hiding this comment

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

One reason is that autocorrect probably should just be skipped if a node's location is missing (instead of raising an error). And while these methods could be updated to return early instead of raising an error, I think they would hide too much of what's going on behind the scenes. For example, the following autocorrect block might get only partially executed because name_location_end could be present while node.location or node.end_location is nil:

issue_for name_location, name_end_location, msg do |corrector|
  next unless name_location_end = name_end_location(obj)

  corrector.insert_after(name_location_end, '!')
  corrector.remove_trailing(node, {{ ".not_nil!".size }})
end

Copy link
Member

@Sija Sija Dec 23, 2022

Choose a reason for hiding this comment

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

Yeah, I was thinking about this the other day. The optimal solution though would be IMO to let add_issue figure it out, instead of fiddling with it at the call-site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind explaining further? Don't know exactly what you mean

# Applies all scheduled changes and returns modified source as a new string.
def process
@rewriter.process
Expand Down