[Fix rubocop#3499] Make Lint/UnusedBlockArgument aware of `#define_…

…method`

This cop would make recommendations that, if followed, would change
the arity of methods defined using `#define_method`. This change
fixes that.

CHANGELOG.md

@@ -16,6 +16,7 @@
 * [#3436](https://github.com/bbatsov/rubocop/issues/3436): Make `Rails/SaveBang` cop not register an offense when return value of a non-bang method is returned by the parent method. ([@coorasse][])
 * [#3540](https://github.com/bbatsov/rubocop/issues/3540): Fix `Style/GuardClause` to register offense for instance and singleton methods. ([@tejasbubane][])
 * [#3311](https://github.com/bbatsov/rubocop/issues/3311): Detect incompatibilities with the external encoding to prevent bad autocorrections in `Style/StringLiterals`. ([@deivid-rodriguez][])
+* [#3499](https://github.com/bbatsov/rubocop/issues/3499): Ensure `Lint/UnusedBlockArgument` doesn't make recommendations that would change arity for methods defined using `#define_method`. ([@drenmi][])
 
 ### Changes
 

lib/rubocop/cop/lint/unused_block_argument.rb

@@ -7,43 +7,76 @@ module Lint
       #
       # @example
       #
-      #   do_something do |used, unused, _unused_but_allowed|
+      #   #good
+      #
+      #   do_something do |used, unused|
+      #     puts used
+      #   end
+      #
+      #   do_something do
+      #     puts :foo
+      #   end
+      #
+      #   define_method(:foo) do |_bar|
+      #     puts :baz
+      #   end
+      #
+      #   # bad
+      #
+      #   do_something do |used, _unused|
       #     puts used
       #   end
+      #
+      #   do_something do |bar|
+      #     puts :foo
+      #   end
+      #
+      #   define_method(:foo) do |bar|
+      #     puts :baz
+      #   end
       class UnusedBlockArgument < Cop
         include UnusedArgument
 
-        def check_argument(variable)
-          return unless variable.block_argument?
-          return if variable.keyword_argument? &&
-                    cop_config && cop_config['AllowUnusedKeywordArguments']
+        private
 
-          if cop_config['IgnoreEmptyBlocks']
-            _send, _args, body = *variable.scope.node
-            return if body.nil?
-          end
+        def check_argument(variable)
+          return if allowed_block?(variable) ||
+                    allowed_keyword_argument?(variable)
 
           super
         end
 
-        def message(variable)
-          message = String.new("Unused #{variable_type(variable)} - " \
-                               "`#{variable.name}`.")
+        def allowed_block?(variable)
+          !variable.block_argument? ||
+            (ignore_empty_blocks? && empty_block?(variable))
+        end
 
-          return message if variable.explicit_block_local_variable?
+        def allowed_keyword_argument?(variable)
+          variable.keyword_argument? &&
+            allow_unused_keyword_arguments?
+        end
 
-          message << ' '
+        def message(variable)
+          message = "Unused #{variable_type(variable)} - `#{variable.name}`."
 
+          if variable.explicit_block_local_variable?
+            message
+          else
+            augment_message(message, variable)
+          end
+        end
+
+        def augment_message(message, variable)
           scope = variable.scope
           all_arguments = scope.variables.each_value.select(&:block_argument?)
 
-          message << if scope.node.lambda?
-                       message_for_lambda(variable, all_arguments)
-                     else
-                       message_for_normal_block(variable, all_arguments)
-                     end
+          augmentation = if scope.node.lambda?
+                           message_for_lambda(variable, all_arguments)
+                         else
+                           message_for_normal_block(variable, all_arguments)
+                         end
 
-          message
+          [message, augmentation].join(' ')
         end
 
         def variable_type(variable)
@@ -55,7 +88,8 @@ def variable_type(variable)
         end
 
         def message_for_normal_block(variable, all_arguments)
-          if all_arguments.none?(&:referenced?)
+          if all_arguments.none?(&:referenced?) &&
+             !define_method_call?(variable)
             if all_arguments.count > 1
               "You can omit all the arguments if you don't care about them."
             else
@@ -67,21 +101,42 @@ def message_for_normal_block(variable, all_arguments)
         end
 
         def message_for_lambda(variable, all_arguments)
-          message = String.new(message_for_underscore_prefix(variable))
+          message = message_for_underscore_prefix(variable)
 
           if all_arguments.none?(&:referenced?)
-            message << ' Also consider using a proc without arguments ' \
-                       'instead of a lambda if you want it ' \
-                        "to accept any arguments but don't care about them."
+            proc_message = 'Also consider using a proc without arguments ' \
+                           'instead of a lambda if you want it ' \
+                           "to accept any arguments but don't care about them."
           end
 
-          message
+          [message, proc_message].compact.join(' ')
         end
 
         def message_for_underscore_prefix(variable)
           "If it's necessary, use `_` or `_#{variable.name}` " \
           "as an argument name to indicate that it won't be used."
         end
+
+        def define_method_call?(variable)
+          call, = *variable.scope.node
+          _, method, = *call
+
+          method == :define_method
+        end
+
+        def empty_block?(variable)
+          _send, _args, body = *variable.scope.node
+
+          body.nil?
+        end
+
+        def allow_unused_keyword_arguments?
+          cop_config['AllowUnusedKeywordArguments']
+        end
+
+        def ignore_empty_blocks?
+          cop_config['IgnoreEmptyBlocks']
+        end
       end
     end
   end

spec/rubocop/cop/lint/unused_block_argument_spec.rb

@@ -71,6 +71,24 @@
           expect(cop.highlights).to eq(['index'])
         end
       end
+
+      context 'and the method call is `define_method`' do
+        let(:source) { <<-END }
+          define_method(:foo) do |bar|
+            puts 'baz'
+          end
+        END
+
+        it 'registers an offense' do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.first.message).to eq(
+            'Unused block argument - `bar`. ' \
+            "If it's necessary, use `_` or `_bar` as an argument name " \
+            "to indicate that it won't be used."
+          )
+          expect(cop.highlights).to eq(['bar'])
+        end
+      end
     end
 
     context 'when a block have a block local variable' do
@@ -145,26 +163,54 @@
     end
 
     context 'when an optional keyword argument is unused', ruby: 2 do
-      let(:source) { <<-END }
-        define_method(:foo) do |bar: 'default'|
-          puts 'bar'
+      context 'when the method call is `define_method`' do
+        let(:source) { <<-END }
+          define_method(:foo) do |bar: 'default'|
+            puts 'bar'
+          end
+        END
+
+        it 'registers an offense' do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.first.message).to eq(
+            'Unused block argument - `bar`. ' \
+            "If it's necessary, use `_` or `_bar` as an argument name " \
+            "to indicate that it won't be used."
+          )
+          expect(cop.highlights).to eq(['bar'])
         end
-      END
 
-      it 'registers an offense but does not suggest underscore-prefix' do
-        expect(cop.offenses.size).to eq(1)
-        expect(cop.highlights).to eq(['bar'])
-        expect(cop.offenses.first.message).to eq(
-          'Unused block argument - `bar`. ' \
-          "You can omit the argument if you don't care about it."
-        )
+        context 'when AllowUnusedKeywordArguments set' do
+          let(:cop_config) { { 'AllowUnusedKeywordArguments' => true } }
+
+          it 'does not care' do
+            expect(cop.offenses).to be_empty
+          end
+        end
       end
 
-      context 'and AllowUnusedKeywordArguments set' do
-        let(:cop_config) { { 'AllowUnusedKeywordArguments' => true } }
+      context 'when the method call is not `define_method`' do
+        let(:source) { <<-END }
+          foo(:foo) do |bar: 'default'|
+            puts 'bar'
+          end
+        END
 
-        it 'does not care' do
-          expect(cop.offenses).to be_empty
+        it 'registers an offense' do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.first.message).to eq(
+            'Unused block argument - `bar`. ' \
+            "You can omit the argument if you don't care about it."
+          )
+          expect(cop.highlights).to eq(['bar'])
+        end
+
+        context 'when AllowUnusedKeywordArguments set' do
+          let(:cop_config) { { 'AllowUnusedKeywordArguments' => true } }
+
+          it 'does not care' do
+            expect(cop.offenses).to be_empty
+          end
         end
       end
     end