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

Conversation

@fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Jul 31, 2020

Inspired by https://eslint.org/docs/rules/no-unreachable-loop and by

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
end

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.

# 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
@bbatsov bbatsov merged commit eb5d16b into rubocop-hq:master Aug 1, 2020
26 checks passed
26 checks passed
windows 2.4
Details
windows 2.5
Details
windows 2.6
Details
windows 2.7
Details
windows mingw
Details
ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: cc-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-spec Your tests passed on CircleCI!
Details
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Aug 1, 2020

Fantastic work! Дякую! 🙇‍♂️

@fatkodima
Copy link
Contributor Author

@fatkodima fatkodima commented Aug 1, 2020

Будь ласка :) Радий зробити свій внесок.

@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 1, 2020

Good stuff! 🙇‍♂️ That FIX ME: 🤣

Unless I'm mistaken, the following case would raise an offense when it shouldn't, right?

while something do
  raise 'oops' rescue nil
end

Probably best to exclude cases with a rescue or catch. Even if you do, this cop is unsafe. But if you do, this cop's autocorrection would actually be safe, something we don't handle well (cop with known false positives but no known false negatives).

@fatkodima
Copy link
Contributor Author

@fatkodima fatkodima commented Aug 1, 2020

Unless I'm mistaken, the following case would raise an offense when it shouldn't, right?

No, this won't raise an offense.

Probably best to exclude cases with a rescue or catch

Yes, those are actually excluded (not checked)

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

@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 1, 2020

No, this won't raise an offense.

Oh, good, my mistake 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.