From 6f5d7f0478599523ac8dd9b8f59d8b37d82345c5 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 1 Nov 2017 12:22:37 +0200 Subject: [PATCH] New rule: a literal in the condition --- spec/ameba/rules/literal_in_condition_spec.cr | 72 +++++++++++++++++++ src/ameba/ast_traverse.cr | 4 +- src/ameba/rules/literal_in_condition.cr | 44 ++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 spec/ameba/rules/literal_in_condition_spec.cr create mode 100644 src/ameba/rules/literal_in_condition.cr diff --git a/spec/ameba/rules/literal_in_condition_spec.cr b/spec/ameba/rules/literal_in_condition_spec.cr new file mode 100644 index 00000000..a8fb0e09 --- /dev/null +++ b/spec/ameba/rules/literal_in_condition_spec.cr @@ -0,0 +1,72 @@ +require "../../spec_helper" + +module Ameba::Rules + subject = LiteralInCondition.new + + describe LiteralInCondition do + it "passes if there is not literals in conditional" do + s = Source.new %( + if a == 2 + :ok + end + + :ok unless b + + case string + when "a" + :ok + when "b" + :ok + end + + unless a.nil? + :ok + end + ) + subject.catch(s).valid?.should be_true + end + + it "fails if there is a predicate in if conditional" do + s = Source.new %( + if "string" + :ok + end + ) + subject.catch(s).valid?.should be_false + end + + it "fails if there is a predicate in unless conditional" do + s = Source.new %( + unless true + :ok + end + ) + subject.catch(s).valid?.should be_false + end + + it "fails if there is a predicate in case conditional" do + s = Source.new %( + case [1, 2, 3] + when :array + :ok + when :not_array + :also_ok + end + ) + subject.catch(s).valid?.should be_false + end + + it "reports rule, pos and message" do + s = Source.new %( + puts "hello" if true + ) + subject.catch(s) + + s.errors.size.should eq 1 + error = s.errors.first + error.rule.should_not be_nil + error.pos.should eq 2 + error.message.should eq "Literal value found in conditional" + end + end +end diff --git a/src/ameba/ast_traverse.cr b/src/ameba/ast_traverse.cr index 5397a610..b4a06c74 100644 --- a/src/ameba/ast_traverse.cr +++ b/src/ameba/ast_traverse.cr @@ -2,8 +2,10 @@ require "compiler/crystal/syntax/*" module Ameba NODE_VISITORS = [ - Unless, Call, + Case, + If, + Unless, ] {% for name in NODE_VISITORS %} diff --git a/src/ameba/rules/literal_in_condition.cr b/src/ameba/rules/literal_in_condition.cr new file mode 100644 index 00000000..7dfae7e7 --- /dev/null +++ b/src/ameba/rules/literal_in_condition.cr @@ -0,0 +1,44 @@ +module Ameba::Rules + # A rule that disallows useless conditional statements that contain a literal + # in place of a variable or predicate function. + # + # This is because a conditional construct with a literal predicate will + # always result in the same behaviour at run time, meaning it can be + # replaced with either the body of the construct, or deleted entirely. + # + # This is considered invalid: + # ``` + # if "something" + # :ok + # end + # ``` + struct LiteralInCondition < Rule + def test(source) + IfVisitor.new self, source + UnlessVisitor.new self, source + CaseVisitor.new self, source + end + + def check_node(source, node) + return unless literal?(node.cond) + source.error self, node.location.try &.line_number, + "Literal value found in conditional" + end + + def test(source, node : Crystal::If) + check_node source, node + end + + def test(source, node : Crystal::Unless) + check_node source, node + end + + def test(source, node : Crystal::Case) + check_node source, node + end + + private def literal?(node) + node.try &.class.name.ends_with? "Literal" + end + end +end