Add Lint/DuplicateBranch rule

spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr

@@ -0,0 +1,69 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  describe ElseIfAwareNodeVisitor do
+    rule = ElseIfRule.new
+    subject = ElseIfAwareNodeVisitor.new rule, Source.new <<-CRYSTAL
+      # rule.ifs[0]
+      foo ? bar : baz
+
+      def foo
+        # rule.ifs[2]
+        if :one
+          1
+        elsif :two
+          2
+        elsif :three
+          3
+        else
+          %w[].each do
+            # rule.ifs[1]
+            if true
+              'a'
+            elsif false
+              'b'
+            else
+              'c'
+            end
+          end
+        end
+      end
+      CRYSTAL
+
+    it "inherits a logic from `NodeVisitor`" do
+      subject.should be_a(NodeVisitor)
+    end
+
+    it "fires a callback for every `if` node, excluding `elsif` branches" do
+      rule.ifs.size.should eq 3
+    end
+
+    it "fires a callback with an array containing an `if` node without an `elsif` branches" do
+      if_node, ifs = rule.ifs[0]
+      if_node.to_s.should eq "foo ? bar : baz"
+
+      ifs.should be_nil
+    end
+
+    it "fires a callback with an array containing an `if` node with multiple `elsif` branches" do
+      if_node, ifs = rule.ifs[2]
+      if_node.cond.to_s.should eq ":one"
+
+      ifs = ifs.should_not be_nil
+      ifs.size.should eq 3
+      ifs.first.should be if_node
+      ifs.map(&.then.to_s).should eq %w[1 2 3]
+    end
+
+    it "fires a callback with an array containing an `if` node with the `else` branch as the last item" do
+      if_node, ifs = rule.ifs[1]
+      if_node.cond.to_s.should eq "true"
+
+      ifs = ifs.should_not be_nil
+      ifs.size.should eq 2
+      ifs.first.should be if_node
+      ifs.map(&.then.to_s).should eq %w['a' 'b']
+      ifs.last.else.to_s.should eq %('c')
+    end
+  end
+end

spec/ameba/rule/lint/duplicate_branch_spec.cr

@@ -0,0 +1,269 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  describe DuplicateBranch do
+    subject = DuplicateBranch.new
+
+    it "does not report different `if` and `else` branch bodies" do
+      expect_no_issues subject, <<-CRYSTAL
+        if :foo
+          1
+        elsif :foo
+          2
+        elsif :foo
+          3
+        else
+          nil
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `if` and `else` branch bodies" do
+      expect_issue subject, <<-CRYSTAL
+        if 1
+          :foo
+        elsif 2
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        elsif 3
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        else
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `if` branch bodies" do
+      expect_issue subject, <<-CRYSTAL
+        if true
+          :foo
+        elsif false
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `else` branch bodies" do
+      expect_issue subject, <<-CRYSTAL
+        if true
+          :foo
+        else
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `else` branch body within `unless`" do
+      expect_issue subject, <<-CRYSTAL
+        unless true
+          :foo
+        else
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `if` / `else` branch bodies nested within `if`" do
+      expect_issue subject, <<-CRYSTAL
+        if true
+          :foo
+        elsif false
+          %w[foo bar].each do
+            if 1
+              :abc
+            elsif 2
+              :abc
+            # ^^^^ error: Duplicate branch body detected
+            else
+              :abc
+            # ^^^^ error: Duplicate branch body detected
+            end
+          end
+          :foo
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `if` / `else` branch bodies nested within `else`" do
+      expect_issue subject, <<-CRYSTAL
+        if true
+          :foo
+        else
+          %w[foo bar].each do
+            if 1
+              :abc
+            elsif 2
+              :abc
+            # ^^^^ error: Duplicate branch body detected
+            else
+              :abc
+            # ^^^^ error: Duplicate branch body detected
+            end
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated `else` branch bodies within a ternary `if`" do
+      expect_issue subject, <<-CRYSTAL
+        true ? :foo : :foo
+                    # ^^^^ error: Duplicate branch body detected
+        CRYSTAL
+    end
+
+    it "reports duplicated `case` branch bodies" do
+      expect_issue subject, <<-CRYSTAL
+        case
+        when true
+          :foo
+        when false
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    it "reports duplicated exception handler branch bodies" do
+      expect_issue subject, <<-CRYSTAL
+        begin
+          :foo
+        rescue ArgumentError
+          :foo
+        rescue OverflowError
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        else
+          :foo
+        # ^^^^ error: Duplicate branch body detected
+        end
+        CRYSTAL
+    end
+
+    context "properties" do
+      context "#ignore_literal_branches" do
+        it "when disabled reports duplicated (static) literal branch bodies" do
+          rule = DuplicateBranch.new
+          rule.ignore_literal_branches = false
+
+          expect_issue rule, <<-CRYSTAL
+            true ? :foo : :foo
+                        # ^^^^ error: Duplicate branch body detected
+            true ? "foo" : "foo"
+                         # ^^^^^ error: Duplicate branch body detected
+            true ? 123 : 123
+                       # ^^^ error: Duplicate branch body detected
+            true ? [1, 2, 3] : [1, 2, 3]
+                             # ^^^^^^^^^ error: Duplicate branch body detected
+            true ? [foo, bar, baz] : [foo, bar, baz]
+                                   # ^^^^^^^^^^^^^^^ error: Duplicate branch body detected
+            CRYSTAL
+        end
+
+        it "when enabled does not report duplicated (static) literal branch bodies" do
+          rule = DuplicateBranch.new
+          rule.ignore_literal_branches = true
+
+          # static literals
+          expect_no_issues rule, <<-CRYSTAL
+            true ? :foo : :foo
+            true ? "foo" : "foo"
+            true ? 123 : 123
+            true ? [1, 2, 3] : [1, 2, 3]
+            true ? {foo: "bar"} : {foo: "bar"}
+            CRYSTAL
+
+          # dynamic literals
+          expect_issue rule, <<-CRYSTAL
+            true ? [foo, bar, baz] : [foo, bar, baz]
+                                   # ^^^^^^^^^^^^^^^ error: Duplicate branch body detected
+            CRYSTAL
+        end
+      end
+
+      context "#ignore_constant_branches" do
+        it "when disabled reports constant branch bodies" do
+          rule = DuplicateBranch.new
+          rule.ignore_constant_branches = false
+
+          expect_issue rule, <<-CRYSTAL
+            true ? FOO : FOO
+                       # ^^^ error: Duplicate branch body detected
+            true ? Foo::Bar : Foo::Bar
+                            # ^^^^^^^^ error: Duplicate branch body detected
+            CRYSTAL
+        end
+
+        it "when enabled does not report constant branch bodies" do
+          rule = DuplicateBranch.new
+          rule.ignore_constant_branches = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            true ? FOO : FOO
+            true ? Foo::Bar : Foo::Bar
+            CRYSTAL
+        end
+      end
+
+      context "#ignore_duplicate_else_branch" do
+        rule = DuplicateBranch.new
+        rule.ignore_duplicate_else_branch = true
+
+        context "when enabled does not report duplicated `else` branch bodies" do
+          it "in `if`" do
+            expect_no_issues rule, <<-CRYSTAL
+              if true
+                :foo
+              else
+                :foo
+              end
+              CRYSTAL
+          end
+
+          it "in ternary `if`" do
+            expect_no_issues rule, <<-CRYSTAL
+              true ? :foo : :foo
+              CRYSTAL
+          end
+
+          it "in `unless`" do
+            expect_no_issues rule, <<-CRYSTAL
+              unless true
+                :foo
+              else
+                :foo
+              end
+              CRYSTAL
+          end
+
+          it "in `case`" do
+            expect_no_issues rule, <<-CRYSTAL
+              case
+              when true
+                :foo
+              else
+                :foo
+              end
+              CRYSTAL
+          end
+
+          it "in exception handler" do
+            expect_no_issues rule, <<-CRYSTAL
+              begin
+                :foo
+              rescue ArgumentError
+                :foo
+              else
+                :foo
+              end
+              CRYSTAL
+          end
+        end
+      end
+    end
+  end
+end

