From 51b0a07e810501895a71a8f8183ccf752d10d083 Mon Sep 17 00:00:00 2001
From: Vitalii Elenhaupt <3624712+veelenga@users.noreply.github.com>
Date: Sun, 31 Jan 2021 16:40:44 +0200
Subject: [PATCH] Avoid exponential recursion while finding variable references
 in scopes (#203)

* Avoid exponential recursion while finding variable references in scopes

* Adjust source example in test
---
 spec/ameba/ast/scope_spec.cr                | 62 +++++++++++++++++++++
 spec/ameba/rule/lint/useless_assign_spec.cr | 42 ++++++++++++++
 src/ameba/ast/scope.cr                      |  6 +-
 src/ameba/ast/variabling/variable.cr        |  2 +-
 src/ameba/rule/lint/useless_assign.cr       |  2 +-
 5 files changed, 109 insertions(+), 5 deletions(-)

diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr
index 72951b14..5a629619 100644
--- a/spec/ameba/ast/scope_spec.cr
+++ b/spec/ameba/ast/scope_spec.cr
@@ -47,6 +47,68 @@ module Ameba::AST
     end
   end
 
+  describe "#references?" do
+    it "returns true if current scope references variable" do
+      nodes = as_nodes %(
+        def method
+          a = 2
+          block do
+            3.times { |i| a = a + i }
+          end
+        end
+      )
+      scope = Scope.new nodes.def_nodes.first
+      var_node = nodes.var_nodes.first
+      scope.add_variable var_node
+      scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
+
+      variable = Variable.new(var_node, scope)
+      variable.reference nodes.var_nodes.first, scope.inner_scopes.first
+
+      scope.references?(variable).should be_true
+    end
+
+    it "returns false if inner scopes are not checked" do
+      nodes = as_nodes %(
+        def method
+          a = 2
+          block do
+            3.times { |i| a = a + i }
+          end
+        end
+      )
+      scope = Scope.new nodes.def_nodes.first
+      var_node = nodes.var_nodes.first
+      scope.add_variable var_node
+      scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
+
+      variable = Variable.new(var_node, scope)
+      variable.reference nodes.var_nodes.first, scope.inner_scopes.first
+
+      scope.references?(variable, check_inner_scopes: false).should be_false
+    end
+
+    it "returns false if current scope does not reference variable" do
+      nodes = as_nodes %(
+        def method
+          a = 2
+          block do
+            b = 3
+            3.times { |i| b = b + i }
+          end
+        end
+      )
+      scope = Scope.new nodes.def_nodes.first
+      var_node = nodes.var_nodes.first
+      scope.add_variable var_node
+      scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope)
+
+      variable = Variable.new(var_node, scope)
+
+      scope.inner_scopes.first.references?(variable).should be_false
+    end
+  end
+
   describe "#add_variable" do
     it "adds a new variable to the scope" do
       scope = Scope.new as_node("")
diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr
index 57d8a57e..91c40076 100644
--- a/spec/ameba/rule/lint/useless_assign_spec.cr
+++ b/spec/ameba/rule/lint/useless_assign_spec.cr
@@ -1006,6 +1006,48 @@ module Ameba::Rule::Lint
       end
     end
 
+    it "does not report if variable is referenced and there is a deep level scope" do
+      s = Source.new %(
+        response = JSON.build do |json|
+          json.object do
+            json.object do
+              json.object do
+                json.object do
+                  json.object do
+                    json.object do
+                      json.object do
+                        json.object do
+                          json.object do
+                            json.object do
+                              json.object do
+                                json.object do
+                                  json.object do
+                                    json.object do
+                                      json.object do
+                                        anything
+                                      end
+                                    end
+                                  end
+                                end
+                              end
+                            end
+                          end
+                        end
+                      end
+                    end
+                  end
+                end
+              end
+            end
+          end
+        end
+
+        response = JSON.parse(response)
+        response
+       )
+      subject.catch(s).should be_valid
+    end
+
     context "uninitialized" do
       it "reports if uninitialized assignment is not referenced at a top level" do
         s = Source.new %(
diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr
index 3627ef73..e1922b6e 100644
--- a/src/ameba/ast/scope.cr
+++ b/src/ameba/ast/scope.cr
@@ -134,10 +134,10 @@ module Ameba::AST
 
     # Returns true if current scope (or any of inner scopes) references variable,
     # false if not.
-    def references?(variable : Variable)
+    def references?(variable : Variable, check_inner_scopes = true)
       variable.references.any? do |reference|
-        reference.scope == self ||
-          inner_scopes.any?(&.references? variable)
+        return true if reference.scope == self
+        check_inner_scopes && inner_scopes.any?(&.references?(variable))
       end || variable.used_in_macro?
     end
 
diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr
index a9d30ed0..59648c3d 100644
--- a/src/ameba/ast/variabling/variable.cr
+++ b/src/ameba/ast/variabling/variable.cr
@@ -113,7 +113,7 @@ module Ameba::AST
     # ```
     def captured_by_block?(scope = @scope)
       scope.inner_scopes.each do |inner_scope|
-        return true if inner_scope.block? && inner_scope.references?(self)
+        return true if inner_scope.block? && inner_scope.references?(self, check_inner_scopes: false)
         return true if captured_by_block?(inner_scope)
       end
 
diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr
index bff18188..6616b6ab 100644
--- a/src/ameba/rule/lint/useless_assign.cr
+++ b/src/ameba/rule/lint/useless_assign.cr
@@ -39,7 +39,7 @@ module Ameba::Rule::Lint
 
     def test(source, node, scope : AST::Scope)
       scope.variables.each do |var|
-        next if var.captured_by_block? || var.used_in_macro? || var.ignored?
+        next if var.ignored? || var.used_in_macro? || var.captured_by_block?
 
         var.assignments.each do |assign|
           next if assign.referenced? || assign.transformed?