Merge pull request #134 from crystal-ameba/feat/redundant-next

New rule: RedundantNext

spec/ameba/ast/visitors/redundant_control_expression_visitor_spec.cr

@@ -0,0 +1,26 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  source = Source.new ""
+  rule = RedundantControlExpressionRule.new
+
+  describe RedundantControlExpressionVisitor do
+    node = as_node %(
+      a = 1
+      b = 2
+      return a + b
+    )
+    subject = RedundantControlExpressionVisitor.new(rule, source, node)
+
+    it "assigns valid attributes" do
+      subject.rule.should eq rule
+      subject.source.should eq source
+      subject.node.should eq node
+    end
+
+    it "fires a callback with a valid node" do
+      rule.nodes.size.should eq 1
+      rule.nodes.first.to_s.should eq "return a + b"
+    end
+  end
+end

spec/ameba/rule/style/redundant_next_spec.cr

@@ -0,0 +1,179 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Style
+  subject = RedundantNext.new
+
+  describe RedundantNext do
+    it "does not report if there is no redundant next" do
+      s = Source.new %(
+        array.map { |x| x + 1 }
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if there is redundant next in the block" do
+      s = Source.new %(
+        block do |v|
+          p v + 1
+          next
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports if there is redundant next with argument in the block" do
+      s = Source.new %(
+        block do |v|
+          next v + 1
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    context "if" do
+      it "doesn't report if there is not redundant next in if branch" do
+        s = Source.new %(
+          block do |v|
+            next if v > 10
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there is redundant next in if/else branch" do
+        s = Source.new %(
+          block do |a|
+            if a > 0
+              next a + 1
+            else
+              next a + 2
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":3:5"
+        s.issues.last.location.to_s.should eq ":5:5"
+      end
+    end
+
+    context "unless" do
+      it "doesn't report if there is no redundant next in unless branch" do
+        s = Source.new %(
+          block do |v|
+            next unless v > 10
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there is redundant next in unless/else branch" do
+        s = Source.new %(
+          block do |a|
+            unless a > 0
+              next a + 1
+            else
+              next a + 2
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":3:5"
+        s.issues.last.location.to_s.should eq ":5:5"
+      end
+    end
+
+    context "expressions" do
+      it "doesn't report if there is no redundant next in expressions" do
+        s = Source.new %(
+          block do |v|
+            a = 1
+            a + v
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there is redundant next in expressions" do
+        s = Source.new %(
+          block do |a|
+            a = 1
+            next
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 1
+        s.issues.first.location.to_s.should eq ":3:3"
+      end
+    end
+
+    context "binary-op" do
+      it "doesn't report if there is no redundant next in binary op" do
+        s = Source.new %(
+          block do |v|
+            a && v
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there is redundant next in binary op" do
+        s = Source.new %(
+          block do |a|
+            a && next
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 1
+        s.issues.first.location.to_s.should eq ":2:8"
+      end
+    end
+
+    context "expception handler" do
+      it "doesn't report if there is no redundant next in exception handler" do
+        s = Source.new %(
+          block do |v|
+            v + 1
+          rescue e
+            next if v > 0
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there is redundant next in exception handler" do
+        s = Source.new %(
+          block do |a|
+            next a + 1
+          rescue ArgumentError
+            next a + 2
+          rescue Exception
+            a + 2
+            next
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 3
+        s.issues[0].location.to_s.should eq ":2:3"
+        s.issues[1].location.to_s.should eq ":4:3"
+        s.issues[2].location.to_s.should eq ":7:3"
+      end
+    end
+
+    it "reports correct rule, error message and position" do
+      s = Source.new %(
+        block do |v|
+          next v + 1
+        end
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+      s.issues.size.should eq 1
+      issue = s.issues.first
+      issue.rule.should_not be_nil
+      issue.location.to_s.should eq "source.cr:2:3"
+      issue.end_location.to_s.should eq "source.cr:2:12"
+      issue.message.should eq "Redundant `next` detected"
+    end
+  end
+end

spec/spec_helper.cr

@@ -81,6 +81,17 @@ module Ameba
     end
   end
 
+  struct RedundantControlExpressionRule < Rule::Base
+    getter nodes = [] of Crystal::ASTNode
+
+    def test(source)
+    end
+
+    def test(source, node, visitor : AST::RedundantControlExpressionVisitor)
+      nodes << node
+    end
+  end
+
   # A rule that always raises an error
   struct RaiseRule < Rule::Base
     property should_raise = false
@@ -145,6 +156,7 @@ module Ameba
       Crystal::While,
       Crystal::MacroLiteral,
       Crystal::Expressions,
+      Crystal::ControlExpression,
     ]
 
     def initialize(node)

src/ameba/ast/visitors/node_visitor.cr

@@ -6,6 +6,7 @@ module Ameba::AST
     Alias,
     Assign,
     Call,
+    Block,
     Case,
     ClassDef,
     ClassVar,

src/ameba/ast/visitors/redundant_control_expression_visitor.cr

@@ -0,0 +1,60 @@
+module Ameba::AST
+  # A class that utilizes a logic to traverse AST nodes and
+  # fire a source test callback if a redundant `Crystal::ControlExpression`
+  # is reached.
+  class RedundantControlExpressionVisitor
+    # A corresponding rule that uses this visitor.
+    getter rule : Rule::Base
+
+    # A source that needs to be traversed.
+    getter source : Source
+
+    # A node to run traversal on.
+    getter node : Crystal::ASTNode
+
+    def initialize(@rule, @source, @node)
+      traverse_node node
+    end
+
+    private def traverse_control_expression(node)
+      @rule.test(@source, node, self)
+    end
+
+    private def traverse_node(node)
+      case node
+      when Crystal::ControlExpression   then traverse_control_expression node
+      when Crystal::Expressions         then traverse_expressions node
+      when Crystal::If, Crystal::Unless then traverse_condition node
+      when Crystal::Case                then traverse_case node
+      when Crystal::BinaryOp            then traverse_binary_op node
+      when Crystal::ExceptionHandler    then traverse_exception_handler node
+      end
+    end
+
+    private def traverse_expressions(node)
+      traverse_node node.expressions.last?
+    end
+
+    private def traverse_condition(node)
+      return if node.else.nil? || node.else.nop?
+
+      traverse_node(node.then)
+      traverse_node(node.else)
+    end
+
+    private def traverse_case(node)
+      node.whens.each { |n| traverse_node n.body }
+      traverse_node(node.else)
+    end
+
+    private def traverse_binary_op(node)
+      traverse_node(node.right)
+    end
+
+    private def traverse_exception_handler(node)
+      traverse_node node.body
+      traverse_node node.else
+      node.rescues.try &.each { |n| traverse_node n.body }
+    end
+  end
+end

src/ameba/rule/style/redundant_next.cr

@@ -0,0 +1,74 @@
+module Ameba::Rule::Style
+  # A rule that disallows redundant next expressions. A `next` keyword allows
+  # a block to skip to the next iteration early, however, it is considered
+  # redundant in cases where it is the last expression in a block or combines
+  # into the node which is the last in a block.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # block do |v|
+  #   next v + 1
+  # end
+  # ```
+  #
+  # ```
+  # block do |v|
+  #   case v
+  #   when .nil?
+  #     next "nil"
+  #   when .blank?
+  #     next "blank"
+  #   else
+  #     next "empty"
+  #   end
+  # end
+  # ```
+  #
+  # And has to be written as the following:
+  #
+  # ```
+  # block do |v|
+  #   v + 1
+  # end
+  # ```
+  #
+  # ```
+  # block do |v|
+  #   case arg
+  #   when .nil?
+  #     "nil"
+  #   when .blank?
+  #     "blank"
+  #   else
+  #     "empty"
+  #   end
+  # end
+  # ```
+  #
+  # ### YAML config example
+  #
+  # ```
+  # Style/RedundantNext:
+  #   Enabled: true
+  # ```
+  struct RedundantNext < Base
+    properties do
+      description "Reports redundant next expressions"
+    end
+
+    MSG = "Redundant `next` detected"
+
+    def test(source)
+      AST::NodeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::Block)
+      AST::RedundantControlExpressionVisitor.new(self, source, node.body)
+    end
+
+    def test(source, node : Crystal::Next, visitor : AST::RedundantControlExpressionVisitor)
+      source.try &.add_issue self, node, MSG
+    end
+  end
+end

src/ameba/rule/style/redundant_return.cr

@@ -102,59 +102,19 @@ module Ameba::Rule::Style
 
     MSG = "Redundant `return` detected"
 
-    @source : Source?
-
     def test(source)
       AST::NodeVisitor.new self, source
     end
 
     def test(source, node : Crystal::Def)
-      @source = source
+      AST::RedundantControlExpressionVisitor.new(self, source, node.body)
-      check_node(node.body)
     end
 
-    private def check_node(node)
+    def test(source, node : Crystal::Return, visitor : AST::RedundantControlExpressionVisitor)
-      case node
-      when Crystal::Return              then check_return node
-      when Crystal::Expressions         then check_expressions node
-      when Crystal::If, Crystal::Unless then check_condition node
-      when Crystal::Case                then check_case node
-      when Crystal::BinaryOp            then check_binary_op node
-      when Crystal::ExceptionHandler    then check_exception_handler node
-      end
-    end
-
-    private def check_return(node)
       return if allow_multi_return && node.exp.is_a?(Crystal::TupleLiteral)
       return if allow_empty_return && (node.exp.nil? || node.exp.not_nil!.nop?)
 
-      @source.try &.add_issue self, node, MSG
+      source.try &.add_issue self, node, MSG
-    end
-
-    private def check_expressions(node)
-      check_node node.expressions.last?
-    end
-
-    private def check_condition(node)
-      return if node.else.nil? || node.else.nop?
-
-      check_node(node.then)
-      check_node(node.else)
-    end
-
-    private def check_case(node)
-      node.whens.each { |n| check_node n.body }
-      check_node(node.else)
-    end
-
-    private def check_binary_op(node)
-      check_node(node.right)
-    end
-
-    private def check_exception_handler(node)
-      check_node node.body
-      check_node node.else
-      node.rescues.try &.each { |n| check_node n.body }
     end
   end
 end