Merge pull request #205 from crystal-ameba/master-into-develop

Merge master into develop

.github/workflows/ci.yml

@@ -3,12 +3,6 @@ name: CI
 on:
   push:
   pull_request:
-    branches:
-      # Branches from forks have the form 'user:branch-name' so we only run
-      # this job on pull_request events for branches that look like fork
-      # branches. Without this we would end up running this job twice for non
-      # forked PRs, once for the push and then once for opening the PR.
-      - "**:**"
   schedule:
     - cron: "0 3 * * 1" # Every monday at 3 AM
 

shard.yml

@@ -1,5 +1,5 @@
 name: ameba
-version: 0.13.3
+version: 0.13.4
 
 authors:
   - Vitalii Elenhaupt <velenhaupt@gmail.com>

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("")

spec/ameba/rule/lint/useless_assign_spec.cr

@@ -944,6 +944,108 @@ module Ameba::Rule::Lint
         )
         subject.catch(s).should be_valid
       end
+
+      it "doesn't report if assignment is referenced in a macro expression as string" do
+        s = Source.new %(
+          foo = 1
+          puts {{ "foo".id }}
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if assignement is referenced in for macro in exp" do
+        s = Source.new %(
+          foo = 22
+
+          {% for x in %w(foo) %}
+            add({{x.id}})
+          {% end %}
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if assignement is referenced in for macro in body" do
+        s = Source.new %(
+          foo = 22
+
+          {% for x in %w(bar) %}
+            puts {{ "foo".id }}
+          {% end %}
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if assignement is referenced in if macro in cond" do
+        s = Source.new %(
+          foo = 22
+          {% if "foo".id %}
+          {% end %}
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if assignement is referenced in if macro in then" do
+        s = Source.new %(
+          foo = 22
+          {% if true %}
+             puts {{ "foo".id }}
+          {% end %}
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if assignement is referenced in if macro in else" do
+        s = Source.new %(
+          foo = 22
+          {% if true %}
+          {% else %}
+             puts {{ "foo".id }}
+          {% end %}
+        )
+        subject.catch(s).should be_valid
+      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

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
 

src/ameba/ast/variabling/variable.cr

@@ -110,19 +110,20 @@ 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
 
       false
     end
 
-    # Returns true if current variable potentially referenced in a macro literal,
+    # Returns true if current variable potentially referenced in a macro,
     # false if not.
     def used_in_macro?(scope = @scope)
       scope.inner_scopes.each do |inner_scope|
-        return true if MacroLiteralFinder.new(inner_scope.node).references? node
+        return true if MacroReferenceFinder.new(inner_scope.node, node.name).references
       end
+      return true if MacroReferenceFinder.new(scope.node, node.name).references
       return true if (outer_scope = scope.outer_scope) && used_in_macro?(outer_scope)
       false
     end
@@ -164,10 +165,10 @@ module Ameba::AST
           var_location.column_number < node_location.column_number)
     end
 
-    private class MacroLiteralFinder < Crystal::Visitor
-      @macro_literals = [] of Crystal::MacroLiteral
+    private class MacroReferenceFinder < Crystal::Visitor
+      property references = false
 
-      def initialize(node)
+      def initialize(node, @reference : String = reference)
         node.accept self
       end
 
@@ -180,7 +181,23 @@ module Ameba::AST
       end
 
       def visit(node : Crystal::MacroLiteral)
-        @macro_literals << node
+        !(self.references ||= node.value.includes?(@reference))
+      end
+
+      def visit(node : Crystal::MacroExpression)
+        !(self.references ||= node.exp.to_s.includes?(@reference))
+      end
+
+      def visit(node : Crystal::MacroFor)
+        exp, body = node.exp, node.body
+        !(self.references ||= exp.to_s.includes?(@reference) ||
+                              body.to_s.includes?(@reference))
+      end
+
+      def visit(node : Crystal::MacroIf)
+        !(self.references ||= node.cond.to_s.includes?(@reference) ||
+                              node.then.to_s.includes?(@reference) ||
+                              node.else.to_s.includes?(@reference))
       end
     end
 

src/ameba/rule/lint/useless_assign.cr

@@ -38,7 +38,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?