From a841076cce449d67fae4dd9d4ef81045f00c3e5b Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Wed, 12 Feb 2025 18:00:59 +0100
Subject: [PATCH] =?UTF-8?q?Refactor=20`AST::ImplicitReturnVisitor`=20to=20?=
 =?UTF-8?q?call=20the=20underlying=20rule=20only=20in=20case=20it=E2=80=99?=
 =?UTF-8?q?s=20not=20being=20used?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../ast/visitors/implicit_return_visitor.cr   | 78 ++++++++++---------
 src/ameba/rule/lint/unused_comparison.cr      |  4 +-
 .../rule/lint/unused_generic_or_union.cr      | 10 +--
 src/ameba/rule/lint/unused_literal.cr         |  7 +-
 4 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr
index d822036d4..bb8d8a327 100644
--- a/src/ameba/ast/visitors/implicit_return_visitor.cr
+++ b/src/ameba/ast/visitors/implicit_return_visitor.cr
@@ -16,7 +16,7 @@ module Ameba::AST
     # their parents scope. Only the last line in an expressions node can be
     # captured by their parent node.
     def visit(node : Crystal::Expressions) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       last_idx = node.expressions.size - 1
 
@@ -37,7 +37,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::BinaryOp) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       if node.right.is_a?(Crystal::Call)
         incr_stack { node.left.accept(self) }
@@ -51,7 +51,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Call) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.obj.try &.accept(self)
@@ -65,7 +65,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Arg) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.default_value.try &.accept(self) }
 
@@ -73,7 +73,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::EnumDef) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.members.each &.accept(self)
 
@@ -81,7 +81,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Assign | Crystal::OpAssign) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.value.accept(self) }
 
@@ -89,7 +89,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::MultiAssign) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.targets.each &.accept(self)
       incr_stack { node.values.each &.accept(self) }
@@ -98,7 +98,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::If | Crystal::Unless) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.cond.accept(self) }
       node.then.accept(self)
@@ -108,7 +108,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::While | Crystal::Until) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.cond.accept(self) }
       node.body.accept(self)
@@ -117,7 +117,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Def) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.args.each &.accept(self)
@@ -141,7 +141,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Macro) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.args.each &.accept(self)
@@ -154,7 +154,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::ClassDef | Crystal::ModuleDef) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.body.accept(self)
 
@@ -162,7 +162,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::FunDef) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.args.each &.accept(self)
@@ -173,7 +173,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Cast | Crystal::NilableCast) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.obj.accept(self) }
 
@@ -181,7 +181,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Annotation) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.args.each &.accept(self)
@@ -192,7 +192,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::TypeDeclaration) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.value.try &.accept(self) }
 
@@ -200,7 +200,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::ArrayLiteral | Crystal::TupleLiteral) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.elements.each &.accept(self) }
 
@@ -208,7 +208,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::StringInterpolation) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.expressions.each do |exp|
         incr_stack { exp.accept(self) }
@@ -218,7 +218,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::HashLiteral | Crystal::NamedTupleLiteral) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.entries.each &.value.accept(self) }
 
@@ -226,7 +226,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Case) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.cond.try &.accept(self) }
       node.whens.each &.accept(self)
@@ -236,7 +236,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Select) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.whens.each &.accept(self)
       node.else.try &.accept(self)
@@ -245,7 +245,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::When) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.conds.each &.accept(self) }
       node.body.accept(self)
@@ -254,7 +254,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Rescue) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.body.accept(self)
 
@@ -262,7 +262,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::ExceptionHandler) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       if node.else
         # Last line of body isn't implicitly returned if there's an else
@@ -281,7 +281,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::ControlExpression) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.exp.try &.accept(self) }
 
@@ -289,7 +289,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::RangeLiteral) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack do
         node.from.accept(self)
@@ -300,7 +300,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::RegexLiteral) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       # Regex literals either contain string literals or string interpolations,
       # both of which are "captured" by the parent regex literal
@@ -313,13 +313,13 @@ module Ameba::AST
       node : Crystal::BoolLiteral | Crystal::CharLiteral | Crystal::NumberLiteral |
              Crystal::StringLiteral | Crystal::SymbolLiteral | Crystal::ProcLiteral,
     ) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       true
     end
 
     def visit(node : Crystal::Yield) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.exps.each &.accept(self) }
 
