Redundant return (#87)

spec/ameba/rule/style/redundant_return_spec.cr

@@ -0,0 +1,246 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Style
+  subject = RedundantReturn.new
+
+  describe RedundantReturn do
+    it "does not report if there is no return" do
+      s = Source.new %(
+        def inc(a)
+          a + 1
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if there is redundant return in method body" do
+      s = Source.new %(
+        def inc(a)
+          return a + 1
+        end
+      )
+      subject.catch(s).should_not be_valid
+      s.issues.size.should eq 1
+      s.issues.first.location.to_s.should eq ":2:3"
+    end
+
+    it "doesn't report if it returns tuple literal" do
+      s = Source.new %(
+        def foo(a)
+          return a, a + 2
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if there are other expressions after control flow" do
+      s = Source.new %(
+        def method(a)
+          case a
+          when true then return true
+          when .nil? then return :nil
+          end
+          false
+        rescue
+          nil
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    context "if" do
+      it "doesn't report if there is return in if branch" do
+        s = Source.new %(
+          def inc(a)
+            return a + 1 if a > 0
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there are returns in if/else branch" do
+        s = Source.new %(
+          def inc(a)
+            do_something(a)
+            if a > 0
+              return :positive
+            else
+              return :negative
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":4:5"
+        s.issues.last.location.to_s.should eq ":6:5"
+      end
+    end
+
+    context "unless" do
+      it "doesn't report if there is return in unless branch" do
+        s = Source.new %(
+          def inc(a)
+            return a + 1 unless a > 0
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if there are returns in unless/else branch" do
+        s = Source.new %(
+          def inc(a)
+            do_something(a)
+            unless a < 0
+              return :positive
+            else
+              return :negative
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":4:5"
+        s.issues.last.location.to_s.should eq ":6:5"
+      end
+    end
+
+    context "case" do
+      it "reports if there are returns in whens" do
+        s = Source.new %(
+          def foo(a)
+            case a
+            when .nil?
+              puts "blah"
+              return nil
+            when .blank?
+              return ""
+            when true
+              true
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":5:5"
+        s.issues.last.location.to_s.should eq ":7:5"
+      end
+
+      it "reports if there is return in else" do
+        s = Source.new %(
+          def foo(a)
+            do_something_with(a)
+
+            case a
+            when true
+              true
+            else
+              return false
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 1
+        s.issues.first.location.to_s.should eq ":8:5"
+      end
+    end
+
+    context "exception handler" do
+      it "reports if there are returns in body" do
+        s = Source.new %(
+          def foo(a)
+            return true
+          rescue
+            false
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 1
+        s.issues.first.location.to_s.should eq ":2:3"
+      end
+
+      it "reports if there are returns in rescues" do
+        s = Source.new %(
+          def foo(a)
+            true
+          rescue ArgumentError
+            return false
+          rescue RuntimeError
+            ""
+          rescue Exception
+            return nil
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 2
+        s.issues.first.location.to_s.should eq ":4:3"
+        s.issues.last.location.to_s.should eq ":8:3"
+      end
+
+      it "reports if there are returns in else" do
+        s = Source.new %(
+          def foo(a)
+            true
+          rescue Exception
+            nil
+          else
+            puts "else branch"
+            return :bar
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.size.should eq 1
+        s.issues.first.location.to_s.should eq ":7:3"
+      end
+    end
+
+    context "properties" do
+      context "#allow_multi_return=" do
+        it "allows multi returns by default" do
+          s = Source.new %(
+            def method(a, b)
+              return a, b
+            end
+          )
+          subject.catch(s).should be_valid
+        end
+
+        it "allows to configure multi returns" do
+          s = Source.new %(
+            def method(a, b)
+              return a, b
+            end
+          )
+          rule = Rule::Style::RedundantReturn.new
+          rule.allow_multi_return = false
+          rule.catch(s).should_not be_valid
+          s.issues.size.should eq 1
+          s.issues.first.location.to_s.should eq ":2:3"
+        end
+      end
+
+      context "#allow_empty_return" do
+        it "allows empty returns by default" do
+          s = Source.new %(
+            def method
+              return
+            end
+          )
+          subject.catch(s).should be_valid
+        end
+
+        it "allows to configure empty returns" do
+          s = Source.new %(
+            def method
+              return
+            end
+          )
+          rule = Rule::Style::RedundantReturn.new
+          rule.allow_empty_return = false
+          rule.catch(s).should_not be_valid
+          s.issues.size.should eq 1
+          s.issues.first.location.to_s.should eq ":2:3"
+        end
+      end
+    end
+  end
+end

src/ameba/ast/flow_expression.cr

@@ -46,6 +46,7 @@ module Ameba::AST
     #
     #   a + 2 # => unreachable assign node
     # end
+    # ```
     def unreachable_nodes
       unreachable_nodes = [] of Crystal::ASTNode
 

src/ameba/rule/style/redundant_return.cr

@@ -0,0 +1,155 @@
+module Ameba::Rule::Style
+  # A rule that disallows redundant return expressions.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # def foo
+  #   return :bar
+  # end
+  # ```
+  #
+  # ```
+  # def bar(arg)
+  #   case arg
+  #   when .nil?
+  #     return "nil"
+  #   when .blank?
+  #     return "blank"
+  #   else
+  #     return "empty"
+  #   end
+  # end
+  # ```
+  #
+  # And has to be written as the following:
+  #
+  # ```
+  # def foo
+  #   :bar
+  # end
+  # ```
+  #
+  # ```
+  # def bar(arg)
+  #   case arg
+  #   when .nil?
+  #     "nil"
+  #   when .blank?
+  #     "blank"
+  #   else
+  #     "empty"
+  #   end
+  # end
+  # ```
+  #
+  # ### Configuration params
+  #
+  # 1. *allow_multi_return*, default: true
+  #
+  # Allows end-user to configure whether to report or not the return statements
+  # which return tuple literals i.e.
+  #
+  # ```
+  # def method(a, b)
+  #   return a, b
+  # end
+  # ```
+  #
+  # If this param equals to `false`, the method above has to be written as:
+  #
+  # ```
+  # def method(a, b)
+  #   {a, b}
+  # end
+  # ```
+  #
+  # 2. *allow_empty_return*, default: true
+  #
+  # Allows end-user to configure whether to report or not the return statements
+  # without arguments. Sometimes such returns are used to return the `nil` value explicitly.
+  #
+  # ```
+  # def method
+  #   @foo = :empty
+  #   return
+  # end
+  # ```
+  #
+  # If this param equals to `false`, the method above has to be written as:
+  #
+  # ```
+  # def method
+  #   @foo = :empty
+  #   nil
+  # end
+  # ```
+  #
+  # ### YAML config example
+  #
+  # ```
+  # Style/RedundantReturn:
+  #   Enabled: true
+  #   AllowMutliReturn: true
+  #   AllowEmptyReturn: true
+  # ```
+  struct RedundantReturn < Base
+    properties do
+      description "Reports redundant return expressions"
+      allow_multi_return true
+      allow_empty_return true
+    end
+
+    MSG = "Redundant `return` detected"
+
+    @source : Source?
+
+    def test(source)
+      AST::NodeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::Def)
+      @source = source
+      check_node(node.body)
+    end
+
+    private def check_node(node)
+      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::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
+    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_exception_handler(node)
+      check_node node.body
+      check_node node.else
+      node.rescues.try &.each { |n| check_node n.body }
+    end
+  end
+end