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/RedundantFetchBlock cop #8147

Merged
merged 1 commit into from Jun 21, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,7 @@

### New features

* [#8147](https://github.com/rubocop-hq/rubocop/pull/8147): Add new `Style/RedundantFetchBlock` cop. ([@fatkodima][])
* [#8111](https://github.com/rubocop-hq/rubocop/pull/8111): Add auto-correct for `Style/StructInheritance`. ([@tejasbubane][])
* [#8113](https://github.com/rubocop-hq/rubocop/pull/8113): Let `expect_offense` templates add variable-length whitespace with `_{foo}`. ([@eugeneius][])
* [#8148](https://github.com/rubocop-hq/rubocop/pull/8148): Support autocorrection for `Style/MultilineTernaryOperator`. ([@koic][])
Expand Down Expand Up @@ -4597,3 +4598,4 @@
[@andrykonchin]: https://github.com/andrykonchin
[@avrusanov]: https://github.com/avrusanov
[@mauro-oto]: https://github.com/mauro-oto
[@fatkodima]: https://github.com/fatkodima
13 changes: 13 additions & 0 deletions config/default.yml
Expand Up @@ -3597,6 +3597,19 @@ Style/RedundantException:
VersionAdded: '0.14'
VersionChanged: '0.29'

Style/RedundantFetchBlock:
Description: >-
Use `fetch(key, value)` instead of `fetch(key) { value }`
when value has Numeric, Rational, Complex, Symbol or String type, `false`, `true`, `nil` or is a constant.
Reference: 'https://github.com/JuanitoFatas/fast-ruby#hashfetch-with-argument-vs-hashfetch--block-code'
Enabled: 'pending'
Safe: false
# If enabled, this cop will autocorrect usages of
# `fetch` being called with block returning a constant.
# This can be dangerous since constants will not be defined at that moment.
SafeForConstants: false
VersionAdded: '0.86'

Style/RedundantFreeze:
Description: "Checks usages of Object#freeze on immutable objects."
Enabled: true
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -434,6 +434,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#styleredundantcondition[Style/RedundantCondition]
* xref:cops_style.adoc#styleredundantconditional[Style/RedundantConditional]
* xref:cops_style.adoc#styleredundantexception[Style/RedundantException]
* xref:cops_style.adoc#styleredundantfetchblock[Style/RedundantFetchBlock]
* xref:cops_style.adoc#styleredundantfreeze[Style/RedundantFreeze]
* xref:cops_style.adoc#styleredundantinterpolation[Style/RedundantInterpolation]
* xref:cops_style.adoc#styleredundantparentheses[Style/RedundantParentheses]
Expand Down
64 changes: 64 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -7004,6 +7004,70 @@ raise 'message'

* https://rubystyle.guide#no-explicit-runtimeerror

== Style/RedundantFetchBlock

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

| Pending
| No
| Yes (Unsafe)
| 0.86
| -
|===

This cop identifies places where `fetch(key) { value }`
can be replaced by `fetch(key, value)`.

In such cases `fetch(key, value)` method is faster
than `fetch(key) { value }`.

=== Examples

==== SafeForConstants: false (default)

[source,ruby]
----
# bad
hash.fetch(:key) { 5 }
hash.fetch(:key) { true }
hash.fetch(:key) { nil }
array.fetch(5) { :value }
ENV.fetch(:key) { 'value' }

# good
hash.fetch(:key, 5)
hash.fetch(:key, true)
hash.fetch(:key, nil)
array.fetch(5, :value)
ENV.fetch(:key, 'value')
----

==== SafeForConstants: true

[source,ruby]
----
# bad
ENV.fetch(:key) { VALUE }

# good
ENV.fetch(:key, VALUE)
----

=== Configurable attributes

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

| SafeForConstants
| `false`
| Boolean
|===

=== References

* https://github.com/JuanitoFatas/fast-ruby#hashfetch-with-argument-vs-hashfetch--block-code

== Style/RedundantFreeze

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -424,6 +424,7 @@
require_relative 'rubocop/cop/style/line_end_concatenation'
require_relative 'rubocop/cop/style/method_call_without_args_parentheses'
require_relative 'rubocop/cop/style/method_call_with_args_parentheses'
require_relative 'rubocop/cop/style/redundant_fetch_block'
require_relative 'rubocop/cop/style/method_call_with_args_parentheses/omit_parentheses'
require_relative 'rubocop/cop/style/method_call_with_args_parentheses/require_parentheses'
require_relative 'rubocop/cop/style/method_called_on_do_end_block'
Expand Down
103 changes: 103 additions & 0 deletions lib/rubocop/cop/style/redundant_fetch_block.rb
@@ -0,0 +1,103 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# This cop identifies places where `fetch(key) { value }`
# can be replaced by `fetch(key, value)`.
#
# In such cases `fetch(key, value)` method is faster
fatkodima marked this conversation as resolved.
Show resolved Hide resolved
# than `fetch(key) { value }`.
#
# @example SafeForConstants: false (default)
# # bad
# hash.fetch(:key) { 5 }
# hash.fetch(:key) { true }
# hash.fetch(:key) { nil }
# array.fetch(5) { :value }
# ENV.fetch(:key) { 'value' }
#
# # good
# hash.fetch(:key, 5)
# hash.fetch(:key, true)
# hash.fetch(:key, nil)
# array.fetch(5, :value)
# ENV.fetch(:key, 'value')
#
# @example SafeForConstants: true
# # bad
# ENV.fetch(:key) { VALUE }
#
# # good
# ENV.fetch(:key, VALUE)
#
class RedundantFetchBlock < Cop
include FrozenStringLiteral
include RangeHelp

MSG = 'Use `%<good>s` instead of `%<bad>s`.'

def_node_matcher :redundant_fetch_block_candidate?, <<~PATTERN
(block
$(send _ :fetch _)
(args)
${#basic_literal? const_type?})
PATTERN

def on_block(node)
redundant_fetch_block_candidate?(node) do |send, body|
return if body.const_type? && !check_for_constant?
return if body.str_type? && !check_for_string?

range = fetch_range(send, node)
good = build_good_method(send, body)
bad = build_bad_method(send, body)

add_offense(
node,
location: range,
message: format(MSG, good: good, bad: bad)
)
end
end

def autocorrect(node)
redundant_fetch_block_candidate?(node) do |send, body|
lambda do |corrector|
receiver, _, key = send.children
corrector.replace(node, "#{receiver.source}.fetch(#{key.source}, #{body.source})")
end
end
end

private

def basic_literal?(node)
node.basic_literal?
end

def fetch_range(send, node)
range_between(send.loc.selector.begin_pos, node.loc.end.end_pos)
end

def build_good_method(send, body)
key = send.children[2].source
"fetch(#{key}, #{body.source})"
end

def build_bad_method(send, body)
key = send.children[2].source
"fetch(#{key}) { #{body.source} }"
end

def check_for_constant?
cop_config['SafeForConstants']
end

def check_for_string?
frozen_string_literals_enabled?
end
end
end
end
end
132 changes: 132 additions & 0 deletions spec/rubocop/cop/style/redundant_fetch_block_spec.rb
@@ -0,0 +1,132 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::RedundantFetchBlock do
subject(:cop) { described_class.new(config) }

context 'with SafeForConstants: true' do
let(:config) do
RuboCop::Config.new(
'Style/RedundantFetchBlock' => {
'SafeForConstants' => true
}
)
end

it 'registers an offense and corrects when using `#fetch` with Integer in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { 5 }
^^^^^^^^^^^^^^^^^ Use `fetch(:key, 5)` instead of `fetch(:key) { 5 }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, 5)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with Float in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { 2.5 }
^^^^^^^^^^^^^^^^^^^ Use `fetch(:key, 2.5)` instead of `fetch(:key) { 2.5 }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, 2.5)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with Symbol in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { :value }
^^^^^^^^^^^^^^^^^^^^^^ Use `fetch(:key, :value)` instead of `fetch(:key) { :value }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, :value)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with Rational in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { 2.0r }
^^^^^^^^^^^^^^^^^^^^ Use `fetch(:key, 2.0r)` instead of `fetch(:key) { 2.0r }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, 2.0r)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with Complex in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { 1i }
^^^^^^^^^^^^^^^^^^ Use `fetch(:key, 1i)` instead of `fetch(:key) { 1i }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, 1i)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with constant in the block' do
expect_offense(<<~RUBY)
hash.fetch(:key) { CONSTANT }
^^^^^^^^^^^^^^^^^^^^^^^^ Use `fetch(:key, CONSTANT)` instead of `fetch(:key) { CONSTANT }`.
RUBY

expect_correction(<<~RUBY)
hash.fetch(:key, CONSTANT)
RUBY
end

it 'registers an offense and corrects when using `#fetch` with String in the block and strings are frozen' do
expect_offense(<<~RUBY)
# frozen_string_literal: true
hash.fetch(:key) { 'value' }
^^^^^^^^^^^^^^^^^^^^^^^ Use `fetch(:key, 'value')` instead of `fetch(:key) { 'value' }`.
RUBY

expect_correction(<<~RUBY)
# frozen_string_literal: true
hash.fetch(:key, 'value')
RUBY
end

it 'does not register an offense when using `#fetch` with String in the block and strings are not frozen' do
expect_no_offenses(<<~RUBY)
hash.fetch(:key) { 'value' }
RUBY
end

it 'does not register an offense when using `#fetch` with argument fallback' do
expect_no_offenses(<<~RUBY)
hash.fetch(:key, :value)
RUBY
end

it 'does not register an offense when using `#fetch` with interpolated Symbol in the block' do
inspect_source('hash.fetch(:key) { :"value_#{value}" }')
expect(cop.offenses.size).to eq(0)
end

it 'does not register an offense when using `#fetch` with an argument in the block' do
inspect_source('hash.fetch(:key) { |k| "missing-#{k}" }')
expect(cop.offenses.size).to eq(0)
end
end

context 'with SafeForConstants: false' do
let(:config) do
RuboCop::Config.new(
'Style/RedundantFetchBlock' => {
'SafeForConstants' => false
}
)
end

it 'does not register an offense when using `#fetch` with constant in the block' do
expect_no_offenses(<<~RUBY)
hash.fetch(:key) { CONSTANT }
RUBY
end
end
end