Skip to content

Commit

Permalink
[Fix #12296] Support Redundant*Names options for `Style/ArgumentsFo…
Browse files Browse the repository at this point in the history
…rwarding`

Fixes #12296.

This PR supports `RedundantRestArgumentNames`, `RedundantKeywordRestArgumentNames`,
and `RedundantBlockArgumentNames` options for `Style/ArgumentsForwarding`.

Meaningless names that are commonly used can be anonymized by default as part of the new `Redundant*Names` defaults.
e.g., `*args`, `**options`, `&block`, and some argument names.

Names not on this list are likely to be meaningful and are allowed by default.

This approach is designed to only detect generally meaningless names without the need for users to edit the configuration
for each meaningful name, with the expectation that the default settings will work well for most cases.
  • Loading branch information
koic authored and bbatsov committed Nov 20, 2023
1 parent dc4cbf0 commit 10c8d32
Show file tree
Hide file tree
Showing 4 changed files with 220 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#12296](https://github.com/rubocop/rubocop/issues/12296): Support `RedundantRestArgumentNames`, `RedundantKeywordRestArgumentNames`, and `RedundantBlockArgumentNames` options for `Style/ArgumentsForwarding`. ([@koic][])
12 changes: 12 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3089,7 +3089,19 @@ Style/ArgumentsForwarding:
Enabled: pending
AllowOnlyRestArgument: true
UseAnonymousForwarding: true
RedundantRestArgumentNames:
- args
- arguments
RedundantKeywordRestArgumentNames:
- kwargs
- options
- opts
RedundantBlockArgumentNames:
- blk
- block
- proc
VersionAdded: '1.1'
VersionChanged: '<<next>>'

Style/ArrayCoercion:
Description: >-
Expand Down
70 changes: 64 additions & 6 deletions lib/rubocop/cop/style/arguments_forwarding.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,16 @@ module Style
#
# This cop also identifies places where `use_args(*args)`/`use_kwargs(**kwargs)` can be
# replaced by `use_args(*)`/`use_kwargs(**)`; if desired, this functionality can be disabled
# by setting UseAnonymousForwarding: false.
# by setting `UseAnonymousForwarding: false`.
#
# And this cop has `RedundantRestArgumentNames`, `RedundantKeywordRestArgumentNames`,
# and `RedundantBlockArgumentNames` options. This configuration is a list of redundant names
# that are sufficient for anonymizing meaningless naming.
#
# Meaningless names that are commonly used can be anonymized by default:
# e.g., `*args`, `**options`, `&block`, and so on.
#
# Names not on this list are likely to be meaningful and are allowed by default.
#
# @example
# # bad
Expand Down Expand Up @@ -72,6 +81,38 @@ module Style
# bar(**kwargs)
# end
#
# @example RedundantRestArgumentNames: ['args', 'arguments'] (default)
# # bad
# def foo(*args)
# bar(*args)
# end
#
# # good
# def foo(*)
# bar(*)
# end
#
# @example RedundantKeywordRestArgumentNames: ['kwargs', 'options', 'opts'] (default)
# # bad
# def foo(**kwargs)
# bar(**kwargs)
# end
#
# # good
# def foo(**)
# bar(**)
# end
#
# @example RedundantBlockArgumentNames: ['blk', 'block', 'proc'] (default)
# # bad
# def foo(&block)
# bar(&block)
# end
#
# # good
# def foo(&)
# bar(&)
# end
class ArgumentsForwarding < Base
include RangeHelp
extend AutoCorrector
Expand All @@ -93,13 +134,12 @@ def self.autocorrect_incompatible_with
def on_def(node)
return unless node.body

forwardable_args = extract_forwardable_args(node.arguments)
restarg, kwrestarg, blockarg = extract_forwardable_args(node.arguments)
forwardable_args = redundant_forwardable_named_args(restarg, kwrestarg, blockarg)
send_nodes = node.each_descendant(:send).to_a

send_classifications = classify_send_nodes(
node,
node.each_descendant(:send).to_a,
non_splat_or_block_pass_lvar_references(node.body),
forwardable_args
node, send_nodes, non_splat_or_block_pass_lvar_references(node.body), forwardable_args
)

return if send_classifications.empty?
Expand All @@ -119,6 +159,14 @@ def extract_forwardable_args(args)
[args.find(&:restarg_type?), args.find(&:kwrestarg_type?), args.find(&:blockarg_type?)]
end

def redundant_forwardable_named_args(restarg, kwrestarg, blockarg)
restarg_node = redundant_named_arg(restarg, 'RedundantRestArgumentNames', '*')
kwrestarg_node = redundant_named_arg(kwrestarg, 'RedundantKeywordRestArgumentNames', '**')
blockarg_node = redundant_named_arg(blockarg, 'RedundantBlockArgumentNames', '&')

[restarg_node, kwrestarg_node, blockarg_node]
end

def only_forwards_all?(send_classifications)
send_classifications.all? { |_, c, _, _| c == :all }
end
Expand Down Expand Up @@ -192,6 +240,16 @@ def classification_and_forwards(def_node, send_node, referenced_lvars, forwardab
[classification, classifier.forwarded_rest_arg, classifier.forwarded_kwrest_arg]
end

def redundant_named_arg(arg, config_name, keyword)
return nil unless arg

redundant_arg_names = cop_config.fetch(config_name, []).map do |redundant_arg_name|
"#{keyword}#{redundant_arg_name}"
end << keyword

redundant_arg_names.include?(arg.source) ? arg : nil
end

def register_forward_args_offense(def_arguments_or_send, rest_arg_or_splat)
add_offense(rest_arg_or_splat, message: ARGS_MSG) do |corrector|
add_parens_if_missing(def_arguments_or_send, corrector)
Expand Down
143 changes: 143 additions & 0 deletions spec/rubocop/cop/style/arguments_forwarding_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::ArgumentsForwarding, :config do
let(:cop_config) do
{
'RedundantRestArgumentNames' => redundant_rest_argument_names,
'RedundantKeywordRestArgumentNames' => redundant_keyword_rest_argument_names,
'RedundantBlockArgumentNames' => redundant_block_argument_names
}
end
let(:redundant_rest_argument_names) { %w[args arguments] }
let(:redundant_keyword_rest_argument_names) { %w[kwargs options opts] }
let(:redundant_block_argument_names) { %w[blk block proc] }

context 'TargetRubyVersion <= 2.6', :ruby26 do
it 'does not register an offense when using restarg with block arg' do
expect_no_offenses(<<~RUBY)
Expand Down Expand Up @@ -162,6 +173,34 @@ def foo(...)
RUBY
end

context 'when `RedundantBlockArgumentNames: [meaningless_block_name]`' do
let(:redundant_block_argument_names) { ['meaningless_block_name'] }

it 'registers an offense when using restarg and block arg' do
expect_offense(<<~RUBY)
def foo(*args, &meaningless_block_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use shorthand syntax `...` for arguments forwarding.
bar(*args, &meaningless_block_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use shorthand syntax `...` for arguments forwarding.
end
RUBY

expect_correction(<<~RUBY)
def foo(...)
bar(...)
end
RUBY
end

it 'does not register an offense when using restarg and unconfigured block arg' do
expect_no_offenses(<<~RUBY)
def foo(*args, &block)
bar(*args, &block)
end
RUBY
end
end

it 'does not register an offense when using arguments forwarding' do
expect_no_offenses(<<~RUBY)
def foo(...)
Expand Down Expand Up @@ -669,6 +708,26 @@ def foo(x, ...)
end
RUBY
end

context 'AllowOnlyRestArgument: false' do
let(:cop_config) { { 'AllowOnlyRestArgument' => false } }

it 'registers an offense when using only rest arg' do
expect_offense(<<~RUBY)
def foo(*args)
^^^^^ Use shorthand syntax `...` for arguments forwarding.
bar(*args)
^^^^^ Use shorthand syntax `...` for arguments forwarding.
end
RUBY

expect_correction(<<~RUBY)
def foo(...)
bar(...)
end
RUBY
end
end
end

context 'TargetRubyVersion >= 3.1', :ruby31 do
Expand Down Expand Up @@ -1615,5 +1674,89 @@ def foo(...)
RUBY
end
end

context 'when `RedundantRestArgumentNames: [meaningless_restarg_name]`' do
let(:redundant_rest_argument_names) { ['meaningless_restarg_name'] }

it 'registers an offense when using separate arg/kwarg forwarding' do
expect_offense(<<~RUBY)
def foo(*meaningless_restarg_name, **options)
^^^^^^^^^^^^^^^^^^^^^^^^^ Use anonymous positional arguments forwarding (`*`).
^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
args_only(*meaningless_restarg_name)
^^^^^^^^^^^^^^^^^^^^^^^^^ Use anonymous positional arguments forwarding (`*`).
kwargs_only(**options)
^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
end
RUBY

expect_correction(<<~RUBY)
def foo(*, **)
args_only(*)
kwargs_only(**)
end
RUBY
end

it 'registers an offense when using separate unconfigured arg/kwarg forwarding' do
expect_offense(<<~RUBY)
def foo(*args, **options)
^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
args_only(*args)
kwargs_only(**options)
^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
end
RUBY

expect_correction(<<~RUBY)
def foo(*args, **)
args_only(*args)
kwargs_only(**)
end
RUBY
end
end

context 'when `RedundantKeywordRestArgumentNames: [meaningless_kwrestarg_name]`' do
let(:redundant_keyword_rest_argument_names) { ['meaningless_kwrestarg_name'] }

it 'registers an offense when using separate arg/kwarg forwarding' do
expect_offense(<<~RUBY)
def foo(*args, **meaningless_kwrestarg_name)
^^^^^ Use anonymous positional arguments forwarding (`*`).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
args_only(*args)
^^^^^ Use anonymous positional arguments forwarding (`*`).
kwargs_only(**meaningless_kwrestarg_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use anonymous keyword arguments forwarding (`**`).
end
RUBY

expect_correction(<<~RUBY)
def foo(*, **)
args_only(*)
kwargs_only(**)
end
RUBY
end

it 'registers an offense when using separate arg/unconfigured kwarg forwarding' do
expect_offense(<<~RUBY)
def foo(*args, **options)
^^^^^ Use anonymous positional arguments forwarding (`*`).
args_only(*args)
^^^^^ Use anonymous positional arguments forwarding (`*`).
kwargs_only(**options)
end
RUBY

expect_correction(<<~RUBY)
def foo(*, **options)
args_only(*)
kwargs_only(**options)
end
RUBY
end
end
end
end

0 comments on commit 10c8d32

Please sign in to comment.