From 5f75baccd5f270593ab1bc0dbff3035cbafa3df2 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 11 Jan 2020 14:42:01 +0900 Subject: [PATCH] [Fix #7549] Add `Style/ArgumentsForwarding` cop Closes #7549. This PR adds `Style/ArgumentsForwarding` cop. As shown at #7549, the following code is unsafe to be arguments forwarding. ```ruby def foo(*args) bar(*args) end ``` So by setting `AllowOnlyRestArgument` configuration option to true by default, This PR makes the cop safe by default. --- CHANGELOG.md | 1 + config/default.yml | 7 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_style.adoc | 75 ++++++ lib/rubocop.rb | 1 + lib/rubocop/cop/style/arguments_forwarding.rb | 142 +++++++++++ .../cop/style/arguments_forwarding_spec.rb | 231 ++++++++++++++++++ 7 files changed, 458 insertions(+) create mode 100644 lib/rubocop/cop/style/arguments_forwarding.rb create mode 100644 spec/rubocop/cop/style/arguments_forwarding_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 1abf666ea36..d302331b009 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/config/default.yml b/config/default.yml index 0a20668e99b..c4b197e88a2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index a32e3621b4f..962acf8a32a 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -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] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index c4197e84415..f28dc3807c9 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index f0b428fba20..beade3dc188 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' diff --git a/lib/rubocop/cop/style/arguments_forwarding.rb b/lib/rubocop/cop/style/arguments_forwarding.rb new file mode 100644 index 00000000000..7ae6083fd94 --- /dev/null +++ b/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 diff --git a/spec/rubocop/cop/style/arguments_forwarding_spec.rb b/spec/rubocop/cop/style/arguments_forwarding_spec.rb new file mode 100644 index 00000000000..9d997eada04 --- /dev/null +++ b/spec/rubocop/cop/style/arguments_forwarding_spec.rb @@ -0,0 +1,231 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::ArgumentsForwarding, :config do + subject(:cop) { described_class.new(config) } + + context 'TargetRubyVersion <= 2.6', :ruby26 do + it 'does not registers an offense when using restarg with block arg' do + expect_no_offenses(<<~RUBY) + def foo(*args, &block) + bar(*args, &block) + end + RUBY + end + end + + context 'TargetRubyVersion >= 2.7', :ruby27 do + it 'registers an offense when using restarg and block arg' do + expect_offense(<<~RUBY) + def foo(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + bar(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + end + RUBY + + expect_correction(<<~RUBY) + def foo(...) + bar(...) + end + RUBY + end + + it 'registers an offense when using restarg, kwargs and block arg' do + expect_offense(<<~RUBY) + def foo(*args, **kwargs, &block) + ^^^^^^^^^^^^^^^^^^^^^^^ Use arguments forwarding. + bar(*args, **kwargs, &block) + ^^^^^^^^^^^^^^^^^^^^^^^ Use arguments forwarding. + end + RUBY + + expect_correction(<<~RUBY) + def foo(...) + bar(...) + end + RUBY + end + + it 'registers an offense when passing restarg and block arg in defs' do + expect_offense(<<~RUBY) + def self.foo(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + bar(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + end + RUBY + + expect_correction(<<~RUBY) + def self.foo(...) + bar(...) + end + RUBY + end + + it 'registers an offense when the parentheses of arguments are omitted' do + expect_offense(<<~RUBY) + def foo *args, &block + ^^^^^^^^^^^^^ Use arguments forwarding. + bar *args, &block + ^^^^^^^^^^^^^ Use arguments forwarding. + end + RUBY + + # A method definition that uses forwarding arguments without parentheses + # is a syntax error. e.g. `def do_something ...` + # Therefore it enforces parentheses with auto-correction. + expect_correction(<<~RUBY) + def foo(...) + bar(...) + end + RUBY + end + + it 'registers an offense when forwarding to a method in block' do + expect_offense(<<~RUBY) + def foo(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + do_something do + bar(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + end + end + RUBY + + expect_correction(<<~RUBY) + def foo(...) + do_something do + bar(...) + end + end + RUBY + end + + it 'registers an offense when delegating' do + expect_offense(<<~RUBY) + def foo(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + obj.bar(*args, &block) + ^^^^^^^^^^^^^ Use arguments forwarding. + end + RUBY + end + + it 'does not register an offense when using arguments forwarding' do + expect_no_offenses(<<~RUBY) + def foo(...) + bar(...) + end + RUBY + end + + it 'does not register an offense when different arguments are used' do + expect_no_offenses(<<~RUBY) + def foo(*args, &block) + bar(*args) + end + RUBY + end + + it 'does not register an offense when different argument names are used' do + expect_no_offenses(<<~RUBY) + def foo(*args, &block) + bar(*arguments, &block) + end + RUBY + end + + it 'does not register an offense when the restarg is overwritten' do + expect_no_offenses(<<~RUBY) + def foo(*args, **kwargs, &block) + args = new_args + bar(*args, **kwargs, &block) + end + RUBY + end + + it 'does not register an offense when the kwarg is overwritten' do + expect_no_offenses(<<~RUBY) + def foo(*args, **kwargs, &block) + kwargs = new_kwargs + bar(*args, **kwargs, &block) + end + RUBY + end + + it 'does not register an offense when the block arg is overwritten' do + expect_no_offenses(<<~RUBY) + def foo(*args, **kwargs, &block) + block = new_block + bar(*args, **kwargs, &block) + end + RUBY + end + + it 'does not register an offense when using the restarg outside forwarding method arguments' do + expect_no_offenses(<<~RUBY) + def foo(*args, **kwargs, &block) + args.do_something + bar(*args, **kwargs, &block) + end + RUBY + end + + it 'does not register an offense when assigning the restarg outside forwarding method arguments' do + expect_no_offenses(<<~'RUBY') + def foo(*args, &block) + var = args + foo(*args, &block) + end + RUBY + end + + it 'does not register an offense when referencing the restarg outside forwarding method arguments' do + expect_no_offenses(<<~RUBY) + def foo(*args, &block) + args + foo(*args, &block) + end + RUBY + end + + it 'does not register an offense when body of method definition is empty' do + expect_no_offenses(<<~'RUBY') + def foo(*args, &block) + end + RUBY + end + + context 'AllowOnlyRestArgument: true' do + let(:cop_config) { { 'AllowOnlyRestArgument' => true } } + + it 'does not register an offense when using only rest arg' do + expect_no_offenses(<<~RUBY) + def foo(*args) + bar(*args) + end + RUBY + end + 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 arguments forwarding. + bar(*args) + ^^^^^ Use arguments forwarding. + end + RUBY + + expect_correction(<<~RUBY) + def foo(...) + bar(...) + end + RUBY + end + end + end +end