From 6d41beba57af282e9b8be4ac537580f870f6d5e0 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sat, 16 Jan 2021 19:57:57 +0100 Subject: [PATCH 1/5] Remove ill-working CI hack --- .github/workflows/ci.yml | 6 ------ 1 file changed, 6 deletions(-) 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 From 4958fa2315eedc78252e680790be0dda2a13a98a Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Sat, 16 Jan 2021 19:53:48 +0100 Subject: [PATCH 2/5] Merge pull request #184 from mamantoha/fix-crystal-nightly fix Crystal nightly --- src/ameba/reportable.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr index 35453950..e31ef3a7 100644 --- a/src/ameba/reportable.cr +++ b/src/ameba/reportable.cr @@ -5,7 +5,7 @@ module Ameba getter issues = [] of Issue # Adds a new issue to the list of issues. - def add_issue(rule, location, end_location, message, status = nil) + def add_issue(rule, location : Crystal::Location?, end_location : Crystal::Location?, message, status = nil) status ||= :disabled if location_disabled?(location, rule) issues << Issue.new rule, location, end_location, message, status end From 7aa7efd4bdf08bfefb1901c97fe6db13ea1aafdf Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 20 Jan 2021 10:02:36 +0200 Subject: [PATCH 3/5] Do not report if variable is assigned and referenced in MacroFor/MacroIf/MacroExpression closes #194 --- spec/ameba/rule/lint/useless_assign_spec.cr | 60 +++++++++++++++++++++ src/ameba/ast/variabling/variable.cr | 33 ++++++++---- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index bd5fdb2d..57d8a57e 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -944,6 +944,66 @@ 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 context "uninitialized" do diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 1a7eab42..a9d30ed0 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -120,12 +120,13 @@ module Ameba::AST 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 @@ -167,23 +168,35 @@ 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 - def references?(node : Crystal::Var) - @macro_literals.any? { |literal| literal.value.includes? node.name } - end - def visit(node : Crystal::ASTNode) true 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 From d28f9f756ea3f5c7ae2dc847341a062c3bda1ce5 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Tue, 26 Jan 2021 18:21:51 +0200 Subject: [PATCH 4/5] Bump v0.13.4 --- shard.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 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 5/5] 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?