Skip to content

Commit

Permalink
Add Style/GuardClause rule (#254)
Browse files Browse the repository at this point in the history
  • Loading branch information
FnControlOption committed Dec 9, 2021
1 parent c7f1f94 commit f288cc3
Show file tree
Hide file tree
Showing 18 changed files with 657 additions and 76 deletions.
411 changes: 411 additions & 0 deletions spec/ameba/rule/style/guard_clause_spec.cr

Large diffs are not rendered by default.

11 changes: 5 additions & 6 deletions src/ameba/ast/variabling/assignment.cr
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ module Ameba::AST
# Assignment.new(node, variable, scope)
# ```
def initialize(@node, @variable, @scope)
if scope = @variable.scope
@branch = Branch.of(@node, scope)
@referenced = true if @variable.special? ||
@variable.scope.type_definition? ||
referenced_in_loop?
end
return unless scope = @variable.scope
@branch = Branch.of(@node, scope)
@referenced = true if @variable.special? ||
@variable.scope.type_definition? ||
referenced_in_loop?
end

def referenced_in_loop?
Expand Down
9 changes: 7 additions & 2 deletions src/ameba/ast/visitors/node_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,19 @@ module Ameba::AST
# A visit callback for `Crystal::{{name}}` node.
# Returns true meaning that child nodes will be traversed as well.
def visit(node : Crystal::{{name}})
return false if skip?(node)

@rule.test @source, node
true
end
{% end %}

def visit(node)
return true unless skip = @skip
!skip.includes?(node.class)
!skip?(node)
end

private def skip?(node)
!!@skip.try(&.includes?(node.class))
end
end
end
10 changes: 4 additions & 6 deletions src/ameba/ast/visitors/scope_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ module Ameba::AST
@scope_queue << @current_scope

# go up if this is not a top level scope
if outer_scope = @current_scope.outer_scope
@current_scope = outer_scope
end
return unless outer_scope = @current_scope.outer_scope
@current_scope = outer_scope
end

private def on_assign_end(target, node)
Expand Down Expand Up @@ -132,9 +131,8 @@ module Ameba::AST

# :nodoc:
def visit(node : Crystal::TypeDeclaration)
if !@current_scope.type_definition? && (var = node.var).is_a?(Crystal::Var)
@current_scope.add_variable var
end
return if @current_scope.type_definition? || !(var = node.var).is_a?(Crystal::Var)
@current_scope.add_variable var
end

# :nodoc:
Expand Down
10 changes: 4 additions & 6 deletions src/ameba/config.cr
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,8 @@ class Ameba::Config
@excluded = load_array_section(config, "Excluded")
@globs = load_array_section(config, "Globs", DEFAULT_GLOBS)

if formatter_name = load_formatter_name(config)
self.formatter = formatter_name
end
return unless formatter_name = load_formatter_name(config)
self.formatter = formatter_name
end

# Loads YAML configuration file by `path`.
Expand Down Expand Up @@ -124,11 +123,10 @@ class Ameba::Config
# config.formatter = :progress
# ```
def formatter=(name : String | Symbol)
if f = AVAILABLE_FORMATTERS[name]?
@formatter = f.new
else
unless f = AVAILABLE_FORMATTERS[name]?
raise "Unknown formatter `#{name}`. Use one of #{Config.formatter_names}."
end
@formatter = f.new
end

# Updates rule properties.
Expand Down
5 changes: 2 additions & 3 deletions src/ameba/formatter/dot_formatter.cr
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,8 @@ module Ameba::Formatter
end

private def finished_in_message(started, finished)
if started && finished
"Finished in #{to_human(finished - started)}".colorize(:default)
end
return unless started && finished
"Finished in #{to_human(finished - started)}".colorize(:default)
end

private def to_human(span : Time::Span)
Expand Down
18 changes: 8 additions & 10 deletions src/ameba/inline_comments.cr
Original file line number Diff line number Diff line change
Expand Up @@ -65,20 +65,18 @@ module Ameba
# parse_inline_directive(line) # => nil
# ```
def parse_inline_directive(line)
if directive = COMMENT_DIRECTIVE_REGEX.match(line)
return if commented_out?(line.gsub(directive[0], ""))
{
action: directive["action"],
rules: directive["rules"].split(/[\s,]/, remove_empty: true),
}
end
return unless directive = COMMENT_DIRECTIVE_REGEX.match(line)
return if commented_out?(line.gsub(directive[0], ""))
{
action: directive["action"],
rules: directive["rules"].split(/[\s,]/, remove_empty: true),
}
end

# Returns true if the line at the given `line_number` is a comment.
def comment?(line_number : Int32)
if line = lines[line_number]?
comment?(line)
end
return unless line = lines[line_number]?
comment?(line)
end

private def comment?(line : String)
Expand Down
13 changes: 6 additions & 7 deletions src/ameba/rule/layout/trailing_blank_lines.cr
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@ module Ameba::Rule::Layout
return if source_lines_size == 1 && last_source_line.empty?

last_line_empty = last_source_line.empty?
if source_lines_size >= 1 && (source_lines.last(2).join.blank? || !last_line_empty)
if last_line_empty
issue_for({source_lines_size, 1}, MSG)
else
issue_for({source_lines_size, 1}, MSG_FINAL_NEWLINE) do |corrector|
corrector.insert_before({source_lines_size + 1, 1}, '\n')
end
return if source_lines_size.zero? || (source_lines.last(2).join.presence && last_line_empty)
if last_line_empty
issue_for({source_lines_size, 1}, MSG)
else
issue_for({source_lines_size, 1}, MSG_FINAL_NEWLINE) do |corrector|
corrector.insert_before({source_lines_size + 1, 1}, '\n')
end
end
end
Expand Down
5 changes: 2 additions & 3 deletions src/ameba/rule/lint/empty_expression.cr
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,8 @@ module Ameba::Rule::Lint
end

def test(source, node : Crystal::Expressions)
if node.expressions.size == 1 && node.expressions.first.nop?
issue_for node, MSG_EXRS
end
return unless node.expressions.size == 1 && node.expressions.first.nop?
issue_for node, MSG_EXRS
end
end
end
5 changes: 2 additions & 3 deletions src/ameba/rule/lint/unreachable_code.cr
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ module Ameba::Rule::Lint
end

def test(source, node, flow_expression : AST::FlowExpression)
if unreachable_node = flow_expression.unreachable_nodes.first?
issue_for unreachable_node, MSG
end
return unless unreachable_node = flow_expression.unreachable_nodes.first?
issue_for unreachable_node, MSG
end
end
end
7 changes: 3 additions & 4 deletions src/ameba/rule/metrics/cyclomatic_complexity.cr
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ module Ameba::Rule::Metrics
def test(source, node : Crystal::Def)
complexity = AST::CountingVisitor.new(node).count

if complexity > max_complexity && (location = node.name_location)
issue_for location, name_end_location(node),
MSG % {complexity, max_complexity}
end
return unless complexity > max_complexity && (location = node.name_location)
issue_for location, name_end_location(node),
MSG % {complexity, max_complexity}
end
end
end
11 changes: 5 additions & 6 deletions src/ameba/rule/style/constant_names.cr
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,13 @@ module Ameba::Rule::Style
MSG = "Constant name should be screaming-cased: %s, not %s"

def test(source, node : Crystal::Assign)
if (target = node.target).is_a?(Crystal::Path)
name = target.names.first
expected = name.upcase
return unless (target = node.target).is_a?(Crystal::Path)
name = target.names.first
expected = name.upcase

return if name.in?(expected, name.camelcase)
return if name.in?(expected, name.camelcase)

issue_for target, MSG % {expected, name}
end
issue_for target, MSG % {expected, name}
end
end
end
183 changes: 183 additions & 0 deletions src/ameba/rule/style/guard_clause.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
module Ameba::Rule::Style
# Use a guard clause instead of wrapping the code inside a conditional
# expression
#
# ```
# # bad
# def test
# if something
# work
# end
# end
#
# # good
# def test
# return unless something
#
# work
# end
#
# # also good
# def test
# work if something
# end
#
# # bad
# if something
# raise "exception"
# else
# ok
# end
#
# # good
# raise "exception" if something
# ok
#
# # bad
# if something
# foo || raise("exception")
# else
# ok
# end
#
# # good
# foo || raise("exception") if something
# ok
# ```
#
# YAML configuration example:
#
# ```
# Style/GuardClause:
# Enabled: true
# ```
class GuardClause < Base
include AST::Util

properties do
enabled false
description "Check for conditionals that can be replaced with guard clauses."
end

MSG = "Use a guard clause (`%s`) instead of wrapping the " \
"code inside a conditional expression."

def test(source)
AST::NodeVisitor.new self, source, skip: [Crystal::Assign]
end

def test(source, node : Crystal::Def)
final_expression =
if (body = node.body).is_a?(Crystal::Expressions)
body.last
else
body
end

case final_expression
when Crystal::If then check_ending_if(source, final_expression)
when Crystal::Unless then check_ending_if(source, final_expression)
end
end

def test(source, node : Crystal::If | Crystal::Unless)
return if accepted_form?(source, node, ending: false)

case
when guard_clause = guard_clause(node.then)
parent, conditional_keyword = node.then, keyword(node)
when guard_clause = guard_clause(node.else)
parent, conditional_keyword = node.else, opposite_keyword(node)
end

return unless guard_clause && parent && conditional_keyword

report_issue(source, node, guard_clause_source(source, guard_clause, parent), conditional_keyword)
end

private def check_ending_if(source, node)
return if accepted_form?(source, node, ending: true)

report_issue(source, node, "return", opposite_keyword(node))
end

private def report_issue(source, node, scope_exiting_keyword, conditional_keyword)
return unless keyword_loc = node.location
return unless cond_code = node_source(node.cond, source.lines)

keyword_end_loc = keyword_loc.adjust(column_number: keyword(node).size - 1)

example = "#{scope_exiting_keyword} #{conditional_keyword} #{cond_code}"
# TODO: check if example is too long for single line

if node.else.is_a?(Crystal::Nop)
return unless end_end_loc = node.end_location

end_loc = end_end_loc.adjust(column_number: {{1 - "end".size}})

issue_for keyword_loc, keyword_end_loc, MSG % example do |corrector|
corrector.replace(keyword_loc, keyword_end_loc, "#{scope_exiting_keyword} #{conditional_keyword}")
corrector.remove(end_loc, end_end_loc)
end
else
issue_for keyword_loc, keyword_end_loc, MSG % example
end
end

private def keyword(node : Crystal::If)
"if"
end

private def keyword(node : Crystal::Unless)
"unless"
end

private def opposite_keyword(node : Crystal::If)
"unless"
end

private def opposite_keyword(node : Crystal::Unless)
"if"
end

private def accepted_form?(source, node, ending)
return true if node.is_a?(Crystal::If) && node.ternary?
return true unless cond_loc = node.cond.location
return true unless cond_end_loc = node.cond.end_location
return true unless cond_loc.line_number == cond_end_loc.line_number
return true unless (then_loc = node.then.location).nil? || cond_loc < then_loc

if ending
!node.else.is_a?(Crystal::Nop)
else
return true if node.else.is_a?(Crystal::Nop)
return true unless code = node_source(node, source.lines)

code.starts_with?("elsif")
end
end

private def guard_clause(node)
node = node.right if node.is_a?(Crystal::And) || node.is_a?(Crystal::Or)

return unless location = node.location
return unless end_location = node.end_location
return unless location.line_number == end_location.line_number

case node
when Crystal::Call
node if node.obj.nil? && node.name == "raise"
when Crystal::Return, Crystal::Break, Crystal::Next
node
end
end

def guard_clause_source(source, guard_clause, parent)
if parent.is_a?(Crystal::And) || parent.is_a?(Crystal::Or)
node_source(parent, source.lines)
else
node_source(guard_clause, source.lines)
end
end
end
end
Loading

0 comments on commit f288cc3

Please sign in to comment.