Unused argument rule (#52)

* Unused argument rule

* IgnoreDefs, IgnoreBlocks, IgnoreProcs parameters

* Implicit reference by super keyworkd

* Handle macro arguments

spec/ameba/ast/variabling/argument_spec.cr

@@ -0,0 +1,40 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  describe Argument do
+    arg = Crystal::Arg.new "a"
+    scope = Scope.new as_node "foo = 1"
+    variable = Variable.new(Crystal::Var.new("foo"), scope)
+
+    describe "#initialize" do
+      it "creates a new argument" do
+        argument = Argument.new(arg, variable)
+        argument.node.should_not be_nil
+      end
+    end
+
+    describe "delegation" do
+      it "delegates location to node" do
+        argument = Argument.new(arg, variable)
+        argument.location.should eq arg.location
+      end
+
+      it "delegates to_s to node" do
+        argument = Argument.new(arg, variable)
+        argument.to_s.should eq arg.to_s
+      end
+    end
+
+    describe "#ignored?" do
+      it "is true if arg starts with _" do
+        argument = Argument.new(Crystal::Arg.new("_a"), variable)
+        argument.ignored?.should be_true
+      end
+
+      it "is false otherwise" do
+        argument = Argument.new(arg, variable)
+        argument.ignored?.should be_false
+      end
+    end
+  end
+end

spec/ameba/rule/unused_argument_spec.cr

@@ -0,0 +1,259 @@
+require "../../spec_helper"
+
+module Ameba::Rule
+  subject = UnusedArgument.new
+  subject.ignore_defs = false
+
+  describe UnusedArgument do
+    it "doesn't report if arguments are used" do
+      s = Source.new %(
+        def method(a, b, c)
+          a + b + c
+        end
+
+        3.times do |i|
+          i + 1
+        end
+
+        ->(i : Int32) { i + 1 }
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if method argument is unused" do
+      s = Source.new %(
+        def method(a, b, c)
+          a + b
+        end
+      )
+      subject.catch(s).should_not be_valid
+      s.errors.first.message.should eq "Unused argument `c`"
+    end
+
+    it "reports if block argument is unused" do
+      s = Source.new %(
+        [1,2].each_with_index do |a, i|
+          a
+        end
+      )
+      subject.catch(s).should_not be_valid
+      s.errors.first.message.should eq "Unused argument `i`"
+    end
+
+    it "reports if proc argument is unused" do
+      s = Source.new %(
+        -> (a : Int32, b : String) do
+          a = a + 1
+        end
+      )
+      subject.catch(s).should_not be_valid
+      s.errors.first.message.should eq "Unused argument `b`"
+    end
+
+    it "reports multiple unused args" do
+      s = Source.new %(
+        def method(a, b, c)
+          nil
+        end
+      )
+      subject.catch(s).should_not be_valid
+      s.errors[0].message.should eq "Unused argument `a`"
+      s.errors[1].message.should eq "Unused argument `b`"
+      s.errors[2].message.should eq "Unused argument `c`"
+    end
+
+    it "doesn't report if it is an instance var argument" do
+      s = Source.new %(
+        class A
+          def method(@name)
+          end
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if a typed argument is used" do
+      s = Source.new %(
+        def method(x : Int32)
+          3.times do
+            puts x
+          end
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if an argument with default value is used" do
+      s = Source.new %(
+        def method(x = 1)
+          puts x
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if argument starts with a _" do
+      s = Source.new %(
+        def method(_x)
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if it is a block and used" do
+      s = Source.new %(
+        def method(&block)
+          block.call
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "reports if block arg is not used" do
+      s = Source.new %(
+        def method(&block)
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports if unused and there is yield" do
+      s = Source.new %(
+        def method(&block)
+          yield 1
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "doesn't report if variable is referenced implicitly" do
+      s = Source.new %(
+        class Bar < Foo
+          def method(a, b)
+            super
+          end
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    context "super" do
+      it "reports if variable is not referenced implicitly by super" do
+        s = Source.new %(
+          class Bar < Foo
+            def method(a, b)
+              super a
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.errors.first.message.should eq "Unused argument `b`"
+      end
+
+      it "reports rule, location and message" do
+        s = Source.new %(
+          def method(a)
+          end
+        ), "source.cr"
+        subject.catch(s).should_not be_valid
+        error = s.errors.first
+        error.rule.should_not be_nil
+        error.message.should eq "Unused argument `a`"
+        error.location.to_s.should eq "source.cr:2:22"
+      end
+    end
+
+    context "macro" do
+      it "doesn't report if it is a used macro argument" do
+        s = Source.new %(
+          macro my_macro(arg)
+            {% arg %}
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if it is a used macro block argument" do
+        s = Source.new %(
+          macro my_macro(&block)
+            {% block %}
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report used macro args with equal names in record" do
+        s = Source.new %(
+          record X do
+            macro foo(a, b)
+              {{a}} + {{b}}
+            end
+
+            macro bar(a, b, c)
+              {{a}} + {{b}} + {{c}}
+            end
+          end
+        )
+        subject.catch(s).should be_valid
+      end
+    end
+
+    context "properties" do
+      describe "#ignore_defs" do
+        it "lets the rule to ignore def scopes if true" do
+          subject.ignore_defs = true
+          s = Source.new %(
+            def method(a)
+            end
+          )
+          subject.catch(s).should be_valid
+        end
+
+        it "lets the rule not to ignore def scopes if false" do
+          subject.ignore_defs = false
+          s = Source.new %(
+            def method(a)
+            end
+          )
+          subject.catch(s).should_not be_valid
+        end
+      end
+
+      context "#ignore_blocks" do
+        it "lets the rule to ignore block scopes if true" do
+          subject.ignore_blocks = true
+          s = Source.new %(
+            3.times { |i| puts "yo!" }
+          )
+          subject.catch(s).should be_valid
+        end
+
+        it "lets the rule not to ignore block scopes if false" do
+          subject.ignore_blocks = false
+          s = Source.new %(
+            3.times { |i| puts "yo!" }
+          )
+          subject.catch(s).should_not be_valid
+        end
+      end
+
+      context "#ignore_procs" do
+        it "lets the rule to ignore proc scopes if true" do
+          subject.ignore_procs = true
+          s = Source.new %(
+            ->(a : Int32) {}
+          )
+          subject.catch(s).should be_valid
+        end
+
+        it "lets the rule not to ignore proc scopes if false" do
+          subject.ignore_procs = false
+          s = Source.new %(
+            ->(a : Int32) {}
+          )
+          subject.catch(s).should_not be_valid
+        end
+      end
+    end
+  end
+end

src/ameba/ast/scope.cr

@@ -7,6 +7,9 @@ module Ameba::AST
     # Link to local variables
     getter variables = [] of Variable
 
+    # Link to the arguments in current scope
+    getter arguments = [] of Argument
+
     # Link to the outer scope
     getter outer_scope : Scope?
 
@@ -40,6 +43,11 @@ module Ameba::AST
       variables << Variable.new(node, self)
     end
 
+    def add_argument(node)
+      add_variable Crystal::Var.new(node.name).at(node.location)
+      arguments << Argument.new(node, variables.last)
+    end
+
     # Returns variable by its name or nil if it does not exist.
     #
     # ```
@@ -77,34 +85,43 @@ module Ameba::AST
         node.is_a?(Crystal::CStructOrUnionDef)
     end
 
-    # Returns true if current scope references variable, false if not.
+    # Returns true if current scope (or any of inner scopes) references variable,
+    # false if not.
     def references?(variable : Variable)
-      variable.references.any? { |reference| reference.scope == self }
+      variable.references.any? do |reference|
+        reference.scope == self || inner_scopes.any?(&.references? variable)
+      end
     end
 
-    # Returns arguments of this scope (if any).
+    # Returns true if current scope is a def, false if not.
-    def args
+    def def?
+      node.is_a? Crystal::Def
+    end
+
+    # Returns true if var is an argument in current scope, false if not.
+    def arg?(var)
       case current_node = node
-      when Crystal::Block, Crystal::Def then current_node.args
+      when Crystal::Def
-      when Crystal::ProcLiteral         then current_node.def.args
+        var.is_a?(Crystal::Arg) && any_arg?(current_node.args, var)
+      when Crystal::Block
+        var.is_a?(Crystal::Var) && any_arg?(current_node.args, var)
+      when Crystal::ProcLiteral
+        var.is_a?(Crystal::Var) && any_arg?(current_node.def.args, var)
       else
-        [] of Crystal::Var
+        false
       end
     end
 
-    # Returns true if variable is an argument in current scope, false if not.
+    private def any_arg?(args, var)
-    def arg?(var : Crystal::Var)
+      args.any? { |arg| arg.name == var.name && arg.location == var.location }
-      args.any? do |arg|
-        arg.is_a?(Crystal::Var) &&
-          arg.name == var.name &&
-          arg.location == var.location
-      end
     end
 
     # Returns true if the `node` represents exactly
     # the same Crystal node as `@node`.
     def eql?(node)
-      node == @node && node.location == @node.location
+      node == @node &&
+        !node.location.nil? &&
+        node.location == @node.location
     end
   end
 end

src/ameba/ast/variabling/argument.cr

@@ -0,0 +1,48 @@
+module Ameba::AST
+  # Represents the argument of some node.
+  # Holds the reference to the variable, thus to scope.
+  #
+  # For example, all these vars are arguments:
+  #
+  # ```
+  # def method(a, b, c = 10, &block)
+  #   3.times do |i|
+  #   end
+  #
+  #   ->(x : Int32) {}
+  # end
+  # ```
+  class Argument
+    # The actual node.
+    getter node : Crystal::Var | Crystal::Arg
+
+    # Variable of this argument (may be the same node)
+    getter variable : Variable
+
+    delegate location, to: @node
+    delegate to_s, to: @node
+
+    # Creates a new argument.
+    #
+    # ```
+    # Argument.new(node, variable)
+    # ```
+    def initialize(@node, @variable)
+    end
+
+    # Returns true if the name starts with '_', false if not.
+    def ignored?
+      name.starts_with? '_'
+    end
+
+    # Name of the argument.
+    def name
+      case current_node = node
+      when Crystal::Var then current_node.name
+      when Crystal::Arg then current_node.name
+      else
+        raise ArgumentError.new "invalid node"
+      end
+    end
+  end
+end

src/ameba/ast/variabling/reference.cr

@@ -5,5 +5,6 @@ module Ameba::AST
   # It behaves like a variable is used to distinguish a
   # the variable from its reference.
   class Reference < Variable
+    property? explicit = true
   end
 end

src/ameba/ast/variabling/variable.cr

@@ -105,8 +105,6 @@ module Ameba::AST
     # 3.times { |i| i + 1 }
     # ```
     def captured_by_block?(scope = @scope)
-      return false unless scope
-
       scope.inner_scopes.each do |inner_scope|
         return true if inner_scope.block? && inner_scope.references?(self)
         return true if captured_by_block?(inner_scope)

src/ameba/ast/visitors/scope_visitor.cr

@@ -73,6 +73,11 @@ module Ameba::AST
       on_scope_enter(node)
     end
 
+    # :nodoc:
+    def visit(node : Crystal::Macro)
+      on_scope_enter(node)
+    end
+
     @current_assign : Crystal::ASTNode?
 
     # :nodoc:
@@ -94,19 +99,22 @@ module Ameba::AST
 
     # :nodoc:
     def visit(node : Crystal::Arg)
-      @current_scope.add_variable Crystal::Var.new(node.name).at(node.location)
+      @current_scope.add_argument node
     end
 
     # :nodoc:
     def visit(node : Crystal::Var)
-      if !@current_scope.arg?(node) && (variable = @current_scope.find_variable node.name)
+      variable = @current_scope.find_variable node.name
-        reference = variable.reference node, @current_scope
 
+      if @current_scope.arg?(node) # node is an argument
+        @current_scope.add_argument(node)
+      elsif variable.nil? && @current_assign # node is a variable
+        @current_scope.add_variable(node)
+      elsif variable # node is a reference
+        reference = variable.reference node, @current_scope
         if @current_assign.is_a?(Crystal::OpAssign) || !reference.target_of?(@current_assign)
           variable.reference_assignments!
         end
-      else
-        @current_scope.add_variable(node)
       end
     end
 
@@ -114,6 +122,17 @@ module Ameba::AST
     def visit(node : Crystal::MacroLiteral)
       MacroLiteralVarVisitor.new(node).vars.each { |var| visit(var) }
     end
+
+    # :nodoc:
+    def visit(node : Crystal::Call)
+      return true unless node.name == "super" && node.args.empty?
+      return true unless (scope = @current_scope).def?
+      scope.arguments.each do |arg|
+        variable = arg.variable
+        variable.reference(variable.node, scope).explicit = false
+      end
+      true
+    end
   end
 
   private class MacroLiteralVarVisitor < Crystal::Visitor

src/ameba/rule/unused_argument.cr

@@ -0,0 +1,63 @@
+module Ameba::Rule
+  # A rule that reports unused arguments.
+  # For example, this is considered invalid:
+  #
+  # ```
+  # def method(a, b, c)
+  #   a + b
+  # end
+  # ```
+  # and should be written as:
+  #
+  # ```
+  # def method(a, b)
+  #   a + b
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # UnusedArgument:
+  #   Enabled: true
+  #   IgnoreDefs: true
+  #   IgnoreBlocks: false
+  #   IgnoreProcs: false
+  # ```
+  #
+  struct UnusedArgument < Base
+    properties do
+      description "Disallows unused arguments"
+
+      ignore_defs true
+      ignore_blocks false
+      ignore_procs false
+    end
+
+    MSG = "Unused argument `%s`"
+
+    def test(source)
+      AST::ScopeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::ProcLiteral, scope : AST::Scope)
+      ignore_procs || find_unused_arguments source, scope
+    end
+
+    def test(source, node : Crystal::Block, scope : AST::Scope)
+      ignore_blocks || find_unused_arguments source, scope
+    end
+
+    def test(source, node : Crystal::Def, scope : AST::Scope)
+      ignore_defs || find_unused_arguments source, scope
+    end
+
+    private def find_unused_arguments(source, scope)
+      scope.arguments.each do |argument|
+        next if argument.ignored? || scope.references?(argument.variable)
+
+        source.error self, argument.location, MSG % argument.name
+      end
+    end
+  end
+end