Skip to content

Commit

Permalink
Add new Style/ExplicitBlockArgument cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima authored and bbatsov committed Aug 1, 2020
1 parent eb5d16b commit e1c057d
Show file tree
Hide file tree
Showing 9 changed files with 267 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -10,6 +10,7 @@
* [#8430](https://github.com/rubocop-hq/rubocop/pull/8430): Add new `Lint/UnreachableLoop` 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][])
Expand Down
10 changes: 10 additions & 0 deletions config/default.yml
Expand Up @@ -2909,6 +2909,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'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -369,6 +369,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]
Expand Down
42 changes: 42 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -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

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -413,6 +413,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'
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/config_loader_resolver.rb
Expand Up @@ -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)
Expand Down
91 changes: 91 additions & 0 deletions 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
4 changes: 2 additions & 2 deletions lib/rubocop/cop/style/parallel_assignment.rb
Expand Up @@ -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)
Expand Down
117 changes: 117 additions & 0 deletions 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

0 comments on commit e1c057d

Please sign in to comment.