Shadowed argument

spec/ameba/ast/scope_spec.cr

@@ -43,14 +43,14 @@ module Ameba::AST
     it "creates a new assignment" do
       scope = Scope.new as_node("foo = 1")
       scope.add_variable Crystal::Var.new "foo"
-      scope.assign_variable(Crystal::Var.new "foo")
+      scope.assign_variable("foo", Crystal::Var.new "foo")
       scope.find_variable("foo").not_nil!.assignments.size.should eq 1
     end
 
     it "does not create the assignment if variable is wrong" do
       scope = Scope.new as_node("foo = 1")
       scope.add_variable Crystal::Var.new "foo"
-      scope.assign_variable(Crystal::Var.new "bar")
+      scope.assign_variable("bar", Crystal::Var.new "bar")
       scope.find_variable("foo").not_nil!.assignments.size.should eq 0
     end
   end

spec/ameba/rule/shadowed_argument_spec.cr

@@ -0,0 +1,165 @@
+require "../../spec_helper"
+
+module Ameba::Rule
+  describe ShadowedArgument do
+    subject = ShadowedArgument.new
+
+    it "doesn't report if there is not a shadowed argument" do
+      s = Source.new %(
+        def foo(bar)
+          baz = 1
+        end
+
+        3.times do |i|
+          a = 1
+        end
+
+        proc = -> (a : Int32) {
+          b = 2
+        }
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if there is a shadowed method argument" do
+      s = Source.new %(
+        def foo(bar)
+          bar = 1
+          bar
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports if there is a shadowed block argument" do
+      s = Source.new %(
+        3.times do |i|
+          i = 2
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports if there is a shadowed proc argument" do
+      s = Source.new %(
+        ->(x : Int32) {
+          x = 20
+          x
+        }
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "doesn't report if the argument is referenced before the assignment" do
+      s = Source.new %(
+        def foo(bar)
+          bar
+          bar = 1
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if the argument is conditionally reassigned" do
+      s = Source.new %(
+        def foo(bar = nil)
+          bar ||= true
+          bar
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if the op assign is followed by another assignment" do
+      s = Source.new %(
+        def foo(bar)
+          bar ||= 3
+          bar = 43
+          bar
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if the shadowing assignment is followed by op assign" do
+      s = Source.new %(
+        def foo(bar)
+          bar = 42
+          bar ||= 43
+          bar
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "doesn't report if the argument is unused" do
+      s = Source.new %(
+        def foo(bar)
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if the argument is shadowed before super" do
+      s = Source.new %(
+        def foo(bar)
+          bar = 1
+          super
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    context "branch" do
+      it "doesn't report if the argument is not shadowed in a condition" do
+        s = Source.new %(
+          def foo(bar, baz)
+            bar = 1 if baz
+            bar
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "reports if the argument is shadowed after the condition" do
+        s = Source.new %(
+          def foo(foo)
+            if something
+              foo = 42
+            end
+            foo = 43
+            foo
+          end
+        )
+        subject.catch(s).should_not be_valid
+      end
+
+      it "doesn't report if the argument is conditionally assigned in a branch" do
+        s = Source.new %(
+          def foo(bar)
+            if something
+              bar ||= 22
+            end
+            bar
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+    end
+
+    it "reports rule, location and message" do
+      s = Source.new %(
+        def foo(bar)
+          bar = 22
+          bar
+        end
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+
+      error = s.errors.first
+      error.rule.should_not be_nil
+      error.location.to_s.should eq "source.cr:3:11"
+      error.message.should eq "Argument `bar` is assigned before it is used"
+    end
+  end
+end

src/ameba/ast/scope.cr

@@ -62,10 +62,10 @@ module Ameba::AST
     #
     # ```
     # scope = Scope.new(class_node, nil)
-    # scope.assign_variable(var_node)
+    # scope.assign_variable(var_name, assign_node)
     # ```
-    def assign_variable(node)
-      node.is_a?(Crystal::Var) && find_variable(node.name).try &.assign(node)
+    def assign_variable(name, node)
+      find_variable(name).try &.assign(node)
     end
 
     # Returns true if current scope represents a block (or proc),

src/ameba/ast/variabling/assignment.cr

@@ -16,7 +16,6 @@ module Ameba::AST
     # Branch of this assignment.
     getter branch : Branch?
 
-    delegate location, to: @node
     delegate to_s, to: @node
     delegate scope, to: @variable
 
@@ -38,5 +37,40 @@ module Ameba::AST
     def referenced_in_loop?
       @variable.referenced? && @branch.try &.in_loop?
     end
+
+    # Returns true if this assignment is an op assign, false if not.
+    # For example, this is an op assign:
+    #
+    # ```
+    # a ||= 1
+    # ```
+    def op_assign?
+      node.is_a? Crystal::OpAssign
+    end
+
+    # Returns true if this assignment is in a branch, false if not.
+    # For example, this assignment is in a branch:
+    #
+    # ```
+    # a = 1 if a.nil?
+    # ```
+    def in_branch?
+      !branch.nil?
+    end
+
+    # Returns the location of the current variable in the assignment.
+    def location
+      case assign = node
+      when Crystal::Assign           then assign.target.location
+      when Crystal::OpAssign         then assign.target.location
+      when Crystal::UninitializedVar then assign.var.location
+      when Crystal::MultiAssign
+        assign.targets.find do |target|
+          target.is_a?(Crystal::Var) && target.name == variable.name
+        end.try &.location
+      else
+        node.location
+      end
+    end
   end
 end

src/ameba/ast/variabling/variable.cr

@@ -14,6 +14,9 @@ module Ameba::AST
     # Scope of this variable.
     getter scope : Scope
 
+    # Node of the first assignment which can be available before any reference.
+    getter assign_before_reference : Crystal::ASTNode?
+
     delegate location, to: @node
     delegate name, to: @node
     delegate to_s, to: @node
@@ -44,6 +47,8 @@ module Ameba::AST
     #
     def assign(node)
       assignments << Assignment.new(node, self)
+
+      update_assign_reference!
     end
 
     # Returns true if variable has any reference.
@@ -127,9 +132,10 @@ module Ameba::AST
     # false otherwise.
     def target_of?(assign)
       case assign
-      when Crystal::Assign      then eql?(assign.target)
-      when Crystal::OpAssign    then eql?(assign.target)
-      when Crystal::MultiAssign then assign.targets.any? { |t| eql?(t) }
+      when Crystal::Assign           then eql?(assign.target)
+      when Crystal::OpAssign         then eql?(assign.target)
+      when Crystal::MultiAssign      then assign.targets.any? { |t| eql?(t) }
+      when Crystal::UninitializedVar then eql?(assign.var)
       else
         false
       end
@@ -162,5 +168,13 @@ module Ameba::AST
         @macro_literals << node
       end
     end
+
+    private def update_assign_reference!
+      if @assign_before_reference.nil? &&
+         references.size <= assignments.size &&
+         assignments.none? { |ass| ass.op_assign? }
+        @assign_before_reference = assignments.find { |ass| !ass.in_branch? }.try &.node
+      end
+    end
   end
 end

src/ameba/ast/visitors/scope_visitor.cr

@@ -23,6 +23,10 @@ module Ameba::AST
       end
     end
 
+    private def on_assign_end(target, node)
+      target.is_a?(Crystal::Var) && @current_scope.assign_variable(target.name, node)
+    end
+
     # :nodoc:
     def end_visit(node : Crystal::ASTNode)
       on_scope_end(node) if @current_scope.eql?(node)
@@ -87,21 +91,21 @@ module Ameba::AST
 
     # :nodoc:
     def end_visit(node : Crystal::Assign | Crystal::OpAssign)
-      @current_scope.assign_variable(node.target)
+      on_assign_end(node.target, node)
       @current_assign = nil
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
     # :nodoc:
     def end_visit(node : Crystal::MultiAssign)
-      node.targets.each { |target| @current_scope.assign_variable(target) }
+      node.targets.each { |target| on_assign_end(target, node) }
       @current_assign = nil
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
     # :nodoc:
     def end_visit(node : Crystal::UninitializedVar)
-      @current_scope.assign_variable(node.var)
+      on_assign_end(node.var, node)
       @current_assign = nil
       on_scope_end(node) if @current_scope.eql?(node)
     end

src/ameba/rule/shadowed_argument.cr

@@ -0,0 +1,58 @@
+module Ameba::Rule
+  # A rule that disallows shadowed arguments.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # do_something do |foo|
+  #   foo = 1 # shadows block argument
+  #   foo
+  # end
+  #
+  # def do_something(foo)
+  #   foo = 1 # shadows method argument
+  #   foo
+  # end
+  # ```
+  #
+  # and it should be written as follows:
+  #
+  # ```
+  # do_something do |foo|
+  #   foo = foo + 42
+  #   foo
+  # end
+  #
+  # def do_something(foo)
+  #   foo = foo + 42
+  #   foo
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # ShadowedArgument:
+  #   Enabled: true
+  # ```
+  #
+  struct ShadowedArgument < Base
+    properties do
+      description "Disallows shadowed arguments"
+    end
+
+    MSG = "Argument `%s` is assigned before it is used"
+
+    def test(source)
+      AST::ScopeVisitor.new self, source
+    end
+
+    def test(source, node, scope : AST::Scope)
+      scope.arguments.each do |arg|
+        next unless assign = arg.variable.assign_before_reference
+
+        source.error self, assign.location, MSG % arg.name
+      end
+    end
+  end
+end