Basic implementation of UnreachableCode rule

spec/ameba/ast/flow_expression_spec.cr

@@ -0,0 +1,61 @@
+require "../../spec_helper"
+
+module Ameba::AST
+  describe FlowExpression do
+    describe "#initialize" do
+      it "creates a new flow expression" do
+        node = as_node("return 22")
+        parent_node = as_node("def foo; return 22; end")
+        flow_expression = FlowExpression.new node, parent_node
+        flow_expression.node.should_not be_nil
+        flow_expression.parent_node.should_not be_nil
+      end
+
+      describe "#delegation" do
+        it "delegates to_s to @node" do
+          node = as_node("return 22")
+          parent_node = as_node("def foo; return 22; end")
+          flow_expression = FlowExpression.new node, parent_node
+          flow_expression.to_s.should eq node.to_s
+        end
+
+        it "delegates location to @node" do
+          node = as_node %(break if true)
+          parent_node = as_node("def foo; return 22 if true; end")
+          flow_expression = FlowExpression.new node, parent_node
+          flow_expression.location.should eq node.location
+        end
+      end
+
+      describe "#find_unreachable_node" do
+        it "returns first unreachable node" do
+          nodes = as_nodes %(
+            def foobar
+              return
+              a = 1
+              a + 1
+            end
+          )
+          node = nodes.control_expression_nodes.first
+          assign_node = nodes.assign_nodes.first
+          def_node = nodes.def_nodes.first
+          flow_expression = FlowExpression.new node, def_node
+          flow_expression.find_unreachable_node.should eq assign_node
+        end
+
+        it "returns nil if there is no unreachable node" do
+          nodes = as_nodes %(
+            def foobar
+              a = 1
+              return a
+            end
+          )
+          node = nodes.control_expression_nodes.first
+          def_node = nodes.def_nodes.first
+          flow_expression = FlowExpression.new node, def_node
+          flow_expression.find_unreachable_node.should eq nil
+        end
+      end
+    end
+  end
+end

spec/ameba/ast/visitors/flow_expression_visitor_spec.cr

@@ -0,0 +1,66 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  source = Source.new ""
+
+  describe FlowExpressionVisitor do
+    it "creates an expression for return" do
+      rule = FlowExpressionRule.new
+      FlowExpressionVisitor.new rule, Source.new %(
+        def foo
+          return :bar
+        end
+      )
+      rule.expressions.size.should eq 1
+    end
+
+    it "can create multiple expressions" do
+      rule = FlowExpressionRule.new
+      FlowExpressionVisitor.new rule, Source.new %(
+        def foo
+          if bar
+            return :baz
+          else
+            return :foobar
+          end
+        end
+      )
+      rule.expressions.size.should eq 2
+    end
+
+    it "properly creates nested flow expressions" do
+      rule = FlowExpressionRule.new
+      FlowExpressionVisitor.new rule, Source.new %(
+        def foo
+          return(
+            loop do
+              break if a > 1
+              return a
+            end
+          )
+        end
+      )
+      rule.expressions.size.should eq 3
+    end
+
+    it "creates an expression for break" do
+      rule = FlowExpressionRule.new
+      FlowExpressionVisitor.new rule, Source.new %(
+        while true
+          break
+        end
+      )
+      rule.expressions.size.should eq 1
+    end
+
+    it "creates an expression for next" do
+      rule = FlowExpressionRule.new
+      FlowExpressionVisitor.new rule, Source.new %(
+        while true
+          next if something
+        end
+      )
+      rule.expressions.size.should eq 1
+    end
+  end
+end

spec/ameba/rule/lint/unreachable_code_spec.cr

