diff --git a/CHANGELOG.md b/CHANGELOG.md index 28fe55da52..2b28ad837b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### New features +* [#126](https://github.com/rubocop-hq/rubocop-performance/pull/126): Add new `Performance/RedundantFetchBlock` cop. ([@fatkodima][]) * [#132](https://github.com/rubocop-hq/rubocop-performance/issues/132): Add new `Performance/RedundantSortBlock` cop. ([@fatkodima][]) * [#125](https://github.com/rubocop-hq/rubocop-performance/pull/125): Support `Array()` and `Hash()` methods for `Performance/Size` cop. ([@fatkodima][]) * [#124](https://github.com/rubocop-hq/rubocop-performance/pull/124): Add new `Performance/Squeeze` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index daf9d8c99a..d440a72f79 100644 --- a/config/default.yml +++ b/config/default.yml @@ -162,6 +162,18 @@ Performance/RedundantBlockCall: Enabled: true VersionAdded: '0.36' +Performance/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: true + VersionAdded: '1.7' + SafeForConstants: 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. + Performance/RedundantMatch: Description: >- Use `=~` instead of `String#match` or `Regexp#match` in a context where the diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index fded315504..f874baf0ff 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -22,6 +22,7 @@ * xref:cops_performance.adoc#performanceopenstruct[Performance/OpenStruct] * xref:cops_performance.adoc#performancerangeinclude[Performance/RangeInclude] * xref:cops_performance.adoc#performanceredundantblockcall[Performance/RedundantBlockCall] +* xref:cops_performance.adoc#performanceredundantfetchblock[Performance/RedundantFetchBlock] * xref:cops_performance.adoc#performanceredundantmatch[Performance/RedundantMatch] * xref:cops_performance.adoc#performanceredundantmerge[Performance/RedundantMerge] * xref:cops_performance.adoc#performanceredundantsortblock[Performance/RedundantSortBlock] diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc index 942ff5f07a..2bbd5a0764 100644 --- a/docs/modules/ROOT/pages/cops_performance.adoc +++ b/docs/modules/ROOT/pages/cops_performance.adoc @@ -948,6 +948,59 @@ end * https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode +== Performance/RedundantFetchBlock + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Enabled +| Yes +| Yes +| 1.7 +| - +|=== + +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 + +[source,ruby] +---- +# bad +hash.fetch(:key) { 5 } +hash.fetch(:key) { true } +hash.fetch(:key) { nil } +array.fetch(5) { :value } +ENV.fetch(:key) { 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) +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 + == Performance/RedundantMatch |=== diff --git a/lib/rubocop/cop/performance/redundant_fetch_block.rb b/lib/rubocop/cop/performance/redundant_fetch_block.rb new file mode 100644 index 0000000000..724c668e3f --- /dev/null +++ b/lib/rubocop/cop/performance/redundant_fetch_block.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # 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 }`. + # + # @example + # # bad + # hash.fetch(:key) { 5 } + # hash.fetch(:key) { true } + # hash.fetch(:key) { nil } + # array.fetch(5) { :value } + # ENV.fetch(:key) { 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) + # ENV.fetch(:key, 'value') + # + class RedundantFetchBlock < Cop + include FrozenStringLiteral + include RangeHelp + + MSG = 'Use `%s` instead of `%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 diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 7dd5c1f079..2034042eaa 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -22,6 +22,7 @@ require_relative 'performance/open_struct' require_relative 'performance/range_include' require_relative 'performance/redundant_block_call' +require_relative 'performance/redundant_fetch_block' require_relative 'performance/redundant_match' require_relative 'performance/redundant_merge' require_relative 'performance/redundant_sort_block' diff --git a/spec/rubocop/cop/performance/redundant_fetch_block_spec.rb b/spec/rubocop/cop/performance/redundant_fetch_block_spec.rb new file mode 100644 index 0000000000..e490cf9bb6 --- /dev/null +++ b/spec/rubocop/cop/performance/redundant_fetch_block_spec.rb @@ -0,0 +1,132 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::RedundantFetchBlock do + subject(:cop) { described_class.new(config) } + + context 'with SafeForConstants: true' do + let(:config) do + RuboCop::Config.new( + 'Performance/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( + 'Performance/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