diff --git a/spec/ameba/rule/lint/literals_comparison_spec.cr b/spec/ameba/rule/lint/literals_comparison_spec.cr index 595197b8..43bd5c15 100644 --- a/spec/ameba/rule/lint/literals_comparison_spec.cr +++ b/spec/ameba/rule/lint/literals_comparison_spec.cr @@ -13,10 +13,10 @@ module Ameba::Rule::Lint CRYSTAL end - it "reports if there is a regex comparison possibly evaluating to the same" do + it "reports if there is a dynamic comparison possibly evaluating to the same" do expect_issue subject, <<-CRYSTAL - /foo/ === "foo" - # ^^^^^^^^^^^^^ error: Comparison most likely evaluates to the same + [foo] === ["foo"] + # ^^^^^^^^^^^^^^^ error: Comparison most likely evaluates to the same CRYSTAL end diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 07eff507..e69ee5f8 100644 --- a/src/ameba/ast/util.cr +++ b/src/ameba/ast/util.cr @@ -1,7 +1,10 @@ # Utility module for Ameba's rules. module Ameba::AST::Util - # Returns true if current `node` is a literal, false otherwise. - def literal?(node) + # Returns tuple with two bool flags: + # + # 1. is *node* a literal? + # 2. can *node* be proven static? + protected def literal_kind?(node, include_paths = false) : {Bool, Bool} case node when Crystal::NilLiteral, Crystal::BoolLiteral, @@ -12,21 +15,49 @@ module Ameba::AST::Util Crystal::RegexLiteral, Crystal::ProcLiteral, Crystal::MacroLiteral - true + {true, true} when Crystal::RangeLiteral - literal?(node.from) && literal?(node.to) + {true, static_literal?(node.from, include_paths) && + static_literal?(node.to, include_paths)} when Crystal::ArrayLiteral, Crystal::TupleLiteral - node.elements.all? { |el| literal?(el) } + {true, node.elements.all? do |el| + static_literal?(el, include_paths) + end} when Crystal::HashLiteral - node.entries.all? { |entry| literal?(entry.key) && literal?(entry.value) } + {true, node.entries.all? do |entry| + static_literal?(entry.key, include_paths) && + static_literal?(entry.value, include_paths) + end} when Crystal::NamedTupleLiteral - node.entries.all? { |entry| literal?(entry.value) } + {true, node.entries.all? do |entry| + static_literal?(entry.value, include_paths) + end} + when Crystal::Path + {include_paths, true} else - false + {false, false} end end + # Returns `true` if current `node` is a static literal, `false` otherwise. + def static_literal?(node, include_paths = false) : Bool + is_literal, is_static = literal_kind?(node, include_paths) + is_literal && is_static + end + + # Returns `true` if current `node` is a dynamic literal, `false` otherwise. + def dynamic_literal?(node, include_paths = false) : Bool + is_literal, is_static = literal_kind?(node, include_paths) + is_literal && !is_static + end + + # Returns `true` if current `node` is a literal, `false` otherwise. + def literal?(node, include_paths = false) : Bool + is_literal, _ = literal_kind?(node, include_paths) + is_literal + end + # Returns a source code for the current node. # This method uses `node.location` and `node.end_location` # to determine and cut a piece of source of the node. @@ -60,7 +91,7 @@ module Ameba::AST::Util node_lines.join('\n') end - # Returns true if node is a flow command, false - otherwise. + # Returns `true` if node is a flow command, `false` otherwise. # Node represents a flow command if it is a control expression, # or special call node that interrupts execution (i.e. raise, exit, abort). def flow_command?(node, in_loop) @@ -76,7 +107,7 @@ module Ameba::AST::Util end end - # Returns true if node is a flow expression, false if not. + # Returns `true` if node is a flow expression, `false` if not. # Node represents a flow expression if it is full-filled by a flow command. # # For example, this node is a flow expression, because each branch contains @@ -128,25 +159,25 @@ module Ameba::AST::Util nodes.all? { |exp| flow_expression? exp, in_loop } end - # Returns true if node represents `raise` method call. + # Returns `true` if node represents `raise` method call. def raise?(node) node.is_a?(Crystal::Call) && node.name == "raise" && node.args.size == 1 && node.obj.nil? end - # Returns true if node represents `exit` method call. + # Returns `true` if node represents `exit` method call. def exit?(node) node.is_a?(Crystal::Call) && node.name == "exit" && node.args.size <= 1 && node.obj.nil? end - # Returns true if node represents `abort` method call. + # Returns `true` if node represents `abort` method call. def abort?(node) node.is_a?(Crystal::Call) && node.name == "abort" && node.args.size <= 2 && node.obj.nil? end - # Returns true if node represents a loop. + # Returns `true` if node represents a loop. def loop?(node) case node when Crystal::While, Crystal::Until diff --git a/src/ameba/rule/lint/literal_in_condition.cr b/src/ameba/rule/lint/literal_in_condition.cr index fc3ce23a..d232ca83 100644 --- a/src/ameba/rule/lint/literal_in_condition.cr +++ b/src/ameba/rule/lint/literal_in_condition.cr @@ -30,7 +30,7 @@ module Ameba::Rule::Lint MSG = "Literal value found in conditional" def check_node(source, node) - issue_for node, MSG if literal?(node.cond) + issue_for node, MSG if static_literal?(node.cond) end def test(source, node : Crystal::If) diff --git a/src/ameba/rule/lint/literals_comparison.cr b/src/ameba/rule/lint/literals_comparison.cr index 59ecc0a8..ee40d075 100644 --- a/src/ameba/rule/lint/literals_comparison.cr +++ b/src/ameba/rule/lint/literals_comparison.cr @@ -29,29 +29,6 @@ module Ameba::Rule::Lint MSG = "Comparison always evaluates to %s" MSG_LIKELY = "Comparison most likely evaluates to %s" - PRIMITIVE_LITERAL_TYPES = { - Crystal::NilLiteral, - Crystal::BoolLiteral, - Crystal::NumberLiteral, - Crystal::CharLiteral, - Crystal::StringLiteral, - Crystal::SymbolLiteral, - Crystal::ProcLiteral, - Crystal::Path, - } - - DYNAMIC_LITERAL_TYPES = { - Crystal::RangeLiteral, - Crystal::RegexLiteral, - Crystal::TupleLiteral, - Crystal::NamedTupleLiteral, - Crystal::ArrayLiteral, - Crystal::HashLiteral, - } - - LITERAL_TYPES = - PRIMITIVE_LITERAL_TYPES + DYNAMIC_LITERAL_TYPES - # Edge-case: `{{ T == Nil }}` # # Current implementation just skips all macro contexts, @@ -72,11 +49,12 @@ module Ameba::Rule::Lint return unless node.name.in?(OP_NAMES) return unless (obj = node.obj) && (arg = node.args.first?) - return unless obj.class.in?(LITERAL_TYPES) && - arg.class.in?(LITERAL_TYPES) + obj_is_literal, obj_is_static = literal_kind?(obj, include_paths: true) + arg_is_literal, arg_is_static = literal_kind?(arg, include_paths: true) - is_dynamic = obj.class.in?(DYNAMIC_LITERAL_TYPES) || - arg.class.in?(DYNAMIC_LITERAL_TYPES) + return unless obj_is_literal && arg_is_literal + + is_dynamic = !obj_is_static || !arg_is_static what = case node.name