@@ -0,0 +1,102 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  subject = UnreachableCode.new
+
+  describe UnreachableCode do
+    context "return" do
+      it "reports if there is unreachable code after return" do
+        s = Source.new %(
+          def foo
+            a = 1
+            return false
+            b = 2
+          end
+        )
+        subject.catch(s).should_not be_valid
+
+        issue = s.issues.first
+        issue.location.to_s.should eq ":4:3"
+      end
+
+      it "doesn't report if there is return in if" do
+        s = Source.new %(
+          def foo
+            a = 1
+            return false if bar
+            b = 2
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if there are returns in if-then-else" do
+        s = Source.new %(
+          if a > 0
+            return :positivie
+          else
+            return :negative
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+    end
+
+    context "break" do
+      it "reports if there is unreachable code after break" do
+        s = Source.new %(
+          def foo
+            loop do
+              break
+              a = 1
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+
+        issue = s.issues.first
+        issue.location.to_s.should eq ":4:5"
+      end
+
+      it "doesn't report if break is in a condition" do
+        s = Source.new %(
+          a = -100
+          while true
+            break if a > 0
+            a += 1
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+    end
+
+    context "next" do
+      it "reports if there is unreachable code after next" do
+        s = Source.new %(
+          a = 1
+          while a < 5
+            next
+            puts a
+          end
+        )
+        subject.catch(s).should_not be_valid
+
+        issue = s.issues.first
+        issue.location.to_s.should eq ":4:3"
+      end
+
+      it "doesn't report if next is in a condition" do
+        s = Source.new %(
+          a = 1
+          while a < 5
+            if a == 3
+              next
+            end
+            puts a
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+    end
+  end
+end

spec/spec_helper.cr

@@ -65,6 +65,17 @@ module Ameba
     end
   end
 
+  struct FlowExpressionRule < Rule::Base
+    getter expressions = [] of AST::FlowExpression
+
+    def test(source)
+    end
+
+    def test(source, node, flow_expression : AST::FlowExpression)
+      @expressions << flow_expression
+    end
+  end
+
   class DummyFormatter < Formatter::BaseFormatter
     property started_sources : Array(Source)?
     property finished_sources : Array(Source)?
@@ -118,6 +129,7 @@ module Ameba
       Crystal::If,
       Crystal::While,
       Crystal::MacroLiteral,
+      Crystal::ControlExpression,
     ]
 
     def initialize(node)

src/ameba/ast/flow_expression.cr

@@ -0,0 +1,79 @@
+module Ameba::AST
+  # Represents a flow expression in Crystal code.
+  # For example,
+  #
+  # ```
+  # def foobar
+  #   a = 3
+  #   return 42 # => flow expression
+  #   a + 1
+  # end
+  # ```
+  #
+  # Flow expression contains an actual node of a control expression and
+  # a parent node, which allows easily search through the related statement
+  # (i.e. find unreachable code)
+  class FlowExpression
+    # The actual node of the flow expression.
+    getter node : Crystal::ASTNode
+
+    # Parent ast node.
+    getter parent_node : Crystal::ASTNode
+
+    delegate to_s, to: @node
+    delegate location, to: @node
+
+    # Creates a new flow expression.
+    #
+    # ```
+    # FlowExpression.new(node, parent_node)
+    # ```
+    def initialize(@node, @parent_node)
+    end
+
+    # Returns first node which can't be reached because of a flow expression.
+    # For example:
+    #
+    # ```
+    # def foobar
+    #   a = 1
+    #   return 42
+    #
+    #   a + 2 # => unreachable assign node
+    # end
+    # ```
+    #
+    def find_unreachable_node
+      UnreachableNodeVisitor.new(node, parent_node)
+        .tap(&.accept parent_node)
+        .unreachable_nodes
+        .first?
+    end
+
+    # :nodoc:
+    private class UnreachableNodeVisitor < Crystal::Visitor
+      getter unreachable_nodes = Array(Crystal::ASTNode).new
+      @after_control_flow_node = false
+      @branch : AST::Branch?
+
+      def initialize(@node : Crystal::ASTNode, parent_node)
+        @branch = Branch.of(@node, parent_node)
+      end
+
+      def visit(node : Crystal::ASTNode)
+        if node.class == @node.class &&
+           node.location == @node.location
+          @after_control_flow_node = true
+          return false
+        end
+
+        if @after_control_flow_node && !node.nop? && @branch.nil?
+          @unreachable_nodes << node
+          return false
+        end
+
+        true
+      end
+    end
+  end
+end

src/ameba/ast/visitors/flow_expression_visitor.cr

@@ -0,0 +1,33 @@
+require "./base_visitor"
+
+module Ameba::AST
+  # AST Visitor that traverses all the flow expressions.
+  class FlowExpressionVisitor < BaseVisitor
+    @node_stack = Array(Crystal::ASTNode).new
+
+    def initialize(@rule, @source)
+      @source.ast.accept self
+    end
+
+    def visit(node)
+      @node_stack.push node
+    end
+
+    def end_visit(node)
+      @node_stack.pop
+    end
+
+    def visit(node : Crystal::ControlExpression)
+      if parent_node = @node_stack.last?
+        flow_expression = FlowExpression.new(node, parent_node)
+        @rule.test @source, node, flow_expression
+      end
+
+      true
+    end
+
+    def end_visit(node : Crystal::ControlExpression)
+      #
+    end
+  end
+end

src/ameba/rule/lint/unreachable_code.cr

@@ -0,0 +1,62 @@
+module Ameba::Rule::Lint
+  # A rule that reports unreachable code.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # def method(a)
+  #   return 42
+  #   a + 1
+  # end
+  # ```
+  #
+  # ```
+  # a = 1
+  # loop do
+  #   break
+  #   a += 1
+  # end
+  # ```
+  #
+  # And has to be written as the following:
+  #
+  # ```
+  # def method(a)
+  #   return 42 if a == 0
+  #   a + 1
+  # end
+  # ```
+  #
+  # ```
+  # a = 1
+  # loop do
+  #   break a > 3
+  #   a += 1
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/UnreachableCode:
+  #   Enabled: true
+  # ```
+  #
+  struct UnreachableCode < Base
+    properties do
+      description "Reports unreachable code"
+    end
+
+    MSG = "Unreachable code detected"
+
+    def test(source)
+      AST::FlowExpressionVisitor.new self, source
+    end
+
+    def test(source, node, flow_expression : AST::FlowExpression)
+      if unreachable_node = flow_expression.find_unreachable_node
+        issue_for unreachable_node, MSG
+      end
+    end
+  end
+end