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

Add new Style/ExplicitBlockArgument cop #8415

Merged
merged 1 commit into from Aug 1, 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 @@ -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][])
Expand Down
10 changes: 10 additions & 0 deletions config/default.yml
Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -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]
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 @@ -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'
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