diff --git a/spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr b/spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr new file mode 100644 index 000000000..81a364666 --- /dev/null +++ b/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 diff --git a/spec/ameba/rule/lint/duplicate_branch_spec.cr b/spec/ameba/rule/lint/duplicate_branch_spec.cr new file mode 100644 index 000000000..e89d34a6f --- /dev/null +++ b/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 diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 2e948660b..f662dab49 100644 --- a/spec/spec_helper.cr +++ b/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 diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 065439fe6..53e1fec61 100644 --- a/src/ameba/ast/util.cr +++ b/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 } diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index ece19b804..21e6dfdcd 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/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 diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 8b513948c..0affad787 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/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 diff --git a/src/ameba/ast/visitors/elseif_aware_node_visitor.cr b/src/ameba/ast/visitors/elseif_aware_node_visitor.cr new file mode 100644 index 000000000..133a7c025 --- /dev/null +++ b/src/ameba/ast/visitors/elseif_aware_node_visitor.cr @@ -0,0 +1,64 @@ +require "./node_visitor" + +module Ameba::AST + # A class that utilizes a logic inherited from `NodeVisitor` to traverse AST + # nodes and fire a source test callback with the `Crystal::If` node and an array + # containing all `elsif` branches (first branch is an `if` node) in case + # at least one `elsif` branch is reached, and `nil` otherwise. + # + # In Crystal, consecutive `elsif` branches are transformed into `if` branches + # attached to the `else` branch of an adjacent `if` branch. + # + # For example: + # + # ``` + # if foo + # do_foo + # elsif bar + # do_bar + # elsif baz + # do_baz + # else + # do_something_else + # end + # ``` + # + # is transformed into: + # + # ``` + # if foo + # do_foo + # else + # if bar + # do_bar + # else + # if baz + # do_baz + # else + # do_something_else + # end + # end + # end + # ``` + class ElseIfAwareNodeVisitor < NodeVisitor + def visit(node : Crystal::If) + if_node = node + ifs = [] of Crystal::If + + loop do + ifs << if_node + + if_node.cond.accept self + if_node.then.accept self + + unless (if_node = if_node.else).is_a?(Crystal::If) + if_node.accept self + break + end + end + + @rule.test @source, node, (ifs if ifs.size > 1) + false + end + end +end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 3ed161d5d..74a336d6b 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -167,19 +167,19 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::Call) scope = @current_scope - case - when (scope.top_level? || scope.type_definition?) && record_macro?(node) - return false - when scope.type_definition? && accessor_macro?(node) - return false + when (scope.top_level? || scope.type_definition?) && record_macro?(node), + scope.type_definition? && accessor_macro?(node) + false when scope.def? && special_node?(node) scope.arguments.each do |arg| ref = arg.variable.reference(scope) ref.explicit = false end + true + else + true end - true end private def special_node?(node) diff --git a/src/ameba/rule/lint/duplicate_branch.cr b/src/ameba/rule/lint/duplicate_branch.cr new file mode 100644 index 000000000..0e9cba9f0 --- /dev/null +++ b/src/ameba/rule/lint/duplicate_branch.cr @@ -0,0 +1,116 @@ +module Ameba::Rule::Lint + # Checks that there are no repeated bodies within `if/unless`, + # `case-when`, `case-in` and `rescue` constructs. + # + # This is considered invalid: + # + # ``` + # if foo + # do_foo + # do_something_else + # elsif bar + # do_foo + # do_something_else + # end + # ``` + # + # And this is valid: + # + # ``` + # if foo || bar + # do_foo + # do_something_else + # end + # ``` + # + # With `IgnoreLiteralBranches: true`, branches are not registered + # as offenses if they return a basic literal value (string, symbol, + # integer, float, `true`, `false`, or `nil`), or return an array, + # hash, regexp or range that only contains one of the above basic + # literal values. + # + # With `IgnoreConstantBranches: true`, branches are not registered + # as offenses if they return a constant value. + # + # With `IgnoreDuplicateElseBranch: true`, in conditionals with multiple branches, + # duplicate 'else' branches are not registered as offenses. + # + # YAML configuration example: + # + # ``` + # Lint/DuplicateBranch: + # Enabled: true + # IgnoreLiteralBranches: false + # IgnoreConstantBranches: false + # IgnoreDuplicateElseBranch: false + # ``` + class DuplicateBranch < Base + include AST::Util + + properties do + since_version "1.7.0" + description "Reports duplicated branch bodies" + + ignore_literal_branches false + ignore_constant_branches false + ignore_duplicate_else_branch false + end + + MSG = "Duplicate branch body detected" + + def test(source) + AST::ElseIfAwareNodeVisitor.new self, source, skip: :macro + end + + def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case | Crystal::ExceptionHandler, ifs : Enumerable(Crystal::If)? = nil) + processed_bodies = Set(String).new + + each_branch(ifs || node) do |body_node| + next if ignore_literal_branches? && static_literal?(body_node) + next if ignore_constant_branches? && body_node.is_a?(Crystal::Path) + + body_s = body_node.to_s + + if processed_bodies.includes?(body_s) + issue_for body_node, MSG + else + processed_bodies << body_s + end + end + end + + private def each_branch(ifs : Enumerable(Crystal::If), &) + ifs.each do |if_node| + yield if_node.then + end + if !ignore_duplicate_else_branch? && (else_node = ifs.last.else) + yield else_node + end + end + + private def each_branch(node : Crystal::If | Crystal::Unless, &) + if !ignore_duplicate_else_branch? && (else_node = node.else) + yield node.then + yield else_node + end + end + + private def each_branch(node : Crystal::Case, &) + node.whens.each do |when_node| + yield when_node.body + end + if !ignore_duplicate_else_branch? && (else_node = node.else) + yield else_node + end + end + + private def each_branch(node : Crystal::ExceptionHandler, &) + node.rescues.try &.each do |rescue_node| + yield rescue_node.body + end + if !ignore_duplicate_else_branch? && (else_node = node.else) + yield else_node + end + end + end +end diff --git a/src/ameba/severity.cr b/src/ameba/severity.cr index 52dd74550..febe52721 100644 --- a/src/ameba/severity.cr +++ b/src/ameba/severity.cr @@ -27,7 +27,7 @@ module Ameba def color : Colorize::Color case self in Error then Colorize::ColorANSI::Red - in Warning then Colorize::ColorANSI::Red + in Warning then Colorize::ColorANSI::Red # ameba:disable Lint/DuplicateBranch in Convention then Colorize::ColorANSI::Blue end end diff --git a/src/ameba/tokenizer.cr b/src/ameba/tokenizer.cr index 86f6aa3bb..a5def1b0e 100644 --- a/src/ameba/tokenizer.cr +++ b/src/ameba/tokenizer.cr @@ -73,11 +73,9 @@ module Ameba block.call token case token.type - when .delimiter_end? - break when .interpolation_start? run_normal_state lexer, break_on_rcurly: true, &block - when .eof? + when .delimiter_end?, .eof? break end end @@ -89,9 +87,7 @@ module Ameba block.call token case token.type - when .string_array_end? - break - when .eof? + when .string_array_end?, .eof? break end end