spec/spec_helper.cr

@@ -76,6 +76,19 @@ module Ameba
     end
   end
 
+  class ElseIfRule < Rule::Base
+    @[YAML::Field(ignore: true)]
+    getter ifs = [] of {Crystal::If, Array(Crystal::If)?}
+
+    properties do
+      description "Internal rule to test else if branches"
+    end
+
+    def test(source, node : Crystal::If, ifs : Array(Crystal::If)? = nil)
+      @ifs << {node, ifs}
+    end
+  end
+
   class FlowExpressionRule < Rule::Base
     @[YAML::Field(ignore: true)]
     getter expressions = [] of AST::FlowExpression

src/ameba/ast/util.cr

@@ -149,9 +149,7 @@ module Ameba::AST::Util
       flow_expressions? [node.whens, node.else].flatten, in_loop
     when Crystal::ExceptionHandler
       flow_expressions? [node.else || node.body, node.rescues].flatten, in_loop
-    when Crystal::While, Crystal::Until
-      flow_expression? node.body, in_loop
-    when Crystal::Rescue, Crystal::When
+    when Crystal::While, Crystal::Until, Crystal::Rescue, Crystal::When
       flow_expression? node.body, in_loop
     when Crystal::Expressions
       node.expressions.any? { |exp| flow_expression? exp, in_loop }

src/ameba/ast/variabling/assignment.cr

@@ -61,9 +61,8 @@ module Ameba::AST
     # Returns the target node of the variable in this assignment.
     def target_node
       case assign = node
-      when Crystal::Assign           then assign.target
-      when Crystal::OpAssign         then assign.target
-      when Crystal::UninitializedVar then assign.var
+      when Crystal::UninitializedVar          then assign.var
+      when Crystal::Assign, Crystal::OpAssign then assign.target
       when Crystal::MultiAssign
         assign.targets.find(node) do |target|
           target.is_a?(Crystal::Var) && target.name == variable.name

src/ameba/ast/variabling/variable.cr

@@ -137,10 +137,10 @@ module Ameba::AST
     # `false` otherwise.
     def target_of?(assign)
       case assign
-      when Crystal::Assign           then eql?(assign.target)
-      when Crystal::OpAssign         then eql?(assign.target)
-      when Crystal::MultiAssign      then assign.targets.any? { |target| eql?(target) }
-      when Crystal::UninitializedVar then eql?(assign.var)
+      when Crystal::UninitializedVar          then eql?(assign.var)
+      when Crystal::Assign, Crystal::OpAssign then eql?(assign.target)
+      when Crystal::MultiAssign
+        assign.targets.any? { |target| eql?(target) }
       else
         false
       end