@@ -327,7 +327,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::MacroExpression) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       if node.output?
         incr_stack { node.exp.accept(self) }
@@ -339,7 +339,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::MacroIf) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       incr_stack { node.cond.accept(self) }
       node.then.accept(self)
@@ -349,7 +349,7 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::MacroFor) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       node.body.accept(self)
 
@@ -361,25 +361,25 @@ module Ameba::AST
     end
 
     def visit(node : Crystal::Generic | Crystal::Path | Crystal::Union) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       false
     end
 
     def visit(node : Crystal::UninitializedVar) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       false
     end
 
     def visit(node : Crystal::LibDef) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       false
     end
 
     def visit(node : Crystal::Include | Crystal::Extend) : Bool
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       false
     end
@@ -393,11 +393,15 @@ module Ameba::AST
     end
 
     def visit(node)
-      @rule.test(@source, node, @stack.positive?)
+      report_implicit_return(node)
 
       true
     end
 
+    private def report_implicit_return(node) : Nil
+      @rule.test(@source, node) unless @stack.positive?
+    end
+
     # Indicates that any nodes visited within the block are captured / used.
     private def incr_stack(&) : Nil
       @stack += 1
diff --git a/src/ameba/rule/lint/unused_comparison.cr b/src/ameba/rule/lint/unused_comparison.cr
index 152948bbd..afbd93c02 100644
--- a/src/ameba/rule/lint/unused_comparison.cr
+++ b/src/ameba/rule/lint/unused_comparison.cr
@@ -48,8 +48,8 @@ module Ameba::Rule::Lint
       AST::ImplicitReturnVisitor.new(self, source)
     end
 
-    def test(source, node : Crystal::Call, node_is_used : Bool)
-      if !node_is_used && node.name.in?(COMPARISON_OPERATORS) && node.args.size == 1
+    def test(source, node : Crystal::Call)
+      if node.name.in?(COMPARISON_OPERATORS) && node.args.size == 1
         issue_for node, MSG
       end
     end
diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr
index a34ceaed9..1f21d4672 100644
--- a/src/ameba/rule/lint/unused_generic_or_union.cr
+++ b/src/ameba/rule/lint/unused_generic_or_union.cr
@@ -47,14 +47,12 @@ module Ameba::Rule::Lint
       AST::ImplicitReturnVisitor.new(self, source)
     end
 
-    def test(source, node : Crystal::Call, node_is_used : Bool)
-      return if node_is_used || !path_or_generic_union?(node)
-
-      issue_for node, MSG_UNION
+    def test(source, node : Crystal::Call)
+      issue_for node, MSG_UNION if path_or_generic_union?(node)
     end
 
-    def test(source, node : Crystal::Generic, node_is_used : Bool)
-      issue_for node, MSG_GENERIC unless node_is_used
+    def test(source, node : Crystal::Generic)
+      issue_for node, MSG_GENERIC
     end
 
     private def path_or_generic_union?(node : Crystal::Call) : Bool
diff --git a/src/ameba/rule/lint/unused_literal.cr b/src/ameba/rule/lint/unused_literal.cr
index 359b4e508..a6aa58f49 100644
--- a/src/ameba/rule/lint/unused_literal.cr
+++ b/src/ameba/rule/lint/unused_literal.cr
@@ -55,10 +55,10 @@ module Ameba::Rule::Lint
       AST::ImplicitReturnVisitor.new(self, source)
     end
 
-    def test(source, node : Crystal::RegexLiteral, node_is_used : Bool)
+    def test(source, node : Crystal::RegexLiteral)
       # Locations for Regex literals were added in Crystal v1.15.0
       {% if compare_versions(Crystal::VERSION, "1.15.0") >= 0 %}
-        issue_for node, MSG unless node_is_used
+        issue_for node, MSG
       {% end %}
     end
 
@@ -69,9 +69,8 @@ module Ameba::Rule::Lint
              Crystal::TupleLiteral | Crystal::NumberLiteral |
              Crystal::StringLiteral | Crystal::SymbolLiteral |
              Crystal::NamedTupleLiteral | Crystal::StringInterpolation,
-      node_is_used : Bool,
     )
-      issue_for node, MSG unless node_is_used
+      issue_for node, MSG
     end
   end
 end