From a591ac2ef06eeaa5bff363259352c3e58a659909 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Thu, 30 Jul 2020 13:36:02 +0300 Subject: [PATCH] Add new `Style/ExplicitBlockArgument` cop --- CHANGELOG.md | 1 + config/default.yml | 10 ++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_style.adoc | 42 +++++++ lib/rubocop.rb | 1 + lib/rubocop/config_loader_resolver.rb | 4 +- .../cop/style/explicit_block_argument.rb | 91 ++++++++++++++ lib/rubocop/cop/style/parallel_assignment.rb | 4 +- .../cop/style/explicit_block_argument_spec.rb | 117 ++++++++++++++++++ 9 files changed, 267 insertions(+), 4 deletions(-) create mode 100644 lib/rubocop/cop/style/explicit_block_argument.rb create mode 100644 spec/rubocop/cop/style/explicit_block_argument_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 60c90fe84dc..46bb52e9429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * [#8389](https://github.com/rubocop-hq/rubocop/pull/8389): Add new `Lint/DuplicateRescueException` cop. ([@fatkodima][]) * [#8412](https://github.com/rubocop-hq/rubocop/pull/8412): Add new `Style/OptionalBooleanParameter` cop. ([@fatkodima][]) * [#8376](https://github.com/rubocop-hq/rubocop/pull/8376): Add new `Lint/MissingSuper` cop. ([@fatkodima][]) +* [#8415](https://github.com/rubocop-hq/rubocop/pull/8415): Add new `Style/ExplicitBlockArgument` cop. ([@fatkodima][]) * [#8339](https://github.com/rubocop-hq/rubocop/pull/8339): Add `Config#for_badge` as an efficient way to get a cop's config merged with its department's. ([@marcandre][]) * [#5067](https://github.com/rubocop-hq/rubocop/issues/5067): Add new `Style/StringConcatenation` cop. ([@fatkodima][]) * [#7425](https://github.com/rubocop-hq/rubocop/pull/7425): Add new `Lint/TopLevelReturnWithArgument` cop. ([@iamravitejag][]) diff --git a/config/default.yml b/config/default.yml index b8c6bc4cf88..a6c66a96a02 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2904,6 +2904,16 @@ Style/ExpandPathArguments: Enabled: true VersionAdded: '0.53' +Style/ExplicitBlockArgument: + Description: >- + Consider using explicit block argument to avoid writing block literal + that just passes its arguments to another block. + StyleGuide: '#block-argument' + Enabled: pending + # May change the yielding arity. + Safe: false + VersionAdded: '0.89' + Style/ExponentialNotation: Description: 'When using exponential notation, favor a mantissa between 1 (inclusive) and 10 (exclusive).' StyleGuide: '#exponential-notation' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index c8b52a383ce..1d7ac462f9e 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -368,6 +368,7 @@ In the following section you find all available cops: * xref:cops_style.adoc#styleevalwithlocation[Style/EvalWithLocation] * xref:cops_style.adoc#styleevenodd[Style/EvenOdd] * xref:cops_style.adoc#styleexpandpatharguments[Style/ExpandPathArguments] +* xref:cops_style.adoc#styleexplicitblockargument[Style/ExplicitBlockArgument] * xref:cops_style.adoc#styleexponentialnotation[Style/ExponentialNotation] * xref:cops_style.adoc#stylefloatdivision[Style/FloatDivision] * xref:cops_style.adoc#stylefor[Style/For] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index 2d3fa76d42b..d867559bdfd 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -2813,6 +2813,48 @@ Pathname.new(__FILE__).parent.expand_path Pathname.new(__dir__).expand_path ---- +== Style/ExplicitBlockArgument + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| No +| Yes (Unsafe) +| 0.89 +| - +|=== + +This cop enforces the use of explicit block argument to avoid writing +block literal that just passes its arguments to another block. + +=== Examples + +[source,ruby] +---- +# bad +def with_tmp_dir + Dir.mktmpdir do |tmp_dir| + Dir.chdir(tmp_dir) { |dir| yield dir } # block just passes arguments + end +end + +# good +def with_tmp_dir(&block) + Dir.mktmpdir do |tmp_dir| + Dir.chdir(tmp_dir, &block) + end +end + +with_tmp_dir do |dir| + puts "dir is accessible as a parameter and pwd is set: #{dir}" +end +---- + +=== References + +* https://rubystyle.guide#block-argument + == Style/ExponentialNotation |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 901bdfbd269..e0c4ab83273 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -412,6 +412,7 @@ require_relative 'rubocop/cop/style/eval_with_location' require_relative 'rubocop/cop/style/even_odd' require_relative 'rubocop/cop/style/expand_path_arguments' +require_relative 'rubocop/cop/style/explicit_block_argument' require_relative 'rubocop/cop/style/exponential_notation' require_relative 'rubocop/cop/style/float_division' require_relative 'rubocop/cop/style/for' diff --git a/lib/rubocop/config_loader_resolver.rb b/lib/rubocop/config_loader_resolver.rb index ad864b3c2ae..0c7e3d7098e 100644 --- a/lib/rubocop/config_loader_resolver.rb +++ b/lib/rubocop/config_loader_resolver.rb @@ -213,8 +213,8 @@ def handle_disabled_by_default(config, new_default_configuration) end end - def transform(config) - config.transform_values { |params| yield(params) } + def transform(config, &block) + config.transform_values(&block) end def gem_config_path(gem_name, relative_config_path) diff --git a/lib/rubocop/cop/style/explicit_block_argument.rb b/lib/rubocop/cop/style/explicit_block_argument.rb new file mode 100644 index 00000000000..76b3738d682 --- /dev/null +++ b/lib/rubocop/cop/style/explicit_block_argument.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # This cop enforces the use of explicit block argument to avoid writing + # block literal that just passes its arguments to another block. + # + # @example + # # bad + # def with_tmp_dir + # Dir.mktmpdir do |tmp_dir| + # Dir.chdir(tmp_dir) { |dir| yield dir } # block just passes arguments + # end + # end + # + # # good + # def with_tmp_dir(&block) + # Dir.mktmpdir do |tmp_dir| + # Dir.chdir(tmp_dir, &block) + # end + # end + # + # with_tmp_dir do |dir| + # puts "dir is accessible as a parameter and pwd is set: #{dir}" + # end + # + class ExplicitBlockArgument < Base + include RangeHelp + extend AutoCorrector + + MSG = 'Consider using explicit block argument in the '\ + "surrounding method's signature over `yield`." + + def_node_matcher :yielding_block?, <<~PATTERN + (block $_ (args $...) (yield $...)) + PATTERN + + def initialize(config = nil, options = nil) + super + @def_nodes = Set.new + end + + def on_yield(node) + block_node = node.parent + + yielding_block?(block_node) do |send_node, block_args, yield_args| + return unless yielding_arguments?(block_args, yield_args) + + add_offense(block_node) do |corrector| + corrector.remove(block_body_range(block_node, send_node)) + + add_block_argument(send_node, corrector) + + def_node = block_node.each_ancestor(:def, :defs).first + add_block_argument(def_node, corrector) if @def_nodes.add?(def_node) + end + end + end + + private + + def yielding_arguments?(block_args, yield_args) + return false if yield_args.empty? + + yield_args.zip(block_args).all? do |yield_arg, block_arg| + block_arg && yield_arg.children.first == block_arg.children.first + end + end + + def add_block_argument(node, corrector) + if node.arguments? + last_arg = node.arguments.last + corrector.insert_after(last_arg, ', &block') unless last_arg.blockarg_type? + elsif node.send_type? + corrector.insert_after(node, '(&block)') + else + corrector.insert_after(node.loc.name, '(&block)') + end + end + + def block_body_range(block_node, send_node) + range_between( + send_node.loc.expression.end_pos, + block_node.loc.end.end_pos + ) + end + end + end + end +end diff --git a/lib/rubocop/cop/style/parallel_assignment.rb b/lib/rubocop/cop/style/parallel_assignment.rb index 7633cc67e2a..dcff4a888d4 100644 --- a/lib/rubocop/cop/style/parallel_assignment.rb +++ b/lib/rubocop/cop/style/parallel_assignment.rb @@ -131,8 +131,8 @@ def initialize(assignments) @assignments = assignments end - def tsort_each_node - @assignments.each { |a| yield a } + def tsort_each_node(&block) + @assignments.each(&block) end def tsort_each_child(assignment) diff --git a/spec/rubocop/cop/style/explicit_block_argument_spec.rb b/spec/rubocop/cop/style/explicit_block_argument_spec.rb new file mode 100644 index 00000000000..be45bd9c936 --- /dev/null +++ b/spec/rubocop/cop/style/explicit_block_argument_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::ExplicitBlockArgument do + subject(:cop) { described_class.new } + + it 'registers an offense and corrects when block just yields its arguments' do + expect_offense(<<~RUBY) + def m + items.something(first_arg) { |i| yield i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Consider using explicit block argument in the surrounding method's signature over `yield`. + end + RUBY + + expect_correction(<<~RUBY) + def m(&block) + items.something(first_arg, &block) + end + RUBY + end + + it 'registers an offense and corrects when block yields several first its arguments' do + expect_offense(<<~RUBY) + def m + items.something { |i, j| yield i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Consider using explicit block argument in the surrounding method's signature over `yield`. + end + RUBY + + expect_correction(<<~RUBY) + def m(&block) + items.something(&block) + end + RUBY + end + + it 'correctly corrects when method already has an explicit block argument' do + expect_offense(<<~RUBY) + def m(&block) + items.something { |i| yield i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Consider using explicit block argument in the surrounding method's signature over `yield`. + end + RUBY + + expect_correction(<<~RUBY) + def m(&block) + items.something(&block) + end + RUBY + end + + it 'registers an offense and corrects when method contains multiple `yield`s' do + expect_offense(<<~RUBY) + def m + items.something { |i| yield i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Consider using explicit block argument in the surrounding method's signature over `yield`. + + if condition + yield 2 + elsif other_condition + 3.times { yield } + else + other_items.something { |i, j| yield i } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Consider using explicit block argument in the surrounding method's signature over `yield`. + end + end + RUBY + + expect_correction(<<~RUBY) + def m(&block) + items.something(&block) + + if condition + yield 2 + elsif other_condition + 3.times { yield } + else + other_items.something(&block) + end + end + RUBY + end + + it 'does not register an offense when `yield` is not inside block' do + expect_no_offenses(<<~RUBY) + def m + yield i + end + RUBY + end + + it 'does not register an offense when `yield` inside block has no arguments' do + expect_no_offenses(<<~RUBY) + def m + 3.times { yield } + end + RUBY + end + + it 'does not register an offense when `yield` is the sole block body' do + expect_no_offenses(<<~RUBY) + def m + items.something do |i| + do_something + yield i + end + end + RUBY + end + + it 'does not register an offense when `yield` arguments is not a prefix of block arguments' do + expect_no_offenses(<<~RUBY) + def m + items.something { |i, j, k| yield j, k } + end + RUBY + end +end