Open pboling opened 7 months ago
Autocorrect should run the complete rubocop linting. It should still fail, even if it can't fix anything. Regular rubocop fails the same way with or without the -a
flag.
@skryukov any thoughts on this?
Hey, @pboling! Sorry for the delay.
Autocorrect should run the complete rubocop linting. It should still fail, even if it can't fix anything.
Yes, that's exactly our approach—we initially run autocorrect, followed by the default rubocop-gradual command: https://github.com/skryukov/rubocop-gradual/blob/dd23f28876f045bf640a07a2a3c8844b517165ea/lib/rubocop/gradual/commands/autocorrect.rb#L10-L21
Based on the logs you shared, the number of inspected files is consistent, yet the number of detected issues varies, the question is why 🤔
I tried to reproduce the issue, but failed. To get more info we could try adding a debug option to the Rakefile
. This will add awful lot of stuff to the output, including RuboCop debug information:
RuboCop::Gradual::RakeTask.new.tap do |t|
t.options = ["--debug"]
end
One thing I have noticed is that most of the issues I've had with discrepancies have come from Betterment/betterlint
or from various Bundler rules (not sure which gem they come from). I'll add debug and see if it sheds any more light.
IMO - it is important that autocorrect run the same rules as a regular check, because that seems to be the behavior of RuboCop, and developers have been trained over many years to just run rubocop -a
, and have the expectation that if there are no failures then they can push and expect it to pass the CI build (which doesn't run -a
).
Since gradual allows hijacking of rubocop to run rubocop_gradual in it's place, it makes sense to support the existing expectations and workflow of developers.
We have the hijack in place, so people run rubocop
and get rubocop_gradual
, and because of this workflow discrepancy our developers frequently push code that appears to pass linting (via autocorrect), but fails the build on CI. It's a a frustrating loop.
I was confused about your comment:
Yes, that's exactly our approach—we initially run autocorrect, followed by the default rubocop-gradual command:
You're saying that this is what you expect to happen for everyone, as that is what the code in rubocop_gradual is intended to do?
That is definitely nothing like our experience, if so.
More info, just saw this again, and this time (thankfully) it is in a fully open source repo.
Without changing any files in between these commands:
❯ bundle exec rake rubocop_gradual:check
Running RuboCop Gradual...
...................................
Found 1 files with 6 issue(s).
Processing results...
RuboCop Gradual got no changes.
❯ bundle exec rake rubocop_gradual:autocorrect
Running RuboCop Gradual...
Inspecting 34 file(s) for autocorrection...
..................................
Fixed 0 file(s).
...................................
Found 1 files with 6 issue(s).
Processing results...
Uh oh, RuboCop Gradual got worse:
-> bin/bundle (1 new issues)
(line 64) "ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods."
RuboCop Gradual failed!
❯ bundle exec rake rubocop_gradual:force_update
Running RuboCop Gradual...
...................................
Found 1 files with 6 issue(s).
Processing results...
RuboCop Gradual got no changes.
@skryukov Just ran into this again on another private project. I tried the debug idea you posted earlier.
Starting with a clean git working tree. I ran force_udpate
to make sure I had no uncommitted changes initially, then check
, then autocorrect
, and what I found was very interesting.
❯ bundle exec rake rubocop_gradual:force_update
Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: force_update
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3896, :left=>3896}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.077933999942616 seconds
noglob bundle exec rake rubocop_gradual:force_update 37.51s user 7.49s system 257% cpu 17.451 total
❯ git status
On branch main
nothing to commit, working tree clean
❯ bundle exec rake rubocop_gradual:check
Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: check
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3896, :left=>3896}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.030246999929659 seconds
noglob bundle exec rake rubocop_gradual:check 38.42s user 8.82s system 263% cpu 17.951 total
❯ git status
On branch main
nothing to commit, working tree clean
Running RuboCop Gradual...
Inspecting 338 file(s) for autocorrection...
..................................................................................................................................................................................................................................................................................................................................................
Fixed 0 file(s).
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3890 issue(s).
Processing results...
RuboCop Gradual got 6 issue(s) fixed, 3890 left. Keep going!
Running RuboCop Gradual...
Gradual mode: update
Inspecting 338 file(s) for autocorrection...
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.127219000016339 seconds
noglob bundle exec rake rubocop_gradual:autocorrect 98.46s user 15.90s system 223% cpu 51.254 total
❯ git status
On branch main
...
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: .rubocop_gradual.lock
no changes added to commit (use "git add" and/or "git commit -a")
This is where the discrepancy occurs. The autocorrect output says there were no changes... but that's not true.
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.
The lockfile was changed. It should have failed, but it reported success with not changes found despite making actual changes.
This is born out by running check
a 2nd time, as this time it will note the failure.
Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
Uh oh, RuboCop Gradual got worse:
...
Unexpected Changes!
RuboCop Gradual lock file is outdated, to fix this message:
- Run `rubocop-gradual` locally and commit the results, or
- EVEN BETTER: before doing the above, try to fix the remaining issues in those files!
`.rubocop_gradual.lock` diff:
- [153, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
- [171, 5, 59, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 3742851078],
- [181, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+ [153, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
- [161, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
- [186, 5, 59, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 3742851078],
- [196, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+ [161, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
- [62, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
- [77, 5, 63, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 1476888842]
+ [62, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
- [229, 5, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
- [262, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+ [229, 5, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
RuboCop Gradual failed!
noglob bundle exec rake rubocop_gradual:check 19.91s user 4.76s system 211% cpu 11.689 total
Strangely, and as evidence of the discrepancy, if I run autocorrect again (still with the changed lockfile that just failed the 2nd check above), it will pass saying there are no changes.
This means that check
and autocorrect
are not parsing rules config, or the lockfile, or both, the same way.
Running RuboCop Gradual...
Inspecting 338 file(s) for autocorrection...
..................................................................................................................................................................................................................................................................................................................................................
Fixed 0 file(s).
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3890 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: update
Inspecting 338 file(s) for autocorrection...
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.472328000003472 seconds
noglob bundle exec rake rubocop_gradual:autocorrect 107.44s user 17.65s system 214% cpu 58.262 total
Because of this discrepancy I can no longer use autocorrect
on this project, since it will always fail check
in CI.
Thanks @pboling I'll try to look into it next week
I should add that so far all of the discrepancy in rules that I've paid enough attention to remember has been from one single rule:
Betterment/UnscopedFind
It comes from the betterlint
gem, which is an extension of RuboCop rules specifically for Rails projects.
It is possible that it is an issue of interplay between various plugins, as I do load many rubocop-*
gems.
Oh! I think I figured it out partially. I had been referencing Betterment rules in my local rubocop config file, but had failed to use the include_gem
directive.
inherit_gem:
betterlint:
- config/default.yml
Now that I've added that, it waffles between running force_update
and check
and autocorrect
consistently giving the same result, and, without changing any code, giving inconsistent results.
I think a bug in the unscoped find rule may be to blame. Perhaps it is not deterministic? https://github.com/Betterment/betterlint/issues/51
Anyhow, now that the config is loaded properly, it seems I can turn off the rule (Betterment/UnscopedFind
) and have it stay off reliably, so my lockfile remains consistent. If I turn the rule on I can't get consistent results.
My CI failed with a RuboCop violation. So locally I ran:
This is a surprising and invalid result, since I saw the result of running rubocop on CI (which runs
check
).Instead of saying it "got no changes" it should have reported the new failures we are about to see below...
So I ran
force_update
:The
force_update
caught the change somehow, when theautocorrect
did not.