Detect shadowing outer local vars

spec/ameba/formatter/disabled_formatter_spec.cr

@@ -15,9 +15,9 @@ module Ameba::Formatter
         Colorize.enabled = false
 
         path = "source.cr"
-        s = Source.new("", path).tap do |s|
+        s = Source.new("", path).tap do |source|
-          s.error(ErrorRule.new, 1, 2, "ErrorRule", :disabled)
+          source.error(ErrorRule.new, 1, 2, "ErrorRule", :disabled)
-          s.error(NamedRule.new, 2, 2, "NamedRule", :disabled)
+          source.error(NamedRule.new, 2, 2, "NamedRule", :disabled)
         end
         subject.finished [s]
         log = output.to_s
@@ -29,9 +29,9 @@ module Ameba::Formatter
       end
 
       it "does not write not-disabled rules" do
-        s = Source.new("", "source.cr").tap do |s|
+        s = Source.new("", "source.cr").tap do |source|
-          s.error(ErrorRule.new, 1, 2, "ErrorRule")
+          source.error(ErrorRule.new, 1, 2, "ErrorRule")
-          s.error(NamedRule.new, 2, 2, "NamedRule", :disabled)
+          source.error(NamedRule.new, 2, 2, "NamedRule", :disabled)
         end
         subject.finished [s]
         output.to_s.should_not contain ErrorRule.name

spec/ameba/formatter/dot_formatter_spec.cr

@@ -39,9 +39,9 @@ module Ameba::Formatter
 
       context "when errors found" do
         it "writes each error" do
-          s = Source.new("").tap do |s|
+          s = Source.new("").tap do |source|
-            s.error(DummyRule.new, 1, 1, "DummyRuleError")
+            source.error(DummyRule.new, 1, 1, "DummyRuleError")
-            s.error(NamedRule.new, 1, 2, "NamedRuleError")
+            source.error(NamedRule.new, 1, 2, "NamedRuleError")
           end
           subject.finished [s]
           log = output.to_s

spec/ameba/rule/shadowing_local_outer_var_spec.cr

@@ -0,0 +1,140 @@
+require "../../spec_helper"
+
+module Ameba::Rule
+  describe ShadowingOuterLocalVar do
+    subject = ShadowingOuterLocalVar.new
+
+    it "doesn't report if there is no shadowing" do
+      source = Source.new %(
+        def some_method
+          foo = 1
+
+          3.times do |bar|
+            bar
+          end
+
+          -> (baz : Int32) {}
+
+          -> (bar : String) {}
+        end
+      )
+
+      subject.catch(source).should be_valid
+    end
+
+    it "reports if there is a shadowing in a block" do
+      source = Source.new %(
+        def some_method
+          foo = 1
+
+          3.times do |foo|
+          end
+        end
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    it "reports if there is a shadowing in a proc" do
+      source = Source.new %(
+        def some_method
+          foo = 1
+
+          -> (foo : Int32) {}
+        end
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    it "reports if there is a shadowing in an inner scope" do
+      source = Source.new %(
+        def foo
+          foo = 1
+
+          3.times do |i|
+            3.times { |foo| foo }
+          end
+        end
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    it "reports if variable is shadowed twice" do
+      source = Source.new %(
+        foo = 1
+
+        3.times do |foo|
+          -> (foo : Int32) { foo + 1 }
+        end
+      )
+      subject.catch(source).should_not be_valid
+
+      source.errors.size.should eq 2
+    end
+
+    it "reports if a splat block argument shadows local var" do
+      source = Source.new %(
+        foo = 1
+
+        3.times do |*foo|
+        end
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    it "reports if a &block argument is shadowed" do
+      source = Source.new %(
+        def method_with_block(a, &block)
+          3.times do |block|
+          end
+        end
+      )
+      subject.catch(source).should_not be_valid
+      source.errors.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 %(
+        foo = 1
+        [1, 2, 3].each_with_index do |i, foo|
+          i + foo
+        end
+      )
+      subject.catch(source).should_not be_valid
+      source.errors.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 %(
+        def foo
+          foo = 1
+          3.times do |i|
+            foo = 2
+          end
+        end
+      )
+      subject.catch(source).should be_valid
+    end
+
+    it "doesn't report if an argument is a black hole '_'" do
+      source = Source.new %(
+        _ = 1
+        3.times do |_|
+        end
+      )
+      subject.catch(source).should be_valid
+    end
+
+    it "reports rule, location and message" do
+      source = Source.new %(
+        foo = 1
+        3.times { |foo| foo + 1 }
+      ), "source.cr"
+      subject.catch(source).should_not be_valid
+
+      error = source.errors.first
+      error.rule.should_not be_nil
+      error.location.to_s.should eq "source.cr:3:20"
+      error.message.should eq "Shadowing outer local variable `foo`"
+    end
+  end
+end

spec/ameba/runner_spec.cr

@@ -43,13 +43,13 @@ module Ameba
         path = "source.cr"
         source = Source.new "", path
 
-        rules = ([] of Rule::Base).tap do |rules|
+        all_rules = ([] of Rule::Base).tap do |rules|
           rule = ErrorRule.new
           rule.excluded = [path]
           rules << rule
         end
 
-        Runner.new(rules, [source], formatter).run.success?.should be_true
+        Runner.new(all_rules, [source], formatter).run.success?.should be_true
       end
 
       context "invalid syntax" do

src/ameba/ast/branch.cr

@@ -83,10 +83,10 @@ module Ameba::AST
       private def on_branchable_start(node, branches : Array | Tuple)
         @branchable = Branchable.new(node, @branchable)
 
-        branches.each do |node|
+        branches.each do |branch_node|
           break if branch # branch found
-          @current_branch = node if node && !node.nop?
+          @current_branch = branch_node if branch_node && !branch_node.nop?
-          node.try &.accept(self)
+          branch_node.try &.accept(self)
         end
 
         false

src/ameba/rule/shadowing_local_outer_var.cr

@@ -0,0 +1,62 @@
+module Ameba::Rule
+  # A rule that disallows the usage of the same name as outer local variables
+  # for block or proc arguments.
+  #
+  # For example, this is considered incorrect:
+  #
+  # ```
+  # def some_method
+  #   foo = 1
+  #
+  #   3.times do |foo| # shadowing outer `foo`
+  #   end
+  # end
+  # ```
+  #
+  # and should be written as:
+  #
+  # ```
+  # def some_method
+  #   foo = 1
+  #
+  #   3.times do |bar|
+  #   end
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # ShadowingOuterLocalVar:
+  #   Enabled: true
+  # ```
+  #
+  struct ShadowingOuterLocalVar < Base
+    properties do
+      description "Disallows the usage of the same name as outer local variables" \
+                  " for block or proc arguments."
+    end
+
+    MSG = "Shadowing outer local variable `%s`"
+
+    def test(source)
+      AST::ScopeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::ProcLiteral, scope : AST::Scope)
+      find_shadowing source, scope
+    end
+
+    def test(source, node : Crystal::Block, scope : AST::Scope)
+      find_shadowing source, scope
+    end
+
+    private def find_shadowing(source, scope)
+      scope.arguments.each do |arg|
+        if scope.outer_scope.try &.find_variable(arg.name)
+          source.error self, arg.location, MSG % arg.name
+        end
+      end
+    end
+  end
+end