From fa6a095e86c31faa5cb3219b515881f95f8e3f0b Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Fri, 30 Sep 2016 20:42:38 +0800 Subject: [PATCH] [Fix #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 | 1 + lib/rubocop/cop/lint/unused_block_argument.rb | 107 +++++++++++++----- .../cop/lint/unused_block_argument_spec.rb | 76 ++++++++++--- 3 files changed, 143 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a583cd6c5eab..1add5ffa1009 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/rubocop/cop/lint/unused_block_argument.rb b/lib/rubocop/cop/lint/unused_block_argument.rb index 58bdae359c36..e4cf494db163 100644 --- a/lib/rubocop/cop/lint/unused_block_argument.rb +++ b/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 diff --git a/spec/rubocop/cop/lint/unused_block_argument_spec.rb b/spec/rubocop/cop/lint/unused_block_argument_spec.rb index 39c3e477fe7b..a5d8586af1c9 100644 --- a/spec/rubocop/cop/lint/unused_block_argument_spec.rb +++ b/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