From 353d09df2915e0a3c81fcd2141c81268279f08a2 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 00:42:22 +0200 Subject: [PATCH 1/8] Rename file according to the rule name --- ..._local_outer_var_spec.cr => shadowing_outer_local_var_spec.cr} | 0 ...{shadowing_local_outer_var.cr => shadowing_outer_local_var.cr} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename spec/ameba/rule/lint/{shadowing_local_outer_var_spec.cr => shadowing_outer_local_var_spec.cr} (100%) rename src/ameba/rule/lint/{shadowing_local_outer_var.cr => shadowing_outer_local_var.cr} (100%) diff --git a/spec/ameba/rule/lint/shadowing_local_outer_var_spec.cr b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr similarity index 100% rename from spec/ameba/rule/lint/shadowing_local_outer_var_spec.cr rename to spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr diff --git a/src/ameba/rule/lint/shadowing_local_outer_var.cr b/src/ameba/rule/lint/shadowing_outer_local_var.cr similarity index 100% rename from src/ameba/rule/lint/shadowing_local_outer_var.cr rename to src/ameba/rule/lint/shadowing_outer_local_var.cr From ad5d56b313d316d6ba6f12e2bd94ea43dd520f37 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 01:30:27 +0200 Subject: [PATCH 2/8] Use issue expectation helpers in `ShadowingOuterLocalVar` spec --- .../lint/shadowing_outer_local_var_spec.cr | 104 ++++++++---------- 1 file changed, 44 insertions(+), 60 deletions(-) diff --git a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr index 4b1e5799..25bea9b0 100644 --- a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr +++ b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr @@ -5,7 +5,7 @@ module Ameba::Rule::Lint subject = ShadowingOuterLocalVar.new it "doesn't report if there is no shadowing" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL def some_method foo = 1 @@ -14,126 +14,117 @@ module Ameba::Rule::Lint end -> (baz : Int32) {} - -> (bar : String) {} end - ) - - subject.catch(source).should be_valid + CRYSTAL end it "reports if there is a shadowing in a block" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL def some_method foo = 1 3.times do |foo| + # ^ error: Shadowing outer local variable `foo` end end - ) - subject.catch(source).should_not be_valid + CRYSTAL end it "does not report outer vars declared below shadowed block" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL methods = klass.methods.select { |m| m.annotation(MyAnn) } m = methods.last - ) - subject.catch(source).should be_valid + CRYSTAL end it "reports if there is a shadowing in a proc" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL def some_method foo = 1 -> (foo : Int32) {} + # ^ error: Shadowing outer local variable `foo` end - ) - subject.catch(source).should_not be_valid + CRYSTAL end it "reports if there is a shadowing in an inner scope" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL def foo foo = 1 3.times do |i| 3.times { |foo| foo } + # ^ error: Shadowing outer local variable `foo` end end - ) - subject.catch(source).should_not be_valid + CRYSTAL end it "reports if variable is shadowed twice" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL foo = 1 3.times do |foo| + # ^ error: Shadowing outer local variable `foo` -> (foo : Int32) { foo + 1 } + # ^ error: Shadowing outer local variable `foo` end - ) - subject.catch(source).should_not be_valid - - source.issues.size.should eq 2 + CRYSTAL end it "reports if a splat block argument shadows local var" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL foo = 1 3.times do |*foo| + # ^ error: Shadowing outer local variable `foo` end - ) - subject.catch(source).should_not be_valid + CRYSTAL end it "reports if a &block argument is shadowed" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL def method_with_block(a, &block) 3.times do |block| + # ^ error: Shadowing outer local variable `block` end end - ) - subject.catch(source).should_not be_valid - source.issues.first.message.should eq "Shadowing outer local variable `block`" + CRYSTAL end it "reports if there are multiple args and one shadows local var" do - source = Source.new %( + expect_issue subject, <<-CRYSTAL foo = 1 [1, 2, 3].each_with_index do |i, foo| + # ^ error: Shadowing outer local variable `foo` i + foo end - ) - subject.catch(source).should_not be_valid - source.issues.first.message.should eq "Shadowing outer local variable `foo`" + CRYSTAL end it "doesn't report if an outer var is reassigned in a block" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL def foo foo = 1 3.times do |i| foo = 2 end end - ) - subject.catch(source).should be_valid + CRYSTAL end it "doesn't report if an argument is a black hole '_'" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL _ = 1 3.times do |_| end - ) - subject.catch(source).should be_valid + CRYSTAL end it "doesn't report if it shadows record type declaration" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL class FooBar record Foo, index : String @@ -142,12 +133,11 @@ module Ameba::Rule::Lint end end end - ) - subject.catch(source).should be_valid + CRYSTAL end it "doesn't report if it shadows throwaway arguments" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL data = [{1, "a"}, {2, "b"}, {3, "c"}] data.each do |_, string| @@ -155,18 +145,16 @@ module Ameba::Rule::Lint puts string, number end end - ) - subject.catch(source).should be_valid + CRYSTAL end it "does not report if argument shadows an ivar assignment" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def bar(@foo) @foo.try do |foo| end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports rule, location and message" do @@ -185,7 +173,7 @@ module Ameba::Rule::Lint context "macro" do it "does not report shadowed vars in outer scope" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL macro included def foo {% for ivar in instance_vars %} @@ -197,12 +185,11 @@ module Ameba::Rule::Lint {% instance_vars.reject { |ivar| ivar } %} end end - ) - subject.catch(source).should be_valid + CRYSTAL end it "does not report shadowed vars in macro within the same scope" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL {% methods = klass.methods.select { |m| m.annotation(MyAnn) } %} {% for m, m_idx in methods %} @@ -210,12 +197,11 @@ module Ameba::Rule::Lint {% d %} {% end %} {% end %} - ) - subject.catch(source).should be_valid + CRYSTAL end it "does not report shadowed vars within nested macro" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL module Foo macro included def foo @@ -233,12 +219,11 @@ module Ameba::Rule::Lint end end end - ) - subject.catch(source).should be_valid + CRYSTAL end it "does not report scoped vars to MacroFor" do - source = Source.new %( + expect_no_issues subject, <<-CRYSTAL struct Test def test {% for ivar in @type.instance_vars %} @@ -248,8 +233,7 @@ module Ameba::Rule::Lint {% ["a", "b"].map { |ivar| puts ivar } %} end end - ) - subject.catch(source).should be_valid + CRYSTAL end end end From 3f7ade573a7b8a26283570d31f1acae0742f43d7 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 01:34:34 +0200 Subject: [PATCH 3/8] Add ability to skip some of the nodes in `AST::ScopeVisitor` --- src/ameba/ast/visitors/scope_visitor.cr | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index aaae84ea..e6f55394 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -8,14 +8,17 @@ module Ameba::AST @scope_queue = [] of Scope @current_scope : Scope + @skip : Array(Crystal::ASTNode.class)? - def initialize(@rule, @source) + def initialize(@rule, @source, skip = nil) + @skip = skip.try &.map(&.as(Crystal::ASTNode.class)) @current_scope = Scope.new(@source.ast) # top level scope @source.ast.accept self @scope_queue.each { |scope| @rule.test @source, scope.node, scope } end private def on_scope_enter(node) + return if skip?(node) @current_scope = Scope.new(node, @current_scope) end @@ -182,5 +185,9 @@ module Ameba::AST private def record_macro?(node) node.name == RECORD_NODE_NAME && node.args.first?.is_a?(Crystal::Path) end + + private def skip?(node) + !!@skip.try(&.includes?(node.class)) + end end end From a38cfd16612b6709feb84516494a51c124363a4f Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 01:35:32 +0200 Subject: [PATCH 4/8] Refactor `AST::ScopeVisitor` similarly to `AST::NodeVisitor` --- src/ameba/ast/visitors/scope_visitor.cr | 84 +++++++------------------ 1 file changed, 23 insertions(+), 61 deletions(-) diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index e6f55394..06931ff3 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -3,11 +3,28 @@ require "./base_visitor" module Ameba::AST # AST Visitor that traverses the source and constructs scopes. class ScopeVisitor < BaseVisitor + # Non-exhaustive list of nodes to be visited by Ameba's rules. + NODES = [ + ClassDef, + ModuleDef, + EnumDef, + LibDef, + FunDef, + TypeDef, + TypeOf, + CStructOrUnionDef, + ProcLiteral, + Block, + Macro, + MacroFor, + ] + SUPER_NODE_NAME = "super" RECORD_NODE_NAME = "record" @scope_queue = [] of Scope @current_scope : Scope + @current_assign : Crystal::ASTNode? @skip : Array(Crystal::ASTNode.class)? def initialize(@rule, @source, skip = nil) @@ -39,73 +56,18 @@ module Ameba::AST on_scope_end(node) if @current_scope.eql?(node) end - # :nodoc: - def visit(node : Crystal::ClassDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::ModuleDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::EnumDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::LibDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::FunDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::TypeDef) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::TypeOf) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::CStructOrUnionDef) - on_scope_enter(node) - end + {% for name in NODES %} + # :nodoc: + def visit(node : Crystal::{{name}}) + on_scope_enter(node) + end + {% end %} # :nodoc: def visit(node : Crystal::Def) node.name == "->" || on_scope_enter(node) end - # :nodoc: - def visit(node : Crystal::ProcLiteral) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::Block) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::Macro) - on_scope_enter(node) - end - - # :nodoc: - def visit(node : Crystal::MacroFor) - on_scope_enter(node) - end - - @current_assign : Crystal::ASTNode? - # :nodoc: def visit(node : Crystal::Assign | Crystal::OpAssign | Crystal::MultiAssign | Crystal::UninitializedVar) @current_assign = node From 2e9ef7fcb238b9fca8f7843a67f45b549ef6672c Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 01:35:59 +0200 Subject: [PATCH 5/8] Reword `visit(node)` methods comment in `AST::NodeVisitor` --- src/ameba/ast/visitors/node_visitor.cr | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ameba/ast/visitors/node_visitor.cr b/src/ameba/ast/visitors/node_visitor.cr index 9119b22c..a37eea08 100644 --- a/src/ameba/ast/visitors/node_visitor.cr +++ b/src/ameba/ast/visitors/node_visitor.cr @@ -46,7 +46,9 @@ module Ameba::AST {% for name in NODES %} # A visit callback for `Crystal::{{name}}` node. - # Returns true meaning that child nodes will be traversed as well. + # + # Returns `true` if the child nodes should be traversed as well, + # `false` otherwise. def visit(node : Crystal::{{name}}) return false if skip?(node) From 614fcfa6a87a206a448c64299919128a18deda87 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 01:36:44 +0200 Subject: [PATCH 6/8] Skip macro scopes in `ShadowingOuterLocalVar` --- .../rule/lint/shadowing_outer_local_var_spec.cr | 17 +++++++++++++++++ .../rule/lint/shadowing_outer_local_var.cr | 9 +++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr index 25bea9b0..5706841f 100644 --- a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr +++ b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr @@ -235,6 +235,23 @@ module Ameba::Rule::Lint end CRYSTAL end + + # https://github.com/crystal-ameba/ameba/issues/224#issuecomment-822245167 + it "does not report scoped vars to MacroFor (2)" do + expect_no_issues subject, <<-CRYSTAL + struct Test + def test + {% begin %} + {% for ivar in @type.instance_vars %} + {% var_type = ivar %} + {% end %} + + {% ["a", "b"].map { |ivar| puts ivar } %} + {% end %} + end + end + CRYSTAL + end end end end diff --git a/src/ameba/rule/lint/shadowing_outer_local_var.cr b/src/ameba/rule/lint/shadowing_outer_local_var.cr index 8f70a285..d9abaec8 100644 --- a/src/ameba/rule/lint/shadowing_outer_local_var.cr +++ b/src/ameba/rule/lint/shadowing_outer_local_var.cr @@ -39,7 +39,10 @@ module Ameba::Rule::Lint MSG = "Shadowing outer local variable `%s`" def test(source) - AST::ScopeVisitor.new self, source + AST::ScopeVisitor.new self, source, skip: [ + Crystal::Macro, + Crystal::MacroFor, + ] end def test(source, node : Crystal::ProcLiteral, scope : AST::Scope) @@ -51,9 +54,7 @@ module Ameba::Rule::Lint end private def find_shadowing(source, scope) - outer_scope = scope.outer_scope - - return if outer_scope.nil? || outer_scope.in_macro? + return unless outer_scope = scope.outer_scope scope.arguments.reject(&.ignored?).each do |arg| variable = outer_scope.find_variable(arg.name) From 71de3f001281a6717617d88e5b1dd94819de4e06 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 02:01:30 +0200 Subject: [PATCH 7/8] Cleanup method docs in `AST::Scope` --- src/ameba/ast/scope.cr | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index bacf4258..87815687 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -71,7 +71,7 @@ module Ameba::AST ivariables << InstanceVariable.new(node) end - # Returns variable by its name or nil if it does not exist. + # Returns variable by its name or `nil` if it does not exist. # # ``` # scope = Scope.new(class_node, nil) @@ -91,13 +91,13 @@ module Ameba::AST find_variable(name).try &.assign(node, self) end - # Returns true if current scope represents a block (or proc), - # false if not. + # Returns `true` if current scope represents a block (or proc), + # `false` otherwise. def block? node.is_a?(Crystal::Block) || node.is_a?(Crystal::ProcLiteral) end - # Returns true if current scope represents a spawn block, e. g. + # Returns `true` if current scope represents a spawn block, e. g. # # ``` # spawn do @@ -110,18 +110,18 @@ module Ameba::AST call.try(&.name) == "spawn" end - # Returns true if current scope sits inside a macro. + # Returns `true` if current scope sits inside a macro. def in_macro? node.is_a?(Crystal::Macro) || !!outer_scope.try(&.in_macro?) end - # Returns true instance variable assinged in this scope. + # Returns `true` if instance variable is assinged in this scope. def assigns_ivar?(name) arguments.find(&.name.== name) && ivariables.find(&.name.== "@#{name}") end - # Returns true if and only if current scope represents some + # Returns `true` if and only if current scope represents some # type definition, for example a class. def type_definition? node.is_a?(Crystal::ClassDef) || @@ -133,7 +133,7 @@ module Ameba::AST end # Returns true if current scope (or any of inner scopes) references variable, - # false if not. + # `false` otherwise. def references?(variable : Variable, check_inner_scopes = true) variable.references.any? do |reference| return true if reference.scope == self @@ -141,17 +141,17 @@ module Ameba::AST end || variable.used_in_macro? end - # Returns true if current scope is a def, false if not. + # Returns `true` if current scope is a def, `false` otherwise. def def? node.is_a?(Crystal::Def) end - # Returns true if this scope is a top level scope, false if not. + # Returns `true` if this scope is a top level scope, `false` otherwise. def top_level? outer_scope.nil? end - # Returns true if var is an argument in current scope, false if not. + # Returns true if var is an argument in current scope, `false` otherwise. def arg?(var) case current_node = node when Crystal::Def @@ -169,7 +169,7 @@ module Ameba::AST args.any? { |arg| arg.name == var.name && arg.location == var.location } end - # Returns true if the `node` represents exactly + # Returns `true` if the *node* represents exactly # the same Crystal node as `@node`. def eql?(node) node == @node && From e6ad7c5d2465593d8fcfcaf2b6dea1981372d552 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 4 Apr 2022 02:04:04 +0200 Subject: [PATCH 8/8] Fix bug in `AST::Scope#in_macro?` not taking account for `Crystal::MacroFor` --- src/ameba/ast/scope.cr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 87815687..bcae082c 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -112,7 +112,8 @@ module Ameba::AST # Returns `true` if current scope sits inside a macro. def in_macro? - node.is_a?(Crystal::Macro) || !!outer_scope.try(&.in_macro?) + (node.is_a?(Crystal::Macro) || node.is_a?(Crystal::MacroFor)) || + !!outer_scope.try(&.in_macro?) end # Returns `true` if instance variable is assinged in this scope.