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

Abort when auto-correct causes an infinite loop. #1492

Merged
merged 1 commit into from Dec 15, 2014

Conversation

dblock
Copy link
Contributor

@dblock dblock commented Dec 12, 2014

This avoids future situations such as the one described in #1484. At the very least when you run into an infinite loop RuboCop won't hang and tell you where the loop occurs so you can go correct it, exclude a cop or report it as a bug.

Also maybe we should call InfiniteCorrectionLoop something else, like LifetimeSentenceLoop ;)

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 12, 2014

You might add a few comments here and there, as casual readers might not realise what this code's for.

@jonas054
Copy link
Collaborator

👍 Nicely done! But keep the line length down.

@dblock dblock force-pushed the conflicting-cops branch 5 times, most recently from 0dcaef5 to 2ac3063 Compare December 12, 2014 19:11
@dblock
Copy link
Contributor Author

dblock commented Dec 12, 2014

This is good to merge. I've also added --max-auto-correct-count to avoid having a potential infinite loop when a Cop continues to flag a violation even after it has been auto-corrected. This will avoid all infinite loops after a certain (default 50) count, and I think it's a necessary evil.

Another change is to make CopStore return another CopStore so you can chain filtering out cops, and introduce test cops that are ignored at runtime so that we don't need to hack collections of cops used exclusively in tests.

@jonas054
Copy link
Collaborator

@dblock I mostly agree. The first commit is all good. We've seen this problem a number of times, so a solution is most welcome.

The problem solved by the second commit is more rare, I think, but I seem to recall that it has happened before that corrections just add more and more space, for example, without end. But I really wish we could find a solution where we don't add another command line option. It seems an excessive measure for a rare problem. If we base the max loop count on the initial number of correctable offenses, wouldn't it be possible to choose a number that will not need customization?

@dblock
Copy link
Contributor Author

dblock commented Dec 13, 2014

I am totally open for suggestions, but I don't think there's any way to derive a number of max loops. You're looking at a misbehaving cop that keeps making a correction, no in a loop, but in the "adding a space each time" way. My spec does just that, it changes a class name to something random, so it will loop forever. So what's the maximum number there?

We could just hard-code the number at something high, like 128, just cause it's a power of 2, that would be just fine. In the usual case you don't have this problem anyway, so this is just a precaution.

Let me know what you want to do to move this PR forward (including ripping that part out).

@dblock dblock force-pushed the conflicting-cops branch 2 times, most recently from c21599f to e309731 Compare December 13, 2014 22:59
@dblock
Copy link
Contributor Author

dblock commented Dec 13, 2014

I removed the questionable part about the second commit, so this is now only detecting infinite loops where the cop autocorrects something and then comes back to the original source. I will PR that later separately and we can discuss it there.

@dblock
Copy link
Contributor Author

dblock commented Dec 13, 2014

🍏

no_color: 'Disable color output.',
version: 'Display version.',
verbose_version: 'Display verbose version.'
only: 'Run only the given cop(s).',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change whitespace only now? Seems unnecessary then.

@jonas054
Copy link
Collaborator

@dblock Good idea! The changes look good to me, apart from some added spaces.

`-V/--verbose-version` | Displays the current version plus the version of Parser and Ruby
`-F/--fail-fast` | Inspects in modification time order and stops after first file with offenses
`-d/--debug` | Displays some extra debug output
`-v/--version` | Displays the current version and exits.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readme changes seem totally unrelated to me.

@dblock dblock force-pushed the conflicting-cops branch 2 times, most recently from 0dfcf88 to 8d8a129 Compare December 14, 2014 16:33
@dblock
Copy link
Contributor Author

dblock commented Dec 14, 2014

Ok, cleaned this up, removed unrelated things (will make separate PRs), fixed the rest of the comments.

bbatsov added a commit that referenced this pull request Dec 15, 2014
Abort when auto-correct causes an infinite loop.
@bbatsov bbatsov merged commit 1d0753b into rubocop:master Dec 15, 2014
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