Merge pull request #320 from crystal-ameba/Sija/unused-block-argument

Add `Lint/UnusedBlockArgument` rule

spec/ameba/rule/lint/unused_argument_spec.cr

@@ -115,21 +115,21 @@ module Ameba::Rule::Lint
       subject.catch(s).should be_valid
     end
 
-    it "reports if block arg is not used" do
+    it "doesn't report if block arg is not used" do
       s = Source.new %(
         def method(&block)
         end
       )
-      subject.catch(s).should_not be_valid
+      subject.catch(s).should be_valid
     end
 
-    it "reports if unused and there is yield" do
+    it "doesn't report if unused and there is yield" do
       s = Source.new %(
         def method(&block)
           yield 1
         end
       )
-      subject.catch(s).should_not be_valid
+      subject.catch(s).should be_valid
     end
 
     it "doesn't report if it's an anonymous block" do

spec/ameba/rule/lint/unused_block_argument_spec.cr

@@ -0,0 +1,110 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  subject = UnusedBlockArgument.new
+
+  describe UnusedBlockArgument do
+    it "doesn't report if it is an instance var argument" do
+      s = Source.new %(
+        class A
+          def initialize(&@callback)
+          end
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if anonymous" do
+      s = Source.new %(
+        def method(a, b, c, &)
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if argument name starts with a `_`" do
+      s = Source.new %(
+        def method(a, b, c, &_block)
+        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(a, b, c, &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(a, b, c, &block)
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports if unused and there is yield" do
+      s = Source.new %(
+        def method(a, b, c, &block)
+          3.times do |i|
+            i.try do
+              yield i
+            end
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "doesn't report if anonymous and there is yield" do
+      s = Source.new %(
+        def method(a, b, c, &)
+          yield 1
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "doesn't report if variable is referenced implicitly" do
+      s = Source.new %(
+        class Bar < Foo
+          def method(a, b, c, &block)
+            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, c, &block)
+              super a, b, c
+            end
+          end
+        )
+        subject.catch(s).should_not be_valid
+        s.issues.first.message.should eq "Unused block argument `block`. If it's necessary, " \
+                                         "use `_block` as an argument name to indicate " \
+                                         "that it won't be used."
+      end
+    end
+
+    context "macro" do
+      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
+    end
+  end
+end

src/ameba/ast/scope.cr

@@ -4,6 +4,9 @@ module Ameba::AST
   # Represents a context of the local variable visibility.
   # This is where the local variables belong to.
   class Scope
+    # Whether the scope yields.
+    setter yields = false
+
     # Link to local variables
     getter variables = [] of Variable
 
@@ -143,6 +146,14 @@ module Ameba::AST
       end || variable.used_in_macro?
     end
 
+    # Returns `true` if current scope (or any of inner scopes) yields,
+    # `false` otherwise.
+    def yields?(check_inner_scopes = true)
+      return true if @yields
+      return inner_scopes.any?(&.yields?) if check_inner_scopes
+      false
+    end
+
     # Returns `true` if current scope is a def, `false` otherwise.
     def def?
       node.is_a?(Crystal::Def)

src/ameba/ast/visitors/scope_visitor.cr

@@ -19,7 +19,7 @@ module Ameba::AST
       MacroFor,
     }
 
-    SUPER_NODE_NAME  = "super"
+    SPECIAL_NODE_NAMES = %w[super previous_def]
     RECORD_NODE_NAME   = "record"
 
     @scope_queue = [] of Scope
@@ -64,6 +64,11 @@ module Ameba::AST
       end
     {% end %}
 
+    # :nodoc:
+    def visit(node : Crystal::Yield)
+      @current_scope.yields = true
+    end
+
     # :nodoc:
     def visit(node : Crystal::Def)
       node.name == "->" || on_scope_enter(node)
@@ -134,7 +139,7 @@ module Ameba::AST
     def visit(node : Crystal::Call)
       case
       when @current_scope.def?
-        if node.name == SUPER_NODE_NAME && node.args.empty?
+        if node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty?
           @current_scope.arguments.each do |arg|
             variable = arg.variable
             variable.reference(variable.node, @current_scope).explicit = false

src/ameba/rule/lint/unused_argument.cr

@@ -50,6 +50,10 @@ module Ameba::Rule::Lint
     end
 
     def test(source, node : Crystal::Def, scope : AST::Scope)
+      # `Lint/UnusedBlockArgument` rule covers this case explicitly
+      if block_arg = node.block_arg
+        scope.arguments.reject!(&.node.== block_arg)
+      end
       ignore_defs? || find_unused_arguments source, scope
     end
 

src/ameba/rule/lint/unused_block_argument.cr

@@ -0,0 +1,62 @@
+module Ameba::Rule::Lint
+  # A rule that reports unused block arguments.
+  # For example, this is considered invalid:
+  #
+  # ```
+  # def foo(a, b, &block)
+  #   a + b
+  # end
+  #
+  # def bar(&block)
+  #   yield 42
+  # end
+  # ```
+  #
+  # and should be written as:
+  #
+  # ```
+  # def foo(a, b, &_block)
+  #   a + b
+  # end
+  #
+  # def bar(&)
+  #   yield 42
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/UnusedBlockArgument:
+  #   Enabled: true
+  # ```
+  class UnusedBlockArgument < Base
+    properties do
+      description "Disallows unused block arguments"
+    end
+
+    MSG_UNUSED = "Unused block argument `%1$s`. If it's necessary, use `_%1$s` " \
+                 "as an argument name to indicate that it won't be used."
+
+    MSG_YIELDED = "Use `&` as an argument name to indicate that it won't be referenced."
+
+    def test(source)
+      AST::ScopeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::Def, scope : AST::Scope)
+      return unless block_arg = node.block_arg
+      return unless block_arg = scope.arguments.find(&.node.== block_arg)
+
+      return if block_arg.anonymous?
+      return if scope.references?(block_arg.variable)
+
+      if scope.yields?
+        issue_for block_arg.node, MSG_YIELDED
+      else
+        return if block_arg.ignored?
+        issue_for block_arg.node, MSG_UNUSED % block_arg.name
+      end
+    end
+  end
+end

src/ameba/rule/style/type_names.cr

@@ -58,16 +58,12 @@ module Ameba::Rule::Style
 
     MSG = "Type name should be camelcased: %s, but it was %s"
 
-    private def check_node(source, node)
+    def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef)
       name = node.name.to_s
       expected = name.camelcase
       return if name == expected
 
       issue_for node, MSG % {expected, name}
     end
-
-    def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef)
-      check_node(source, node)
-    end
   end
 end

src/ameba/rule/style/variable_names.cr

@@ -29,18 +29,16 @@ module Ameba::Rule::Style
 
     MSG = "Var name should be underscore-cased: %s, not %s"
 
-    private def check_node(source, node)
-      return if (expected = node.name.underscore) == node.name
-
-      issue_for node, MSG % {expected, node.name}
-    end
-
     def test(source : Source)
       VarVisitor.new self, source
     end
 
     def test(source, node : Crystal::Var | Crystal::InstanceVar | Crystal::ClassVar)
-      check_node source, node
+      name = node.name.to_s
+      expected = name.underscore
+      return if name == expected
+
+      issue_for node, MSG % {expected, name}
     end
 
     private class VarVisitor < AST::NodeVisitor