From 5491d31b5ffa94d8d29a58a32f2e8ec8a0047f9e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 13 Nov 2022 00:24:58 +0100 Subject: [PATCH 1/2] Fix the edge case re: free var comparison --- spec/ameba/rule/lint/literals_comparison_spec.cr | 13 +++++++++++++ src/ameba/rule/lint/literals_comparison.cr | 11 +++++++++++ 2 files changed, 24 insertions(+) diff --git a/spec/ameba/rule/lint/literals_comparison_spec.cr b/spec/ameba/rule/lint/literals_comparison_spec.cr index 5c039653..920a133b 100644 --- a/spec/ameba/rule/lint/literals_comparison_spec.cr +++ b/spec/ameba/rule/lint/literals_comparison_spec.cr @@ -41,6 +41,13 @@ module Ameba::Rule::Lint CRYSTAL end + pending "reports if there is a static path comparison evaluating to false" do + expect_issue subject, <<-CRYSTAL + String == Nil + # ^^^^^^^^^^^ error: Comparison always evaluates to false + CRYSTAL + end + context "macro" do it "reports in macro scope" do expect_issue subject, <<-CRYSTAL @@ -48,6 +55,12 @@ module Ameba::Rule::Lint # ^^^^^^^^^^^^^^ error: Comparison always evaluates to true CRYSTAL end + + it "passes for free variables comparisons in macro scope" do + expect_no_issues subject, <<-CRYSTAL + {{ T == Nil }} + CRYSTAL + end end it "reports rule, pos and message" do diff --git a/src/ameba/rule/lint/literals_comparison.cr b/src/ameba/rule/lint/literals_comparison.cr index 183da4eb..7bdbfe60 100644 --- a/src/ameba/rule/lint/literals_comparison.cr +++ b/src/ameba/rule/lint/literals_comparison.cr @@ -59,6 +59,17 @@ module Ameba::Rule::Lint return unless obj.class.in?(LITERAL_TYPES) && arg.class.in?(LITERAL_TYPES) + # Edge-case: `{{ T == Nil }}` + # + # Current implementation works for any single name path comparisons: + # `String == Nil`, regardless of the free variable being present. + # + # Ideally we should only check whether either of the sides + # is a free var + if obj.is_a?(Crystal::Path) && arg.is_a?(Crystal::Path) + return if obj.single_name? && arg.single_name? && (obj != arg) + end + is_dynamic = obj.class.in?(DYNAMIC_LITERAL_TYPES) || arg.class.in?(DYNAMIC_LITERAL_TYPES) From 84540d2a22397692f82d064563ecab5dabff990e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sun, 13 Nov 2022 01:25:24 +0100 Subject: [PATCH 2/2] Change to alternative approach skipping all macro contexts --- .../rule/lint/literals_comparison_spec.cr | 4 +-- src/ameba/rule/lint/literals_comparison.cr | 27 +++++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/spec/ameba/rule/lint/literals_comparison_spec.cr b/spec/ameba/rule/lint/literals_comparison_spec.cr index 920a133b..595197b8 100644 --- a/spec/ameba/rule/lint/literals_comparison_spec.cr +++ b/spec/ameba/rule/lint/literals_comparison_spec.cr @@ -41,7 +41,7 @@ module Ameba::Rule::Lint CRYSTAL end - pending "reports if there is a static path comparison evaluating to false" do + it "reports if there is a static path comparison evaluating to false" do expect_issue subject, <<-CRYSTAL String == Nil # ^^^^^^^^^^^ error: Comparison always evaluates to false @@ -49,7 +49,7 @@ module Ameba::Rule::Lint end context "macro" do - it "reports in macro scope" do + pending "reports in macro scope" do expect_issue subject, <<-CRYSTAL {{ "foo" == "foo" }} # ^^^^^^^^^^^^^^ error: Comparison always evaluates to true diff --git a/src/ameba/rule/lint/literals_comparison.cr b/src/ameba/rule/lint/literals_comparison.cr index 7bdbfe60..59ecc0a8 100644 --- a/src/ameba/rule/lint/literals_comparison.cr +++ b/src/ameba/rule/lint/literals_comparison.cr @@ -52,6 +52,22 @@ module Ameba::Rule::Lint LITERAL_TYPES = PRIMITIVE_LITERAL_TYPES + DYNAMIC_LITERAL_TYPES + # Edge-case: `{{ T == Nil }}` + # + # Current implementation just skips all macro contexts, + # regardless of the free variable being present. + # + # Ideally we should only check whether either of the sides + # is a free var + def test(source) + AST::NodeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroExpression, + Crystal::MacroIf, + Crystal::MacroFor, + ] + end + def test(source, node : Crystal::Call) return unless node.name.in?(OP_NAMES) return unless (obj = node.obj) && (arg = node.args.first?) @@ -59,17 +75,6 @@ module Ameba::Rule::Lint return unless obj.class.in?(LITERAL_TYPES) && arg.class.in?(LITERAL_TYPES) - # Edge-case: `{{ T == Nil }}` - # - # Current implementation works for any single name path comparisons: - # `String == Nil`, regardless of the free variable being present. - # - # Ideally we should only check whether either of the sides - # is a free var - if obj.is_a?(Crystal::Path) && arg.is_a?(Crystal::Path) - return if obj.single_name? && arg.single_name? && (obj != arg) - end - is_dynamic = obj.class.in?(DYNAMIC_LITERAL_TYPES) || arg.class.in?(DYNAMIC_LITERAL_TYPES)