Skip to content

Commit

Permalink
Add new Lint/UnreachableLoop cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima authored and bbatsov committed Aug 1, 2020
1 parent 10cedb8 commit eb5d16b
Show file tree
Hide file tree
Showing 10 changed files with 444 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -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][])
Expand Down
5 changes: 5 additions & 0 deletions config/default.yml
Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -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]
Expand Down
83 changes: 83 additions & 0 deletions docs/modules/ROOT/pages/cops_lint.adoc
Expand Up @@ -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

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -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'
Expand Down
5 changes: 1 addition & 4 deletions lib/rubocop/cop/lint/safe_navigation_chain.rb
Expand Up @@ -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
Expand Down
9 changes: 3 additions & 6 deletions lib/rubocop/cop/lint/suppressed_exception.rb
Expand Up @@ -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
Expand Down
174 changes: 174 additions & 0 deletions 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
10 changes: 7 additions & 3 deletions lib/rubocop/cop/style/alias.rb
Expand Up @@ -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)
Expand Down

0 comments on commit eb5d16b

Please sign in to comment.