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 NonLocalExitFromIterator lint and fix other cops complained by it #1728

Merged
merged 2 commits into from Mar 23, 2015

Conversation

ypresto
Copy link
Contributor

@ypresto ypresto commented Mar 20, 2015

I found bug in our product that aborts iterator unexpectedly, with return statement.

def iterate
    array.each do |item|
        return if item.skip? # oh, no! it returns from "iterate" method!
        ...
    end
end

NonLocalExitFromIterator detects misused return statements in iterator block.
It uses heuristic way to distinguish intended use of return or not:

  • No return value.
    Because where next should be used tend to have no return value (but of map).
  • Method chain.
    E.g. array.each do |item| ... end and not before_fork do |server| ... end.
  • Block with arguments.
    E.g. not foobar.with_lock do ... end.

Cops affect with this bug also found: LiteralInInterpolation and AssignmentInCondition.
UnneededCapitalW is false-positive, but is still easy to use any? instead.

Thanks for great product!

@ypresto
Copy link
Contributor Author

ypresto commented Mar 20, 2015

jshint has /* falls through */ comment to allow case without break.
This would be better to have comment like # non-local exit, but I have no idea how to impl it...

@jonas054
Copy link
Collaborator

👍 I like this cop. Just had a few comments.

BTW, the equivalent to /* falls through */ is # rubocop:disable Lint/NonLocalExitFromIterator.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 20, 2015

Small remark from me - please, capitalize commit messages.

@ypresto ypresto force-pushed the return-from-iteration-block branch 2 times, most recently from f142e04 to 562de6c Compare March 21, 2015 12:27
@ypresto
Copy link
Contributor Author

ypresto commented Mar 21, 2015

# rubocop:disable Lint/NonLocalExitFromIterator

I was thinking it is verbose a little, but it is simple enough for something rarely used :)

Thanks for reviewing! Fixed for all comments. Rebased, CHANGELOG added.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 22, 2015

@ypresto You'll have to rebase again. Sorry about this.

- Fixed: AssignmentInCondition, LiteralInInterpolation
- Code style change: UnneededCapitalW
@ypresto ypresto force-pushed the return-from-iteration-block branch from 562de6c to 9ddb8f0 Compare March 23, 2015 01:28
@ypresto
Copy link
Contributor Author

ypresto commented Mar 23, 2015

re-rebased!

bbatsov added a commit that referenced this pull request Mar 23, 2015
Add NonLocalExitFromIterator lint and fix other cops complained by it
@bbatsov bbatsov merged commit 6aeafff into rubocop:master Mar 23, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 23, 2015

👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants