New rule: shadowed exception (#25)

spec/ameba/rule/shadowed_exception_spec.cr

@@ -0,0 +1,174 @@
+require "../../spec_helper"
+
+private def check_shadowed(source, exceptions)
+  s = Ameba::Source.new source
+  Ameba::Rule::ShadowedException.new.catch(s).should_not be_valid
+  s.errors.first.message.should contain exceptions.join(", ")
+end
+
+module Ameba::Rule
+  describe ShadowedException do
+    subject = ShadowedException.new
+
+    it "passes if there isn't shadowed exception" do
+      s = Source.new %(
+        def method
+          do_something
+        rescue ArgumentError
+          handle_argument_error_exception
+        rescue Exception
+          handle_exception
+        end
+
+        def method
+        rescue Exception
+          handle_exception
+        end
+
+        def method
+        rescue e : ArgumentError
+          handle_argument_error_exception
+        rescue e : Exception
+          handle_exception
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "fails if there is a shadowed exception" do
+      check_shadowed %(
+        begin
+          do_something
+        rescue Exception
+          handle_exception
+        rescue ArgumentError
+          handle_argument_error_exception
+        end
+      ), %w(ArgumentError)
+    end
+
+    it "fails if there is a custom shadowed exceptions" do
+      check_shadowed %(
+        begin
+          1
+        rescue Exception
+          2
+        rescue MySuperException
+          3
+        end
+      ), %w(MySuperException)
+    end
+
+    it "fails if there is a shadowed exception in a type list" do
+      check_shadowed %(
+        begin
+        rescue Exception | IndexError
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there is a first shadowed exception in a type list" do
+      check_shadowed %(
+        begin
+        rescue IndexError | Exception
+        rescue Exception
+        rescue
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there is a shadowed duplicated exception" do
+      check_shadowed %(
+        begin
+        rescue IndexError
+        rescue ArgumentError
+        rescue IndexError
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there is a shadowed duplicated exception in a type list" do
+      check_shadowed %(
+        begin
+        rescue IndexError
+        rescue ArgumentError | IndexError
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there is only shadowed duplicated exceptions" do
+      check_shadowed %(
+        begin
+        rescue IndexError
+        rescue IndexError
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there is only shadowed duplicated exceptions in a type list" do
+      check_shadowed %(
+        begin
+        rescue IndexError | IndexError
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if all rescues are shadowed and there is a catch-all rescue" do
+      check_shadowed %(
+        begin
+        rescue Exception
+        rescue ArgumentError
+        rescue IndexError
+        rescue KeyError | IO::Error
+        rescue
+        end
+      ), %w(IndexError KeyError IO::Error)
+    end
+
+    it "fails if there are shadowed exception with args" do
+      check_shadowed %(
+        begin
+        rescue Exception
+        rescue ex : IndexError
+        rescue
+        end
+      ), %w(IndexError)
+    end
+
+    it "fails if there are multiple shadowed exceptions" do
+      check_shadowed %(
+        begin
+        rescue Exception
+        rescue ArgumentError
+        rescue IndexError
+        end
+      ), %w(ArgumentError IndexError)
+    end
+
+    it "fails if there are multipe shadowed exceptions in a type list" do
+      check_shadowed %(
+        begin
+        rescue Exception
+        rescue ArgumentError | IndexError
+        rescue IO::Error
+        end
+      ), %w(ArgumentError IndexError IO::Error)
+    end
+
+    it "reports rule, location and a message" do
+      s = Source.new %q(
+        begin
+        rescue Exception | IndexError
+        end
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+      error = s.errors.first
+
+      error.rule.should_not be_nil
+      error.location.to_s.should eq "source.cr:2:9"
+      error.message.should eq(
+        "Exception handler has shadowed exceptions: IndexError"
+      )
+    end
+  end
+end

src/ameba/rule/redundant_begin.cr

@@ -48,6 +48,7 @@ module Ameba::Rule
   #   b = 2
   # end
   # ```
+  #
   # YAML configuration example:
   #
   # ```

src/ameba/rule/shadowed_exception.cr

@@ -0,0 +1,80 @@
+module Ameba::Rule
+  # A rule that disallows a rescued exception that get shadowed by a
+  # less specific exception being rescued before a more specific
+  # exception is rescued.
+  #
+  # For example, this is invalid:
+  #
+  # ```
+  # begin
+  #   do_something
+  # rescue Exception
+  #   handle_exception
+  # rescue ArgumentError
+  #   handle_argument_error_exception
+  # end
+  # ```
+  #
+  # And it has to be written as follows:
+  #
+  # ```
+  # begin
+  #   do_something
+  # rescue ArgumentError
+  #   handle_argument_error_exception
+  # rescue Exception
+  #   handle_exception
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # ShadowedException:
+  #   Enabled: true
+  # ```
+  #
+  struct ShadowedException < Base
+    properties do
+      description = "Disallows rescued exception that get shadowed"
+    end
+
+    def test(source)
+      AST::Visitor.new self, source
+    end
+
+    def test(source, node : Crystal::ExceptionHandler)
+      return unless excs = node.rescues
+
+      if (excs = shadowed excs.map(&.types)).any?
+        source.error self, node.location,
+          "Exception handler has shadowed exceptions: #{excs.join(", ")}"
+      end
+    end
+
+    private def shadowed(exceptions, exception_found = false)
+      previous_exceptions = [] of String
+
+      exceptions.reduce([] of String) do |shadowed, excs|
+        excs = excs ? excs.map(&.to_s) : ["Exception"]
+
+        if exception_found
+          shadowed.concat excs
+          previous_exceptions.concat excs
+        else
+          exception_found ||= excs.any? &.== "Exception"
+          excs.each do |exc|
+            if exception_found && exc != "Exception"
+              shadowed << exc
+            else
+              shadowed << exc if previous_exceptions.any? &.== exc
+            end
+            previous_exceptions << exc
+          end
+        end
+
+        shadowed
+      end
+    end
+  end
+end