diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 544e4f20..0298a508 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/shard.yml b/shard.yml index 68a3ae25..e40c86dc 100644 --- a/shard.yml +++ b/shard.yml @@ -1,5 +1,5 @@ name: ameba -version: 0.13.3 +version: 0.13.4 authors: - Vitalii Elenhaupt diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr index 00f3061c..2e97ab6c 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 bd5fdb2d..91c40076 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/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 diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index a6a8ee08..68bed156 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 b3dab305..bb45877b 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/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 diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index 5b06763c..e09df45e 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/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?