Skip to content

Commit

Permalink
Add Lint/DuplicateBranch rule
Browse files Browse the repository at this point in the history
  • Loading branch information
@Sija
Sija committed Feb 25, 2025
1 parent 4cea7eb commit 19818c8
Show file tree
Hide file tree
Showing 5 changed files with 495 additions and 0 deletions.
47 changes: 47 additions & 0 deletions spec/ameba/ast/visitors/elseif_aware_node_visitor_spec.cr
View file
Original file line number Diff line number Diff line change
@@ -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
255 changes: 255 additions & 0 deletions spec/ameba/rule/lint/duplicate_branch_spec.cr
View file
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions spec/spec_helper.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 64 additions & 0 deletions src/ameba/ast/visitors/elseif_aware_node_visitor.cr
View file
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 19818c8

Please sign in to comment.