diff --git a/spec/ameba/ast/flow_expression_spec.cr b/spec/ameba/ast/flow_expression_spec.cr new file mode 100644 index 00000000..fe982463 --- /dev/null +++ b/spec/ameba/ast/flow_expression_spec.cr @@ -0,0 +1,61 @@ +require "../../spec_helper" + +module Ameba::AST + describe FlowExpression do + describe "#initialize" do + it "creates a new flow expression" do + node = as_node("return 22") + parent_node = as_node("def foo; return 22; end") + flow_expression = FlowExpression.new node, parent_node + flow_expression.node.should_not be_nil + flow_expression.parent_node.should_not be_nil + end + + describe "#delegation" do + it "delegates to_s to @node" do + node = as_node("return 22") + parent_node = as_node("def foo; return 22; end") + flow_expression = FlowExpression.new node, parent_node + flow_expression.to_s.should eq node.to_s + end + + it "delegates location to @node" do + node = as_node %(break if true) + parent_node = as_node("def foo; return 22 if true; end") + flow_expression = FlowExpression.new node, parent_node + flow_expression.location.should eq node.location + end + end + + describe "#find_unreachable_node" do + it "returns first unreachable node" do + nodes = as_nodes %( + def foobar + return + a = 1 + a + 1 + end + ) + node = nodes.control_expression_nodes.first + assign_node = nodes.assign_nodes.first + def_node = nodes.def_nodes.first + flow_expression = FlowExpression.new node, def_node + flow_expression.find_unreachable_node.should eq assign_node + end + + it "returns nil if there is no unreachable node" do + nodes = as_nodes %( + def foobar + a = 1 + return a + end + ) + node = nodes.control_expression_nodes.first + def_node = nodes.def_nodes.first + flow_expression = FlowExpression.new node, def_node + flow_expression.find_unreachable_node.should eq nil + end + end + end + end +end diff --git a/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr b/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr new file mode 100644 index 00000000..b8ce338a --- /dev/null +++ b/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr @@ -0,0 +1,66 @@ +require "../../../spec_helper" + +module Ameba::AST + source = Source.new "" + + describe FlowExpressionVisitor do + it "creates an expression for return" do + rule = FlowExpressionRule.new + FlowExpressionVisitor.new rule, Source.new %( + def foo + return :bar + end + ) + rule.expressions.size.should eq 1 + end + + it "can create multiple expressions" do + rule = FlowExpressionRule.new + FlowExpressionVisitor.new rule, Source.new %( + def foo + if bar + return :baz + else + return :foobar + end + end + ) + rule.expressions.size.should eq 2 + end + + it "properly creates nested flow expressions" do + rule = FlowExpressionRule.new + FlowExpressionVisitor.new rule, Source.new %( + def foo + return( + loop do + break if a > 1 + return a + end + ) + end + ) + rule.expressions.size.should eq 3 + end + + it "creates an expression for break" do + rule = FlowExpressionRule.new + FlowExpressionVisitor.new rule, Source.new %( + while true + break + end + ) + rule.expressions.size.should eq 1 + end + + it "creates an expression for next" do + rule = FlowExpressionRule.new + FlowExpressionVisitor.new rule, Source.new %( + while true + next if something + end + ) + rule.expressions.size.should eq 1 + end + end +end diff --git a/spec/ameba/rule/lint/unreachable_code_spec.cr b/spec/ameba/rule/lint/unreachable_code_spec.cr new file mode 100644 index 00000000..4a30272d --- /dev/null +++ b/spec/ameba/rule/lint/unreachable_code_spec.cr @@ -0,0 +1,102 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + subject = UnreachableCode.new + + describe UnreachableCode do + context "return" do + it "reports if there is unreachable code after return" do + s = Source.new %( + def foo + a = 1 + return false + b = 2 + end + ) + subject.catch(s).should_not be_valid + + issue = s.issues.first + issue.location.to_s.should eq ":4:3" + end + + it "doesn't report if there is return in if" do + s = Source.new %( + def foo + a = 1 + return false if bar + b = 2 + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if there are returns in if-then-else" do + s = Source.new %( + if a > 0 + return :positivie + else + return :negative + end + ) + subject.catch(s).should be_valid + end + end + + context "break" do + it "reports if there is unreachable code after break" do + s = Source.new %( + def foo + loop do + break + a = 1 + end + end + ) + subject.catch(s).should_not be_valid + + issue = s.issues.first + issue.location.to_s.should eq ":4:5" + end + + it "doesn't report if break is in a condition" do + s = Source.new %( + a = -100 + while true + break if a > 0 + a += 1 + end + ) + subject.catch(s).should be_valid + end + end + + context "next" do + it "reports if there is unreachable code after next" do + s = Source.new %( + a = 1 + while a < 5 + next + puts a + end + ) + subject.catch(s).should_not be_valid + + issue = s.issues.first + issue.location.to_s.should eq ":4:3" + end + + it "doesn't report if next is in a condition" do + s = Source.new %( + a = 1 + while a < 5 + if a == 3 + next + end + puts a + end + ) + subject.catch(s).should be_valid + end + end + end +end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 4c771642..e99a87c0 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -65,6 +65,17 @@ module Ameba end end + struct FlowExpressionRule < Rule::Base + getter expressions = [] of AST::FlowExpression + + def test(source) + end + + def test(source, node, flow_expression : AST::FlowExpression) + @expressions << flow_expression + end + end + class DummyFormatter < Formatter::BaseFormatter property started_sources : Array(Source)? property finished_sources : Array(Source)? @@ -118,6 +129,7 @@ module Ameba Crystal::If, Crystal::While, Crystal::MacroLiteral, + Crystal::ControlExpression, ] def initialize(node) diff --git a/src/ameba/ast/flow_expression.cr b/src/ameba/ast/flow_expression.cr new file mode 100644 index 00000000..8c7bae16 --- /dev/null +++ b/src/ameba/ast/flow_expression.cr @@ -0,0 +1,79 @@ +module Ameba::AST + # Represents a flow expression in Crystal code. + # For example, + # + # ``` + # def foobar + # a = 3 + # return 42 # => flow expression + # a + 1 + # end + # ``` + # + # Flow expression contains an actual node of a control expression and + # a parent node, which allows easily search through the related statement + # (i.e. find unreachable code) + class FlowExpression + # The actual node of the flow expression. + getter node : Crystal::ASTNode + + # Parent ast node. + getter parent_node : Crystal::ASTNode + + delegate to_s, to: @node + delegate location, to: @node + + # Creates a new flow expression. + # + # ``` + # FlowExpression.new(node, parent_node) + # ``` + def initialize(@node, @parent_node) + end + + # Returns first node which can't be reached because of a flow expression. + # For example: + # + # ``` + # def foobar + # a = 1 + # return 42 + # + # a + 2 # => unreachable assign node + # end + # ``` + # + def find_unreachable_node + UnreachableNodeVisitor.new(node, parent_node) + .tap(&.accept parent_node) + .unreachable_nodes + .first? + end + + # :nodoc: + private class UnreachableNodeVisitor < Crystal::Visitor + getter unreachable_nodes = Array(Crystal::ASTNode).new + @after_control_flow_node = false + @branch : AST::Branch? + + def initialize(@node : Crystal::ASTNode, parent_node) + @branch = Branch.of(@node, parent_node) + end + + def visit(node : Crystal::ASTNode) + if node.class == @node.class && + node.location == @node.location + @after_control_flow_node = true + return false + end + + if @after_control_flow_node && !node.nop? && @branch.nil? + @unreachable_nodes << node + return false + end + + true + end + end + end +end diff --git a/src/ameba/ast/visitors/flow_expression_visitor.cr b/src/ameba/ast/visitors/flow_expression_visitor.cr new file mode 100644 index 00000000..88b3c72d --- /dev/null +++ b/src/ameba/ast/visitors/flow_expression_visitor.cr @@ -0,0 +1,33 @@ +require "./base_visitor" + +module Ameba::AST + # AST Visitor that traverses all the flow expressions. + class FlowExpressionVisitor < BaseVisitor + @node_stack = Array(Crystal::ASTNode).new + + def initialize(@rule, @source) + @source.ast.accept self + end + + def visit(node) + @node_stack.push node + end + + def end_visit(node) + @node_stack.pop + end + + def visit(node : Crystal::ControlExpression) + if parent_node = @node_stack.last? + flow_expression = FlowExpression.new(node, parent_node) + @rule.test @source, node, flow_expression + end + + true + end + + def end_visit(node : Crystal::ControlExpression) + # + end + end +end diff --git a/src/ameba/rule/lint/unreachable_code.cr b/src/ameba/rule/lint/unreachable_code.cr new file mode 100644 index 00000000..e069054d --- /dev/null +++ b/src/ameba/rule/lint/unreachable_code.cr @@ -0,0 +1,62 @@ +module Ameba::Rule::Lint + # A rule that reports unreachable code. + # + # For example, this is considered invalid: + # + # ``` + # def method(a) + # return 42 + # a + 1 + # end + # ``` + # + # ``` + # a = 1 + # loop do + # break + # a += 1 + # end + # ``` + # + # And has to be written as the following: + # + # ``` + # def method(a) + # return 42 if a == 0 + # a + 1 + # end + # ``` + # + # ``` + # a = 1 + # loop do + # break a > 3 + # a += 1 + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/UnreachableCode: + # Enabled: true + # ``` + # + struct UnreachableCode < Base + properties do + description "Reports unreachable code" + end + + MSG = "Unreachable code detected" + + def test(source) + AST::FlowExpressionVisitor.new self, source + end + + def test(source, node, flow_expression : AST::FlowExpression) + if unreachable_node = flow_expression.find_unreachable_node + issue_for unreachable_node, MSG + end + end + end +end