Merge pull request #173 from fatkodima/block_given_with_explicit_block

Add new `Performance/BlockGivenWithExplicitBlock` cop

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## master (unreleased)
 
+### New features
+
+* [#173](https://github.com/rubocop-hq/rubocop-performance/pull/173): Add new `Performance/BlockGivenWithExplicitBlock` cop. ([@fatkodima][])
+
 ### Changes
 
 * [#181](https://github.com/rubocop-hq/rubocop-performance/pull/181): Change default configuration for `Performance/CollectionLiteralInLoop` to `Enabled: 'pending'`. ([@ghiculescu][])

config/default.yml

@@ -17,6 +17,11 @@ Performance/BindCall:
   Enabled: true
   VersionAdded: '1.6'
 
+Performance/BlockGivenWithExplicitBlock:
+  Description: 'Check block argument explicitly instead of using `block_given?`.'
+  Enabled: pending
+  VersionAdded: '1.9'
+
 Performance/Caller:
   Description: >-
              Use `caller(n..n)` instead of `caller`.

docs/modules/ROOT/pages/cops.adoc

@@ -15,6 +15,7 @@ Performance cops optimization analysis for your projects.
 * xref:cops_performance.adoc#performanceancestorsinclude[Performance/AncestorsInclude]
 * xref:cops_performance.adoc#performancebigdecimalwithnumericargument[Performance/BigDecimalWithNumericArgument]
 * xref:cops_performance.adoc#performancebindcall[Performance/BindCall]
+* xref:cops_performance.adoc#performanceblockgivenwithexplicitblock[Performance/BlockGivenWithExplicitBlock]
 * xref:cops_performance.adoc#performancecaller[Performance/Caller]
 * xref:cops_performance.adoc#performancecasewhensplat[Performance/CaseWhenSplat]
 * xref:cops_performance.adoc#performancecasecmp[Performance/Casecmp]

docs/modules/ROOT/pages/cops_performance.adoc

@@ -93,6 +93,43 @@ umethod.bind(obj).(foo, bar)
 umethod.bind_call(obj, foo, bar)
 ----
 
+== Performance/BlockGivenWithExplicitBlock
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 1.9
+| -
+|===
+
+This cop identifies unnecessary use of a `block_given?` where explicit check
+of block argument would suffice.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+def method(&block)
+  do_something if block_given?
+end
+
+# good
+def method(&block)
+  do_something if block
+end
+
+# good - block is reassigned
+def method(&block)
+  block ||= -> { do_something }
+  warn "Using default ..." unless block_given?
+  # ...
+end
+----
+
 == Performance/Caller
 
 |===

lib/rubocop/cop/performance/block_given_with_explicit_block.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Performance
+      # This cop identifies unnecessary use of a `block_given?` where explicit check
+      # of block argument would suffice.
+      #
+      # @example
+      #   # bad
+      #   def method(&block)
+      #     do_something if block_given?
+      #   end
+      #
+      #   # good
+      #   def method(&block)
+      #     do_something if block
+      #   end
+      #
+      #   # good - block is reassigned
+      #   def method(&block)
+      #     block ||= -> { do_something }
+      #     warn "Using default ..." unless block_given?
+      #     # ...
+      #   end
+      #
+      class BlockGivenWithExplicitBlock < Base
+        extend AutoCorrector
+
+        RESTRICT_ON_SEND = %i[block_given?].freeze
+        MSG = 'Check block argument explicitly instead of using `block_given?`.'
+
+        def_node_matcher :reassigns_block_arg?, '`(lvasgn %1 ...)'
+
+        def on_send(node)
+          def_node = node.each_ancestor(:def, :defs).first
+          return unless def_node
+
+          block_arg = def_node.arguments.find(&:blockarg_type?)
+          return unless block_arg
+
+          block_arg_name = block_arg.loc.name.source.to_sym
+          return if reassigns_block_arg?(def_node, block_arg_name)
+
+          add_offense(node) do |corrector|
+            corrector.replace(node, block_arg_name)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/performance_cops.rb

@@ -6,6 +6,7 @@
 require_relative 'performance/ancestors_include'
 require_relative 'performance/big_decimal_with_numeric_argument'
 require_relative 'performance/bind_call'
+require_relative 'performance/block_given_with_explicit_block'
 require_relative 'performance/caller'
 require_relative 'performance/case_when_splat'
 require_relative 'performance/casecmp'

spec/rubocop/cop/performance/block_given_with_explicit_block_spec.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Performance::BlockGivenWithExplicitBlock do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense and corrects when using `block_given?` in an instance method with block arg' do
+    expect_offense(<<~RUBY)
+      def method(x, &block)
+        do_something if block_given?
+                        ^^^^^^^^^^^^ Check block argument explicitly instead of using `block_given?`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      def method(x, &block)
+        do_something if block
+      end
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `block_given?` in a class method with block arg' do
+    expect_offense(<<~RUBY)
+      def self.method(x, &block)
+        do_something if block_given?
+                        ^^^^^^^^^^^^ Check block argument explicitly instead of using `block_given?`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      def self.method(x, &block)
+        do_something if block
+      end
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `block_given?` in a method with non standard block arg name' do
+    expect_offense(<<~RUBY)
+      def method(x, &myblock)
+        do_something if block_given?
+                        ^^^^^^^^^^^^ Check block argument explicitly instead of using `block_given?`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      def method(x, &myblock)
+        do_something if myblock
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when using `block_given?` in a method without block arg' do
+    expect_no_offenses(<<~RUBY)
+      def method(x)
+        do_something if block_given?
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when block arg is reassigned' do
+    expect_no_offenses(<<~RUBY)
+      def method(a, &block)
+        block ||= -> {}
+        do_something if block_given?
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when `block_given?` is used outside of a method' do
+    expect_no_offenses(<<~RUBY)
+      do_something if block_given?
+    RUBY
+  end
+end