Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #7549] Add Style/ArgumentsForwarding cop #7646

Merged
merged 1 commit into from Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
### New features

* [#8895](https://github.com/rubocop-hq/rubocop/pull/8895): Add new `Lint/EmptyBlock` cop. ([@fatkodima][])
* [#7549](https://github.com/rubocop-hq/rubocop/issues/7549): Add new `Style/ArgumentsForwarding` cop. ([@koic][])

## 1.0.0 (2020-10-21)

Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Expand Up @@ -2484,6 +2484,13 @@ Style/AndOr:
- always
- conditionals

Style/ArgumentsForwarding:
Description: 'Use arguments forwarding.'
StyleGuide: '#arguments-forwarding'
Enabled: pending
AllowOnlyRestArgument: true
VersionAdded: '1.1'

Style/ArrayCoercion:
Description: >-
Use Array() instead of explicit Array check or [*var], when dealing
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -337,6 +337,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#styleaccessorgrouping[Style/AccessorGrouping]
* xref:cops_style.adoc#stylealias[Style/Alias]
* xref:cops_style.adoc#styleandor[Style/AndOr]
* xref:cops_style.adoc#styleargumentsforwarding[Style/ArgumentsForwarding]
* xref:cops_style.adoc#stylearraycoercion[Style/ArrayCoercion]
* xref:cops_style.adoc#stylearrayjoin[Style/ArrayJoin]
* xref:cops_style.adoc#styleasciicomments[Style/AsciiComments]
Expand Down
75 changes: 75 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -293,6 +293,81 @@ end

* https://rubystyle.guide#no-and-or-or

== Style/ArgumentsForwarding

NOTE: Required Ruby version: 2.7

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Pending
| Yes
| Yes
| 1.1
| -
|===

In Ruby 2.7, arguments forwarding has been added.

This cop identifies places where `do_something(*args, &block)`
can be replaced by `do_something(...)`.

=== Examples

[source,ruby]
----
# bad
def foo(*args, &block)
bar(*args, &block)
end

# bad
def foo(*args, **kwargs, &block)
bar(*args, **kwargs, &block)
end

# good
def foo(...)
bar(...)
end
----

==== AllowOnlyRestArgument: true (default)

[source,ruby]
----
# good
def foo(*args)
bar(*args)
end
----

==== AllowOnlyRestArgument: false

[source,ruby]
----
# bad
# The following code can replace the arguments with `...`,
# but it will change the behavior. Because `...` forwards block also.
def foo(*args)
bar(*args)
end
----

=== Configurable attributes

|===
| Name | Default value | Configurable values

| AllowOnlyRestArgument
| `true`
| Boolean
|===

=== References

* https://rubystyle.guide#arguments-forwarding

== Style/ArrayCoercion

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -385,6 +385,7 @@
require_relative 'rubocop/cop/style/accessor_grouping'
require_relative 'rubocop/cop/style/alias'
require_relative 'rubocop/cop/style/and_or'
require_relative 'rubocop/cop/style/arguments_forwarding'
require_relative 'rubocop/cop/style/array_coercion'
require_relative 'rubocop/cop/style/array_join'
require_relative 'rubocop/cop/style/ascii_comments'
Expand Down
142 changes: 142 additions & 0 deletions lib/rubocop/cop/style/arguments_forwarding.rb
@@ -0,0 +1,142 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# In Ruby 2.7, arguments forwarding has been added.
#
# This cop identifies places where `do_something(*args, &block)`
# can be replaced by `do_something(...)`.
#
# @example
# # bad
# def foo(*args, &block)
# bar(*args, &block)
# end
#
# # bad
# def foo(*args, **kwargs, &block)
# bar(*args, **kwargs, &block)
# end
#
# # good
# def foo(...)
# bar(...)
# end
#
# @example AllowOnlyRestArgument: true (default)
# # good
# def foo(*args)
# bar(*args)
# end
#
# @example AllowOnlyRestArgument: false
# # bad
# # The following code can replace the arguments with `...`,
# # but it will change the behavior. Because `...` forwards block also.
# def foo(*args)
# bar(*args)
# end
#
class ArgumentsForwarding < Base
include RangeHelp
extend AutoCorrector
extend TargetRubyVersion

minimum_target_ruby_version 2.7

MSG = 'Use arguments forwarding.'

def_node_matcher :use_rest_arguments?, <<~PATTERN
(args (restarg $_) $...)
PATTERN

def_node_matcher :only_rest_arguments?, <<~PATTERN
(send _ _ (splat (lvar %1)))
PATTERN

def_node_matcher :forwarding_method_arguments?, <<~PATTERN
{
(send _ _
(splat (lvar %1))
(block-pass (lvar %2)))
(send _ _
(splat (lvar %1))
(hash (kwsplat (lvar %3)))
(block-pass (lvar %2)))
}
PATTERN

def on_def(node)
return unless node.body
return unless (rest_args_name, args = use_rest_arguments?(node.arguments))

node.each_descendant(:send) do |send_node|
kwargs_name, block_name = extract_argument_names_from(args)

next unless forwarding_method?(send_node, rest_args_name, kwargs_name, block_name) &&
all_lvars_as_forwarding_method_arguments?(node, send_node)

register_offense_to_forwarding_method_arguments(send_node)
register_offense_to_method_definition_arguments(node)
end
end
alias on_defs on_def

private

def extract_argument_names_from(args)
kwargs_name = args.first.source.delete('**') if args.first&.kwrestarg_type?
block_arg_name = args.last.source.delete('&') if args.last&.blockarg_type?

[kwargs_name, block_arg_name].map { |name| name&.to_sym }
end

def forwarding_method?(node, rest_arg, kwargs, block_arg)
return only_rest_arguments?(node, rest_arg) unless allow_only_rest_arguments?

forwarding_method_arguments?(node, rest_arg, block_arg, kwargs)
end

def all_lvars_as_forwarding_method_arguments?(def_node, forwarding_method)
lvars = def_node.body.each_descendant(:lvar, :lvasgn)

begin_pos = forwarding_method.source_range.begin_pos
end_pos = forwarding_method.source_range.end_pos

lvars.all? do |lvar|
lvar.source_range.begin_pos.between?(begin_pos, end_pos)
end
end

def register_offense_to_forwarding_method_arguments(forwarding_method)
add_offense(arguments_range(forwarding_method)) do |corrector|
range = range_between(
forwarding_method.loc.selector.end_pos, forwarding_method.source_range.end_pos
)
corrector.replace(range, '(...)')
end
end

def register_offense_to_method_definition_arguments(method_definition)
add_offense(arguments_range(method_definition)) do |corrector|
arguments_range = range_with_surrounding_space(
range: method_definition.arguments.source_range, side: :left
)
corrector.replace(arguments_range, '(...)')
end
end

def arguments_range(node)
arguments = node.arguments

range_between(arguments.first.source_range.begin_pos, arguments.last.source_range.end_pos)
end

def allow_only_rest_arguments?
cop_config.fetch('AllowOnlyRestArgument', true)
end
end
end
end
end