From eb5d16b31a2962ce5a4b59f4c85abd60ad7f7e17 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 31 Jul 2020 21:40:03 +0300 Subject: [PATCH] Add new `Lint/UnreachableLoop` cop --- CHANGELOG.md | 1 + config/default.yml | 5 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 83 +++++++++ lib/rubocop.rb | 1 + lib/rubocop/cop/lint/safe_navigation_chain.rb | 5 +- lib/rubocop/cop/lint/suppressed_exception.rb | 9 +- lib/rubocop/cop/lint/unreachable_loop.rb | 174 ++++++++++++++++++ lib/rubocop/cop/style/alias.rb | 10 +- .../rubocop/cop/lint/unreachable_loop_spec.rb | 168 +++++++++++++++++ 10 files changed, 444 insertions(+), 13 deletions(-) create mode 100644 lib/rubocop/cop/lint/unreachable_loop.rb create mode 100644 spec/rubocop/cop/lint/unreachable_loop_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 60c90fe84dc..43bd8cea9f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#8322](https://github.com/rubocop-hq/rubocop/pull/8322): Support autocorrect for `Style/CaseEquality` cop. ([@fatkodima][]) * [#8291](https://github.com/rubocop-hq/rubocop/pull/8291): Add new `Lint/SelfAssignment` cop. ([@fatkodima][]) * [#8389](https://github.com/rubocop-hq/rubocop/pull/8389): Add new `Lint/DuplicateRescueException` cop. ([@fatkodima][]) +* [#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][]) * [#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][]) diff --git a/config/default.yml b/config/default.yml index b8c6bc4cf88..3119cc103b1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1846,6 +1846,11 @@ Lint/UnreachableCode: Enabled: true VersionAdded: '0.9' +Lint/UnreachableLoop: + Description: 'This cop checks for loops that will have at most one iteration.' + Enabled: pending + VersionAdded: '0.89' + Lint/UnusedBlockArgument: Description: 'Checks for unused block arguments.' StyleGuide: '#underscore-unused-vars' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index c8b52a383ce..9f735aeb8d7 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -261,6 +261,7 @@ In the following section you find all available cops: * xref:cops_lint.adoc#lintunderscoreprefixedvariablename[Lint/UnderscorePrefixedVariableName] * xref:cops_lint.adoc#lintunifiedinteger[Lint/UnifiedInteger] * xref:cops_lint.adoc#lintunreachablecode[Lint/UnreachableCode] +* xref:cops_lint.adoc#lintunreachableloop[Lint/UnreachableLoop] * xref:cops_lint.adoc#lintunusedblockargument[Lint/UnusedBlockArgument] * xref:cops_lint.adoc#lintunusedmethodargument[Lint/UnusedMethodArgument] * xref:cops_lint.adoc#linturiescapeunescape[Lint/UriEscapeUnescape] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 578a0f5625a..7abd7390c02 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -3661,6 +3661,89 @@ def some_method end ---- +== Lint/UnreachableLoop + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| No +| 0.89 +| - +|=== + +This cop checks for loops that will have at most one iteration. + +A loop that can never reach the second iteration is a possible error in the code. +In rare cases where only one iteration (or at most one iteration) is intended behavior, +the code should be refactored to use `if` conditionals. + +=== Examples + +[source,ruby] +---- +# bad +while node + do_something(node) + node = node.parent + break +end + +# good +while node + do_something(node) + node = node.parent +end + +# bad +def verify_list(head) + item = head + begin + if verify(item) + return true + else + return false + end + end while(item) +end + +# good +def verify_list(head) + item = head + begin + if verify(item) + item = item.next + else + return false + end + end while(item) + + true +end + +# bad +def find_something(items) + items.each do |item| + if something?(item) + return item + else + raise NotFoundError + end + end +end + +# good +def find_something(items) + items.each do |item| + if something?(item) + return item + end + end + raise NotFoundError +end +---- + == Lint/UnusedBlockArgument |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 901bdfbd269..8beb6f50cee 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -319,6 +319,7 @@ require_relative 'rubocop/cop/lint/underscore_prefixed_variable_name' require_relative 'rubocop/cop/lint/unified_integer' require_relative 'rubocop/cop/lint/unreachable_code' +require_relative 'rubocop/cop/lint/unreachable_loop' require_relative 'rubocop/cop/lint/unused_block_argument' require_relative 'rubocop/cop/lint/unused_method_argument' require_relative 'rubocop/cop/lint/uri_escape_unescape' diff --git a/lib/rubocop/cop/lint/safe_navigation_chain.rb b/lib/rubocop/cop/lint/safe_navigation_chain.rb index 760faf1a62b..32ca6f91c96 100644 --- a/lib/rubocop/cop/lint/safe_navigation_chain.rb +++ b/lib/rubocop/cop/lint/safe_navigation_chain.rb @@ -53,10 +53,7 @@ def on_send(node) def method_chain(node) chain = node - while chain.send_type? - chain = chain.parent if chain.parent&.call_type? - break # FIXME: Unconditional break. Why while "loop" then? - end + chain = chain.parent if chain.send_type? && chain.parent&.call_type? chain end end diff --git a/lib/rubocop/cop/lint/suppressed_exception.rb b/lib/rubocop/cop/lint/suppressed_exception.rb index 696a50e479a..c69bd6b5fba 100644 --- a/lib/rubocop/cop/lint/suppressed_exception.rb +++ b/lib/rubocop/cop/lint/suppressed_exception.rb @@ -77,13 +77,10 @@ def on_resbody(node) private def comment_between_rescue_and_end?(node) - end_line = nil - node.each_ancestor(:kwbegin, :def, :defs, :block) do |ancestor| - end_line = ancestor.loc.end.line - break - end - return false unless end_line + ancestor = node.each_ancestor(:kwbegin, :def, :defs, :block).first + return unless ancestor + end_line = ancestor.loc.end.line processed_source[node.first_line...end_line].any? { |line| comment_line?(line) } end end diff --git a/lib/rubocop/cop/lint/unreachable_loop.rb b/lib/rubocop/cop/lint/unreachable_loop.rb new file mode 100644 index 00000000000..ecf863cbee7 --- /dev/null +++ b/lib/rubocop/cop/lint/unreachable_loop.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # This cop checks for loops that will have at most one iteration. + # + # A loop that can never reach the second iteration is a possible error in the code. + # In rare cases where only one iteration (or at most one iteration) is intended behavior, + # the code should be refactored to use `if` conditionals. + # + # @example + # # bad + # while node + # do_something(node) + # node = node.parent + # break + # end + # + # # good + # while node + # do_something(node) + # node = node.parent + # end + # + # # bad + # def verify_list(head) + # item = head + # begin + # if verify(item) + # return true + # else + # return false + # end + # end while(item) + # end + # + # # good + # def verify_list(head) + # item = head + # begin + # if verify(item) + # item = item.next + # else + # return false + # end + # end while(item) + # + # true + # end + # + # # bad + # def find_something(items) + # items.each do |item| + # if something?(item) + # return item + # else + # raise NotFoundError + # end + # end + # end + # + # # good + # def find_something(items) + # items.each do |item| + # if something?(item) + # return item + # end + # end + # raise NotFoundError + # end + # + class UnreachableLoop < Base + MSG = 'This loop will have at most one iteration.' + + def on_while(node) + check(node) + end + alias on_until on_while + alias on_while_post on_while + alias on_until_post on_while + alias on_for on_while + + def on_block(node) + check(node) if loop_method?(node) + end + + private + + def loop_method?(node) + return false unless node.block_type? + + send_node = node.send_node + send_node.enumerable_method? || send_node.enumerator_method? || send_node.method?(:loop) + end + + def check(node) + statements = statements(node) + break_statement = statements.find { |statement| break_statement?(statement) } + return unless break_statement + + add_offense(node) unless preceded_by_continue_statement?(break_statement) + end + + def statements(node) + body = node.body + + if body.nil? + [] + elsif body.begin_type? + body.children + else + [body] + end + end + + def_node_matcher :break_command?, <<~PATTERN + { + return break + (send + {nil? (const {nil? cbase} :Kernel)} + {:raise :fail :throw :exit :exit! :abort} + ...) + } + PATTERN + + def break_statement?(node) + return true if break_command?(node) + + case node.type + when :begin, :kwbegin + statements = *node + statements.any? { |statement| break_statement?(statement) } + when :if + check_if(node) + when :case + check_case(node) + else + false + end + end + + def check_if(node) + if_branch = node.if_branch + else_branch = node.else_branch + if_branch && else_branch && + break_statement?(if_branch) && break_statement?(else_branch) + end + + def check_case(node) + else_branch = node.else_branch + return false unless else_branch + return false unless break_statement?(else_branch) + + node.when_branches.all? do |branch| + branch.body && break_statement?(branch.body) + end + end + + def preceded_by_continue_statement?(break_statement) + left_siblings_of(break_statement).any? do |sibling| + next if sibling.loop_keyword? || loop_method?(sibling) + + sibling.each_descendant(:next, :redo).any? + end + end + + def left_siblings_of(node) + node.parent.children[0, node.sibling_index] + end + end + end + end +end diff --git a/lib/rubocop/cop/style/alias.rb b/lib/rubocop/cop/style/alias.rb index a86818b7333..66663d3985c 100644 --- a/lib/rubocop/cop/style/alias.rb +++ b/lib/rubocop/cop/style/alias.rb @@ -101,10 +101,14 @@ def scope_type(node) end def lexical_scope_type(node) - node.each_ancestor(:class, :module) do |ancestor| - return ancestor.class_type? ? 'in a class body' : 'in a module body' + ancestor = node.each_ancestor(:class, :module).first + if ancestor.nil? + 'at the top level' + elsif ancestor.class_type? + 'in a class body' + else + 'in a module body' end - 'at the top level' end def bareword?(sym_node) diff --git a/spec/rubocop/cop/lint/unreachable_loop_spec.rb b/spec/rubocop/cop/lint/unreachable_loop_spec.rb new file mode 100644 index 00000000000..21dc9b64127 --- /dev/null +++ b/spec/rubocop/cop/lint/unreachable_loop_spec.rb @@ -0,0 +1,168 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Lint::UnreachableLoop do + subject(:cop) { described_class.new } + + context 'without preceding continue statements' do + it 'registers an offense when using `break`' do + expect_offense(<<~RUBY) + while x > 0 + ^^^^^^^^^^^ This loop will have at most one iteration. + x += 1 + break + end + RUBY + end + + it 'registers an offense when using `if-else` with all break branches' do + expect_offense(<<~RUBY) + while x > 0 + ^^^^^^^^^^^ This loop will have at most one iteration. + if condition + break + else + raise MyError + end + end + RUBY + end + + it 'does not register an offense when using `if` without `else`' do + expect_no_offenses(<<~RUBY) + while x > 0 + if condition + break + elsif other_condition + raise MyError + end + end + RUBY + end + + it 'does not register an offense when using `if-elsif-else` and not all branches are breaking' do + expect_no_offenses(<<~RUBY) + while x > 0 + if condition + break + elsif other_condition + do_something + else + raise MyError + end + end + RUBY + end + + it 'registers an offense when using `case-when-else` with all break branches' do + expect_offense(<<~RUBY) + while x > 0 + ^^^^^^^^^^^ This loop will have at most one iteration. + case x + when 1 + break + else + raise MyError + end + end + RUBY + end + + it 'does not register an offense when using `case` without `else`' do + expect_no_offenses(<<~RUBY) + while x > 0 + case x + when 1 + break + end + end + RUBY + end + + it 'does not register an offense when using `case-when-else` and not all branches are breaking' do + expect_no_offenses(<<~RUBY) + while x > 0 + case x + when 1 + break + when 2 + do_something + else + raise MyError + end + end + RUBY + end + end + + context 'with preceding continue statements' do + it 'does not register an offense when using `break`' do + expect_no_offenses(<<~RUBY) + while x > 0 + next if x.odd? + x += 1 + break + end + RUBY + end + + it 'does not register an offense when using `if-else` with all break branches' do + expect_no_offenses(<<~RUBY) + while x > 0 + next if x.odd? + if condition + break + else + raise MyError + end + end + RUBY + end + + it 'does not register an offense when using `case-when-else` with all break branches' do + expect_no_offenses(<<~RUBY) + while x > 0 + redo if x.odd? + + case x + when 1 + break + else + raise MyError + end + end + RUBY + end + end + + it 'handles inner loops' do + expect_offense(<<~RUBY) + until x > 0 + ^^^^^^^^^^^ This loop will have at most one iteration. + + items.each do |item| + next if item.odd? + break + end + + if x > 0 + break some_value + else + raise MyError + end + + loop do + ^^^^^^^ This loop will have at most one iteration. + + case y + when 1 + return something + when 2 + break + else + throw :exit + end + end + end + RUBY + end +end