From 415432713a49c5dc578a5e01a77de83ef63b7a11 Mon Sep 17 00:00:00 2001
From: Vitalii Elenhaupt <velenhaupt@gmail.com>
Date: Sat, 12 May 2018 17:37:54 +0300
Subject: [PATCH] Prevent false positiveness cause by macro literals

https://github.com/crystal-lang/crystal/pull/6055#issuecomment-386376227
---
 spec/ameba/rule/useless_assign_spec.cr  | 31 +++++++++++++++++++++++++
 src/ameba/ast/scope.cr                  |  3 +++
 src/ameba/ast/variabling/variable.cr    | 12 +++++++++-
 src/ameba/ast/visitors/scope_visitor.cr | 24 +------------------
 src/ameba/rule/useless_assign.cr        |  2 +-
 5 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/spec/ameba/rule/useless_assign_spec.cr b/spec/ameba/rule/useless_assign_spec.cr
index 2f85c7df..0d73443b 100644
--- a/spec/ameba/rule/useless_assign_spec.cr
+++ b/spec/ameba/rule/useless_assign_spec.cr
@@ -820,6 +820,37 @@ module Ameba::Rule
         )
         subject.catch(s).should be_valid
       end
+
+      it "doesn't report if assignment is referenced in macro def" do
+        s = Source.new %(
+          macro macro_call
+            puts x
+          end
+
+          def foo
+            x = 1
+            macro_call
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if assignment is referenced in macro def in a different scope" do
+        s = Source.new %(
+          class Foo
+            def foo
+              x = 1
+            end
+          end
+
+          class Bar
+            macro macro_call
+              puts x
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+      end
     end
   end
 end
diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr
index 1a287385..b2d7ecb1 100644
--- a/src/ameba/ast/scope.cr
+++ b/src/ameba/ast/scope.cr
@@ -19,6 +19,9 @@ module Ameba::AST
     # The actual AST node that represents a current scope.
     getter node : Crystal::ASTNode
 
+    # Macro literals in current scope
+    getter macro_literals = [] of Crystal::MacroLiteral
+
     delegate to_s, to: node
     delegate location, to: node
 
diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr
index 1a0d1349..cecb44e8 100644
--- a/src/ameba/ast/variabling/variable.cr
+++ b/src/ameba/ast/variabling/variable.cr
@@ -89,7 +89,7 @@ module Ameba::AST
       end
     end
 
-    # Returns true if the current assignment is referenced in
+    # Returns true if the current var is referenced in
     # in the block. For example this variable is captured
     # by block:
     #
@@ -113,6 +113,16 @@ module Ameba::AST
       false
     end
 
+    # Returns true if current variable potentially referenced in a macro literal,
+    # false if not.
+    def used_in_macro?(scope = @scope)
+      scope.inner_scopes.each do |inner_scope|
+        return true if inner_scope.macro_literals.any? { |literal| literal.value.includes?(name) }
+      end
+      return true if (outer_scope = scope.outer_scope) && used_in_macro?(outer_scope)
+      false
+    end
+
     # Returns true if the variable is a target (on the left) of the assignment,
     # false otherwise.
     def target_of?(assign)
diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr
index ac9c8d34..af86cee6 100644
--- a/src/ameba/ast/visitors/scope_visitor.cr
+++ b/src/ameba/ast/visitors/scope_visitor.cr
@@ -120,7 +120,7 @@ module Ameba::AST
 
     # :nodoc:
     def visit(node : Crystal::MacroLiteral)
-      MacroLiteralVarVisitor.new(node).vars.each { |var| visit(var) }
+      @current_scope.macro_literals << node
     end
 
     # :nodoc:
@@ -134,26 +134,4 @@ module Ameba::AST
       true
     end
   end
-
-  private class MacroLiteralVarVisitor < Crystal::Visitor
-    getter vars = [] of Crystal::Var
-
-    def initialize(literal)
-      Crystal::Parser.new(literal.value).parse.accept self
-    rescue
-      nil
-    end
-
-    def visit(node : Crystal::ASTNode)
-      true
-    end
-
-    def visit(node : Crystal::Var)
-      vars << node
-    end
-
-    def visit(node : Crystal::Call)
-      vars << Crystal::Var.new(node.name).at(node.location)
-    end
-  end
 end
diff --git a/src/ameba/rule/useless_assign.cr b/src/ameba/rule/useless_assign.cr
index b01eaf60..1e5a4d50 100644
--- a/src/ameba/rule/useless_assign.cr
+++ b/src/ameba/rule/useless_assign.cr
@@ -39,7 +39,7 @@ module Ameba::Rule
 
     def test(source, node, scope : AST::Scope)
       scope.variables.each do |var|
-        next if var.captured_by_block?
+        next if var.captured_by_block? || var.used_in_macro?
 
         var.assignments.each do |assign|
           next if assign.referenced?