From 2fb37da80f89055803a1c4e43f3127558068e4ac Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 15 Nov 2022 17:41:37 +0100 Subject: [PATCH] Refactor `AST::Util#literal?` helper --- .../rule/lint/literals_comparison_spec.cr | 6 +-- src/ameba/ast/util.cr | 47 +++++++++++++++---- src/ameba/rule/lint/literal_in_condition.cr | 2 +- src/ameba/rule/lint/literals_comparison.cr | 32 ++----------- 4 files changed, 48 insertions(+), 39 deletions(-) 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..39b3df88 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. 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