diff --git a/spec/ameba/rule/shadowed_exception_spec.cr b/spec/ameba/rule/shadowed_exception_spec.cr new file mode 100644 index 00000000..9779944b --- /dev/null +++ b/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 diff --git a/src/ameba/rule/redundant_begin.cr b/src/ameba/rule/redundant_begin.cr index f47c3bc5..1c918fb6 100644 --- a/src/ameba/rule/redundant_begin.cr +++ b/src/ameba/rule/redundant_begin.cr @@ -48,6 +48,7 @@ module Ameba::Rule # b = 2 # end # ``` + # # YAML configuration example: # # ``` diff --git a/src/ameba/rule/shadowed_exception.cr b/src/ameba/rule/shadowed_exception.cr new file mode 100644 index 00000000..19130f41 --- /dev/null +++ b/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