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 Lint/UnreachableLoop cop #8430

Merged
merged 1 commit into from Aug 1, 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
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