Merge pull request #260 from crystal-ameba/fix-issue-224

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
-      )
+        CRYSTAL
-
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
-
-      source.issues.size.should eq 2
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
-      source.issues.first.message.should eq "Shadowing outer local variable `block`"
     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
-      )
+        CRYSTAL
-      subject.catch(source).should_not be_valid
-      source.issues.first.message.should eq "Shadowing outer local variable `foo`"
     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
-      )
+        CRYSTAL
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(source).should be_valid
     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
-      )
+        CRYSTAL
-      subject.catch(s).should be_valid
     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
-        )
+          CRYSTAL
-        subject.catch(source).should be_valid
       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 %}
-        )
+          CRYSTAL
-        subject.catch(source).should be_valid
       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
-        )
+          CRYSTAL
-        subject.catch(source).should be_valid
       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,24 @@ module Ameba::Rule::Lint
               {% ["a", "b"].map { |ivar| puts ivar } %}
             end
           end
-        )
+          CRYSTAL
-        subject.catch(source).should be_valid
+      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

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),
+    # Returns `true` if current scope represents a block (or proc),
-    # false if not.
+    # `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,19 @@ 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?)
+      (node.is_a?(Crystal::Macro) || node.is_a?(Crystal::MacroFor)) ||
+        !!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 +134,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 +142,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 +170,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 &&

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)
 

src/ameba/ast/visitors/scope_visitor.cr

@@ -3,19 +3,39 @@ 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)
+    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
 
@@ -36,73 +56,18 @@ module Ameba::AST
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
-    # :nodoc:
+    {% for name in NODES %}
-    def visit(node : Crystal::ClassDef)
+      # :nodoc:
-      on_scope_enter(node)
+      def visit(node : Crystal::{{name}})
-    end
+        on_scope_enter(node)
-
+      end
-    # :nodoc:
+    {% end %}
-    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
 
     # :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
@@ -182,5 +147,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

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 unless outer_scope = scope.outer_scope
-
-      return if outer_scope.nil? || outer_scope.in_macro?
 
       scope.arguments.reject(&.ignored?).each do |arg|
         variable = outer_scope.find_variable(arg.name)