Add Style/GuardClause rule (#254)

spec/ameba/rule/style/guard_clause_spec.cr

@@ -0,0 +1,411 @@
+require "../../../spec_helper"
+
+module Ameba
+  subject = Rule::Style::GuardClause.new
+
+  def it_reports_body(body, *, line = __LINE__)
+    rule = Rule::Style::GuardClause.new
+
+    it "reports an issue if method body is if / unless without else" do
+      source = expect_issue rule, <<-CRYSTAL, line: line
+        def func
+          if something
+        # ^^ error: Use a guard clause (`return unless something`) instead of wrapping the code inside a conditional expression.
+            #{body}
+          end
+        end
+
+        def func
+          unless something
+        # ^^^^^^ error: Use a guard clause (`return if something`) instead of wrapping the code inside a conditional expression.
+            #{body}
+          end
+        end
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL, line: line
+        def func
+          return unless something
+            #{body}
+         #{trailing_whitespace}
+        end
+
+        def func
+          return if something
+            #{body}
+         #{trailing_whitespace}
+        end
+        CRYSTAL
+    end
+
+    it "reports an issue if method body ends with if / unless without else" do
+      source = expect_issue rule, <<-CRYSTAL, line: line
+        def func
+          test
+          if something
+        # ^^ error: Use a guard clause (`return unless something`) instead of wrapping the code inside a conditional expression.
+            #{body}
+          end
+        end
+
+        def func
+          test
+          unless something
+        # ^^^^^^ error: Use a guard clause (`return if something`) instead of wrapping the code inside a conditional expression.
+            #{body}
+          end
+        end
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL, line: line
+        def func
+          test
+          return unless something
+            #{body}
+         #{trailing_whitespace}
+        end
+
+        def func
+          test
+          return if something
+            #{body}
+         #{trailing_whitespace}
+        end
+        CRYSTAL
+    end
+  end
+
+  def it_reports_control_expression(kw, *, line = __LINE__)
+    rule = Rule::Style::GuardClause.new
+
+    it "reports an issue with #{kw} in the if branch" do
+      source = expect_issue rule, <<-CRYSTAL, line: line
+        def func
+          if something
+        # ^^ error: Use a guard clause (`#{kw} if something`) instead of wrapping the code inside a conditional expression.
+            #{kw}
+          else
+            puts "hello"
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source, line: line
+    end
+
+    it "reports an issue with #{kw} in the else branch" do
+      source = expect_issue rule, <<-CRYSTAL, line: line
+        def func
+          if something
+        # ^^ error: Use a guard clause (`#{kw} unless something`) instead of wrapping the code inside a conditional expression.
+          puts "hello"
+          else
+            #{kw}
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source, line: line
+    end
+
+    it "doesn't report an issue if condition has multiple lines" do
+      expect_no_issues rule, <<-CRYSTAL, line: line
+        def func
+          if something &&
+               something_else
+            #{kw}
+          else
+            puts "hello"
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report an issue if #{kw} is inside elsif" do
+      expect_no_issues rule, <<-CRYSTAL, line: line
+        def func
+          if something
+            a
+          elsif something_else
+            #{kw}
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report an issue if #{kw} is inside if..elsif..else..end" do
+      expect_no_issues rule, <<-CRYSTAL, line: line
+        def func
+          if something
+            a
+          elsif something_else
+            b
+          else
+            #{kw}
+          end
+        end
+        CRYSTAL
+    end
+
+    it "doesn't report an issue if control flow expr has multiple lines" do
+      expect_no_issues rule, <<-CRYSTAL, line: line
+        def func
+          if something
+            #{kw} \\
+                  "blah blah blah" \\
+                  "blah blah blah"
+          else
+            puts "hello"
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports an issue if non-control-flow branch has multiple lines" do
+      source = expect_issue rule, <<-CRYSTAL, line: line
+        def func
+          if something
+        # ^^ error: Use a guard clause (`#{kw} if something`) instead of wrapping the code inside a conditional expression.
+            #{kw}
+          else
+            puts "hello" \\
+                 "blah blah blah"
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source, line: line
+    end
+  end
+
+  describe Rule::Style::GuardClause do
+    it_reports_body "work"
+    it_reports_body "# TODO"
+
+    pending "does not report an issue if `else` branch is present but empty" do
+      expect_no_issues subject, <<-CRYSTAL
+        def method
+          if bar = foo
+            puts bar
+          else
+            # nothing
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report an issue if body is if..elsif..end" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          if something
+            a
+          elsif something_else
+            b
+          end
+        end
+        CRYSTAL
+    end
+
+    it "doesn't report an issue if condition has multiple lines" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          if something &&
+               something_else
+            work
+          end
+        end
+
+        def func
+          unless something &&
+                   something_else
+            work
+          end
+        end
+        CRYSTAL
+    end
+
+    it "accepts a method body that is if / unless with else" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          if something
+            work
+          else
+            test
+          end
+        end
+
+        def func
+          unless something
+            work
+          else
+            test
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports an issue when using `|| raise` in `then` branch" do
+      source = expect_issue subject, <<-CRYSTAL
+        def func
+          if something
+        # ^^ error: Use a guard clause (`work || raise("message") if something`) instead of [...]
+            work || raise("message")
+          else
+            test
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source
+    end
+
+    it "reports an issue when using `|| raise` in `else` branch" do
+      source = expect_issue subject, <<-CRYSTAL
+        def func
+          if something
+        # ^^ error: Use a guard clause (`test || raise("message") unless something`) instead of [...]
+            work
+          else
+            test || raise("message")
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source
+    end
+
+    it "reports an issue when using `&& return` in `then` branch" do
+      source = expect_issue subject, <<-CRYSTAL
+        def func
+          if something
+        # ^^ error: Use a guard clause (`work && return if something`) instead of wrapping the code inside a conditional expression.
+            work && return
+          else
+            test
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source
+    end
+
+    it "reports an issue when using `&& return` in `else` branch" do
+      source = expect_issue subject, <<-CRYSTAL
+        def func
+          if something
+        # ^^ error: Use a guard clause (`test && return unless something`) instead of wrapping the code inside a conditional expression.
+            work
+          else
+            test && return
+          end
+        end
+        CRYSTAL
+
+      expect_no_corrections source
+    end
+
+    it "accepts a method body that does not end with if / unless" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          if something
+            work
+          end
+          test
+        end
+
+        def func
+          unless something
+            work
+          end
+          test
+        end
+        CRYSTAL
+    end
+
+    it "accepts a method body that is a modifier if / unless" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          work if something
+        end
+
+        def func
+          work unless something
+        end
+        CRYSTAL
+    end
+
+    it "accepts a method with empty parentheses as its body" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          ()
+        end
+        CRYSTAL
+    end
+
+    it "does not report an issue when assigning the result of a guard condition with `else`" do
+      expect_no_issues subject, <<-CRYSTAL
+        def func
+          result =
+            if something
+              work || raise("message")
+            else
+              test
+            end
+        end
+        CRYSTAL
+    end
+
+    it_reports_control_expression "return"
+    it_reports_control_expression "next"
+    it_reports_control_expression "break"
+    it_reports_control_expression %(raise "error")
+
+    context "method in module" do
+      it "reports an issue for instance method" do
+        source = expect_issue subject, <<-CRYSTAL
+          module CopTest
+            def test
+              if something
+            # ^^ error: Use a guard clause (`return unless something`) instead of wrapping the code inside a conditional expression.
+                work
+              end
+            end
+          end
+          CRYSTAL
+
+        expect_correction source, <<-CRYSTAL
+          module CopTest
+            def test
+              return unless something
+                work
+             #{trailing_whitespace}
+            end
+          end
+          CRYSTAL
+      end
+
+      it "reports an issue for singleton methods" do
+        source = expect_issue subject, <<-CRYSTAL
+          module CopTest
+            def self.test
+              if something && something_else
+            # ^^ error: Use a guard clause (`return unless something && something_else`) instead of [...]
+                work
+              end
+            end
+          end
+          CRYSTAL
+
+        expect_correction source, <<-CRYSTAL
+          module CopTest
+            def self.test
+              return unless something && something_else
+                work
+             #{trailing_whitespace}
+            end
+          end
+          CRYSTAL
+      end
+    end
+  end
+end

src/ameba/ast/variabling/assignment.cr

@@ -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?

src/ameba/ast/visitors/node_visitor.cr

@@ -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

src/ameba/ast/visitors/scope_visitor.cr

@@ -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)
@@ -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:

