diff --git a/spec/ameba/rule/style/redundant_return_spec.cr b/spec/ameba/rule/style/redundant_return_spec.cr new file mode 100644 index 00000000..77ddbab1 --- /dev/null +++ b/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 diff --git a/src/ameba/ast/flow_expression.cr b/src/ameba/ast/flow_expression.cr index 80691bee..c947094b 100644 --- a/src/ameba/ast/flow_expression.cr +++ b/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 diff --git a/src/ameba/rule/style/redundant_return.cr b/src/ameba/rule/style/redundant_return.cr new file mode 100644 index 00000000..00202151 --- /dev/null +++ b/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