diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a50aba28ba..866a3629fc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#8451](https://github.com/rubocop-hq/rubocop/issues/8451): Add new `Style/RedundantSelfAssignment` cop. ([@fatkodima][]) * [#8390](https://github.com/rubocop-hq/rubocop/pull/8390): Add new `Style/SoleNestedConditional` cop. ([@fatkodima][]) * [#8562](https://github.com/rubocop-hq/rubocop/pull/8562): Add new `Style/KeywordParametersOrder` cop. ([@fatkodima][]) +* [#8486](https://github.com/rubocop-hq/rubocop/pull/8486): Add new `Style/CombinableLoops` cop. ([@fatkodima][]) * [#8381](https://github.com/rubocop-hq/rubocop/pull/8381): Add new `Style/ClassMethodsDefinitions` cop. ([@fatkodima][]) * [#8474](https://github.com/rubocop-hq/rubocop/pull/8474): Add new `Lint/DuplicateRequire` cop. ([@fatkodima][]) * [#8472](https://github.com/rubocop-hq/rubocop/issues/8472): Add new `Lint/UselessMethodDefinition` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index 8027c697130..ab73b084c52 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2721,6 +2721,14 @@ Style/ColonMethodDefinition: Enabled: true VersionAdded: '0.52' +Style/CombinableLoops: + Description: >- + Checks for places where multiple consecutive loops over the same data + can be combined into a single loop. + Enabled: pending + Safe: false + VersionAdded: '0.90' + Style/CommandLiteral: Description: 'Use `` or %x around command literals.' StyleGuide: '#percent-x' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index e78da508f88..723124af31a 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -350,6 +350,7 @@ In the following section you find all available cops: * xref:cops_style.adoc#stylecollectionmethods[Style/CollectionMethods] * xref:cops_style.adoc#stylecolonmethodcall[Style/ColonMethodCall] * xref:cops_style.adoc#stylecolonmethoddefinition[Style/ColonMethodDefinition] +* xref:cops_style.adoc#stylecombinableloops[Style/CombinableLoops] * xref:cops_style.adoc#stylecommandliteral[Style/CommandLiteral] * xref:cops_style.adoc#stylecommentannotation[Style/CommentAnnotation] * xref:cops_style.adoc#stylecommentedkeyword[Style/CommentedKeyword] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index cff2ee51ff8..c2f3db78c99 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -1374,6 +1374,68 @@ end * https://rubystyle.guide#colon-method-definition +== Style/CombinableLoops + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| No +| No +| 0.90 +| - +|=== + +This cop checks for places where multiple consecutive loops over the same data +can be combined into a single loop. It is very likely that combining them +will make the code more efficient and more concise. + +It is marked as unsafe, because the first loop might modify +a state that the second loop depends on; these two aren't combinable. + +=== Examples + +[source,ruby] +---- +# bad +def method + items.each do |item| + do_something(item) + end + + items.each do |item| + do_something_else(item) + end +end + +# good +def method + items.each do |item| + do_something(item) + do_something_else(item) + end +end + +# bad +def method + for item in items do + do_something(item) + end + + for item in items do + do_something_else(item) + end +end + +# good +def method + for item in items do + do_something(item) + do_something_else(item) + end +end +---- + == Style/CommandLiteral |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 6964edbc756..787e1da8614 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -395,6 +395,7 @@ require_relative 'rubocop/cop/style/collection_methods' require_relative 'rubocop/cop/style/colon_method_call' require_relative 'rubocop/cop/style/colon_method_definition' +require_relative 'rubocop/cop/style/combinable_loops' require_relative 'rubocop/cop/style/command_literal' require_relative 'rubocop/cop/style/comment_annotation' require_relative 'rubocop/cop/style/commented_keyword' diff --git a/lib/rubocop/cop/style/combinable_loops.rb b/lib/rubocop/cop/style/combinable_loops.rb new file mode 100644 index 00000000000..a57ff860f43 --- /dev/null +++ b/lib/rubocop/cop/style/combinable_loops.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # This cop checks for places where multiple consecutive loops over the same data + # can be combined into a single loop. It is very likely that combining them + # will make the code more efficient and more concise. + # + # It is marked as unsafe, because the first loop might modify + # a state that the second loop depends on; these two aren't combinable. + # + # @example + # # bad + # def method + # items.each do |item| + # do_something(item) + # end + # + # items.each do |item| + # do_something_else(item) + # end + # end + # + # # good + # def method + # items.each do |item| + # do_something(item) + # do_something_else(item) + # end + # end + # + # # bad + # def method + # for item in items do + # do_something(item) + # end + # + # for item in items do + # do_something_else(item) + # end + # end + # + # # good + # def method + # for item in items do + # do_something(item) + # do_something_else(item) + # end + # end + # + class CombinableLoops < Base + MSG = 'Combine this loop with the previous loop.' + + def on_block(node) + return unless collection_looping_method?(node) + + sibling = left_sibling_of(node) + add_offense(node) if same_collection_looping?(node, sibling) + end + + def on_for(node) + sibling = left_sibling_of(node) + add_offense(node) if sibling&.for_type? && node.collection == sibling.collection + end + + private + + def collection_looping_method?(node) + method_name = node.send_node.method_name + method_name.match?(/^each/) || method_name.match?(/_each$/) + end + + def left_sibling_of(node) + return unless node.parent&.begin_type? + + index = node.sibling_index - 1 + node.parent.children[index] if index >= 0 + end + + def same_collection_looping?(node, sibling) + sibling&.block_type? && + sibling.send_node.method?(node.method_name) && + sibling.send_node.receiver == node.send_node.receiver + end + end + end + end +end diff --git a/spec/rubocop/cop/lint/void_spec.rb b/spec/rubocop/cop/lint/void_spec.rb index 23906f2c62d..56a4654b678 100644 --- a/spec/rubocop/cop/lint/void_spec.rb +++ b/spec/rubocop/cop/lint/void_spec.rb @@ -15,18 +15,14 @@ a %{op} b RUBY end - end - described_class::BINARY_OPERATORS.each do |op| it "accepts void op #{op} if on last line" do expect_no_offenses(<<~RUBY) something a #{op} b RUBY end - end - described_class::BINARY_OPERATORS.each do |op| it "accepts void op #{op} by itself without a begin block" do expect_no_offenses("a #{op} b") end @@ -67,9 +63,7 @@ #{op}b RUBY end - end - unary_operators.each do |op| it "accepts void unary op #{op} by itself without a begin block" do expect_no_offenses("#{op}b") end diff --git a/spec/rubocop/cop/style/combinable_loops_spec.rb b/spec/rubocop/cop/style/combinable_loops_spec.rb new file mode 100644 index 00000000000..0de1450d8b7 --- /dev/null +++ b/spec/rubocop/cop/style/combinable_loops_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::CombinableLoops do + subject(:cop) { described_class.new } + + context 'when looping method' do + it 'registers an offense when looping over the same data as previous loop' do + expect_offense(<<~RUBY) + items.each { |item| do_something(item) } + items.each { |item| do_something_else(item, arg) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine this loop with the previous loop. + + items.each_with_index { |item| do_something(item) } + items.each_with_index { |item| do_something_else(item, arg) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine this loop with the previous loop. + + items.reverse_each { |item| do_something(item) } + items.reverse_each { |item| do_something_else(item, arg) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine this loop with the previous loop. + RUBY + end + + it 'does not register an offense when the same loops are interleaved with some code' do + expect_no_offenses(<<~RUBY) + items.each { |item| do_something(item) } + + some_code + + items.each { |item| do_something_else(item, arg) } + RUBY + end + + it 'does not register an offense when the same loop method is used over different collections' do + expect_no_offenses(<<~RUBY) + items.each { |item| do_something(item) } + bars.each { |bar| do_something(bar) } + RUBY + end + + it 'does not register an offense when different loop methods are used over the same collection' do + expect_no_offenses(<<~RUBY) + items.reverse_each { |item| do_something(item) } + items.each { |item| do_something(item) } + RUBY + end + + it 'does not register an offense when each branch contains the same single loop over the same collection' do + expect_no_offenses(<<~RUBY) + if condition + items.each { |item| do_something(item) } + else + items.each { |item| do_something_else(item, arg) } + end + RUBY + end + end + + context 'when for loop' do + it 'registers an offense when looping over the same data as previous loop' do + expect_offense(<<~RUBY) + for item in items do do_something(item) end + for item in items do do_something_else(item, arg) end + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine this loop with the previous loop. + RUBY + end + + it 'does not register an offense when the same loops are interleaved with some code' do + expect_no_offenses(<<~RUBY) + for item in items do do_something(item) end + + some_code + + for item in items do do_something_else(item, arg) end + RUBY + end + + it 'does not register an offense when the same loop method is used over different collections' do + expect_no_offenses(<<~RUBY) + for item in items do do_something(item) end + for foo in foos do do_something(foo) end + RUBY + end + + it 'does not register an offense when each branch contains the same single loop over the same collection' do + expect_no_offenses(<<~RUBY) + if condition + for item in items do do_something(item) end + else + for item in items do do_something_else(item, arg) end + end + RUBY + end + end +end