src/ameba/config.cr

@@ -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`.
@@ -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.

src/ameba/formatter/dot_formatter.cr

@@ -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)

src/ameba/inline_comments.cr

@@ -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)

src/ameba/rule/layout/trailing_blank_lines.cr

@@ -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

src/ameba/rule/lint/empty_expression.cr

@@ -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

src/ameba/rule/lint/unreachable_code.cr

@@ -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

src/ameba/rule/metrics/cyclomatic_complexity.cr

@@ -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

src/ameba/rule/style/constant_names.cr

@@ -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

src/ameba/rule/style/guard_clause.cr

@@ -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

src/ameba/rule/style/predicate_name.cr

@@ -37,12 +37,11 @@ module Ameba::Rule::Style
     MSG = "Favour method name '%s?' over '%s'"
 
     def test(source, node : Crystal::Def)
-      if node.name =~ /^(is|has)_(\w+)\?/
-        alternative = $2
-        return unless alternative =~ /^[a-z][a-zA-Z_0-9]*$/
+      return unless node.name =~ /^(is|has)_(\w+)\?/
+      alternative = $2
+      return unless alternative =~ /^[a-z][a-zA-Z_0-9]*$/
 
-        issue_for node, MSG % {alternative, node.name}
-      end
+      issue_for node, MSG % {alternative, node.name}
     end
   end
 end

src/ameba/rule/style/redundant_begin.cr

@@ -98,9 +98,8 @@ module Ameba::Rule::Style
     end
 
     private def begin_exprs_in_handler?(handler)
-      if (body = handler.body).is_a?(Crystal::Expressions)
-        body.expressions.first?.is_a?(Crystal::ExceptionHandler)
-      end
+      return unless (body = handler.body).is_a?(Crystal::Expressions)
+      body.expressions.first?.is_a?(Crystal::ExceptionHandler)
     end
 
     private def def_redundant_begin_range(source, node)

src/ameba/runner.cr

@@ -221,9 +221,8 @@ module Ameba
     end
 
     private def check_unneeded_directives(source)
-      if (rule = @unneeded_disable_directive_rule) && rule.enabled
-        rule.test(source)
-      end
+      return unless (rule = @unneeded_disable_directive_rule) && rule.enabled
+      rule.test(source)
     end
   end
 end

src/ameba/source/rewriter.cr

@@ -124,9 +124,8 @@ class Ameba::Source
     end
 
     private def check_range_validity(begin_pos, end_pos)
-      if begin_pos < 0 || end_pos > code.size
-        raise IndexError.new("The range #{begin_pos}...#{end_pos} is outside the bounds of the source")
-      end
+      return unless begin_pos < 0 || end_pos > code.size
+      raise IndexError.new("The range #{begin_pos}...#{end_pos} is outside the bounds of the source")
     end
   end
 end

src/ameba/spec/expect_issue.cr

@@ -161,13 +161,12 @@ module Ameba::Spec::ExpectIssue
                        line = __LINE__)
     lines = code.split('\n') # must preserve trailing newline
     _, actual_annotations = actual_annotations(rules, code, path, lines)
-    unless actual_annotations.to_s == code
-      fail <<-MSG, file, line
-        Expected no issues, but got:
+    return if actual_annotations.to_s == code
+    fail <<-MSG, file, line
+      Expected no issues, but got:
 
-        #{actual_annotations}
-        MSG
-    end
+      #{actual_annotations}
+      MSG
   end
 
   private def actual_annotations(rules, code, path, lines)