From 19818c8bb81dd8851479eb0095760a100197dcba Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 19 Feb 2025 03:00:35 +0100 Subject: [PATCH] Add `Lint/DuplicateBranch` rule --- .../elseif_aware_node_visitor_spec.cr | 47 ++++ spec/ameba/rule/lint/duplicate_branch_spec.cr | 255 ++++++++++++++++++ spec/spec_helper.cr | 13 + .../ast/visitors/elseif_aware_node_visitor.cr | 64 +++++ src/ameba/rule/lint/duplicate_branch.cr | 116 ++++++++ 5 files changed, 495 insertions(+) create mode 100644 spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr create mode 100644 spec/ameba/rule/lint/duplicate_branch_spec.cr create mode 100644 src/ameba/ast/visitors/elseif_aware_node_visitor.cr create mode 100644 src/ameba/rule/lint/duplicate_branch.cr 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..41bbeaba3 --- /dev/null +++ b/spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr @@ -0,0 +1,47 @@ +require "../../../spec_helper" + +module Ameba::AST + describe ElseIfAwareNodeVisitor do + rule = ElseIfRule.new + subject = ElseIfAwareNodeVisitor.new rule, Source.new <<-CRYSTAL + # ifs[0] + foo ? bar : baz + + def foo + # ifs[2] + if :foo + foo + elsif :bar + bar + elsif :baz + baz + else + %w[foo bar].each do + # ifs[1] + if :foo + :foo + elsif :bar + :bar + else + :baz + end + end + end + end + CRYSTAL + + it "fires a callback with an array containing an `if` node with an `elsif` branches" do + rule.ifs.size.should eq 3 + rule.ifs[0].should be_nil + + second_ifs = rule.ifs[2].should_not be_nil + second_ifs.size.should eq 3 + second_ifs.map(&.then.to_s).should eq %w[foo bar baz] + + third_ifs = rule.ifs[1].should_not be_nil + third_ifs.size.should eq 2 + third_ifs.map(&.then.to_s).should eq %w[:foo :bar] + third_ifs[-1].else.to_s.should eq ":baz" + 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..58092e94e --- /dev/null +++ b/spec/ameba/rule/lint/duplicate_branch_spec.cr @@ -0,0 +1,255 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + describe DuplicateBranch do + subject = DuplicateBranch.new + + 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..6207069b3 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 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 << ifs + end + end + class FlowExpressionRule < Rule::Base @[YAML::Field(ignore: true)] getter expressions = [] of AST::FlowExpression 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/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