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

--auto-gen-config not resolving all offenses after upgrading from 0.32.1 #2228

Closed
maxjacobson opened this issue Sep 7, 2015 · 16 comments · Fixed by #2568
Closed

--auto-gen-config not resolving all offenses after upgrading from 0.32.1 #2228

maxjacobson opened this issue Sep 7, 2015 · 16 comments · Fixed by #2568

Comments

@maxjacobson
Copy link
Contributor

I'm having trouble reproducing this in an isolated code base, so here's the relevant information:

  • 1779 ruby files
  • many offenses listed in the .rubocop_todo.yml file, which was generated by rubocop 0.32.1
  • everything is green in rubocop 0.32.1, but I'm trying to upgrade
  • the Lint/EndAlignment cop is set to AlignWith: variable
  • I have no Lint/EndAlignment offenses, but Style/ElseAlignment is disabled in the todo file
  • I upgrade to a newer rubocop (both 0.33 and 0.34 have this behavior), and now I have some new offenses, because some new cops were introduced
  • (my .rubocop_todo.yml is still the one generated by 0.32.1)
  • I don't want to resolve all of the new offenses right now, I'd rather just regenerate the .rubocop_todo.yml file, so I run bin/rubocop --auto-gen-config and it regenerates my .rubocop_todo.yml file.

At this point, when I run bin/rubocop, there should be no offenses, right?

I start getting a bunch of Style/ElseAlignment offenses.

Before the regeneration (excerpts):

# .rubocop_todo.yml
Style/ElseAlignment:
  Enabled: false

# .rubocop.yml
inherit_from .rubocop_todo.yml
Lint/EndAlignment:
  AlignWith: variable

After the regeneration (excerpts):

# .rubocop_todo.yml
Lint/EndAlignment:
  Enabled: false

# .rubocop.yml
inherit_from .rubocop_todo.yml
Lint/EndAlignment:
  AlignWith: variable

Two interesting things:

  1. Lint/EndAlignment is disabled even though it's got a configuration (new offenses seem to have been discovered in 0.33 related to the alignment of the end of case statements, but I'm not sure which entry in the changelog is responsible for this)
  2. Style/ElseAlignment is no longer disabled, even though those offenses didn't go anywhere, so now I have a bunch of new offenses

Desired behavior: after running bin/rubocop --auto-gen-config, there would be no offenses

@jonas054
Copy link
Collaborator

jonas054 commented Sep 8, 2015

It would be great if you could run git bisect to find where the bug was introduced.

cd /path/to/rubocop_repo
git bisect start
git bisect good v0.32.1
git bisect bad v0.33.0
git bisect run /path/to/testscript.sh
git bisect reset

# testscript.sh
cd /path/to/code_base_where_problem_was_found
/path/to/rubocop_repo/bin/rubocop --auto-gen-config
/path/to/rubocop_repo/bin/rubocop

@maxjacobson
Copy link
Contributor Author

👍 trying it now

@maxjacobson
Copy link
Contributor Author

Kind of inconclusive. It found 3bc885c as the first bad commit, but it doesn't seem related to me

@jonas054
Copy link
Collaborator

jonas054 commented Sep 8, 2015

Too bad. I was going to say that d8c7c37 was a prime suspect, but it was merged after 0.33.0 was released. Could you still try and see what happens if you git revert d8c7c37?

@alexdowad
Copy link
Contributor

@maxjacobson What if you rm the old .rubocop_todo.yml file before generating a new one?

@maxjacobson
Copy link
Contributor Author

Yea I think we moved on from the issue by manually resolving it, but I wanted to report it in case it's a bug that can be fixed. It's not a huge deal I think

@alexdowad
Copy link
Contributor

My point was, I think the problem may have been caused by the presence of the old .rubocop_todo.yml. There is another open issue in this tracker regarding a similar problem.

@maxjacobson
Copy link
Contributor Author

Ah, got it.

I was under the impression that the .rubocop_todo.yml file was temporarily ignored while generating a new one -- at least, this seems to be the behavior.

I'll revert our codebase to when I opened this issue...

OK, yep, just reconfirmed the behavior from the original report. Trying now with removing the old file before generating the new one.

No, it seems to have the same behavior. Still some offenses snuck through.

@alexdowad
Copy link
Contributor

No, it seems to have the same behavior. Still some offenses snuck through.

OK, that's good to know.

@alexdowad
Copy link
Contributor

Is this codebase proprietary?

@maxjacobson
Copy link
Contributor Author

Yes. If you'd like, I can try to reproduce it in a public codebase

@alexdowad
Copy link
Contributor

I can try to reproduce it in a public codebase

Please do.

My technique for coming up with minimal repro:

  1. Fresh checkout of your private repo
  2. Create a new branch, maybe call it debug_rubocop or something
  3. Confirm you can reproduce the problem. Set yourself up so you can reproduce it with a single, short command (maybe a script).
  4. Start deleting subdirectories. After each deletion, confirm you can still reproduce the problem. If you can, commit the change to the debug_rubocop branch. If you can't, back out.
  5. Start deleting individual files. Same deal; commit if you can still reproduce.
  6. Start deleting code from the files which remain (hopefully just one). Commit each time, if you can still reproduce.
  7. If there are more than one file remaining, try merging their contents and see if you can still reproduce.
  8. After a few hundred iterations, each of which should be fairly fast, you should be left with 1 or 2 very small files. Obfuscate the contents if you like by renaming methods, etc.

@madwort
Copy link
Contributor

madwort commented Jan 1, 2016

Could this bug caused by this line?
https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/style/else_alignment.rb#L89
How about this instead:

end_config = config.for_cop('Lint/EndAlignment')
style = end_config['AlignWith'] ? end_config['AlignWith'] : 'keyword'

@madwort
Copy link
Contributor

madwort commented Jan 1, 2016

I think this reproduces the bug:

.rubocop.yml:

Lint/EndAlignment:
  AlignWith: variable

test case:

def method_name
  my_var = if condition
    'thing1'
  else
    'thing2'
  end

  my_var = if condition
             'thing1'
  else
             'thing2'
           end
end

output:

$ bundle exec rubocop test_align.rb  --auto-gen-config ; bundle exec rubocop test_align.rb  ; cat .rubocop_todo.yml 
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
W

Offenses:

test_align.rb:2:3: W: Useless assignment to variable - my_var.
  my_var = if condition
  ^^^^^^
test_align.rb:8:3: W: Useless assignment to variable - my_var.
  my_var = if condition
  ^^^^^^
test_align.rb:9:3: C: Use 2 (not 11) spaces for indentation.
             'thing1'
  ^^^^^^^^^^^
test_align.rb:11:3: C: Use 2 (not 11) spaces for indentation.
             'thing2'
  ^^^^^^^^^^^
test_align.rb:12:12: W: end at 12, 11 is not aligned with my_var = if at 8, 2.
           end
           ^^^

1 file inspected, 5 offenses detected
Created .rubocop_todo.yml.
Run `rubocop --config .rubocop_todo.yml`, or add `inherit_from: .rubocop_todo.yml` in a .rubocop.yml file.
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test_align.rb:4:3: C: Align else with if.
  else
  ^^^^
test_align.rb:10:3: C: Align else with if.
  else
  ^^^^

1 file inspected, 2 offenses detected
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2016-01-01 12:51:09 +0000 using RuboCop version 0.35.1.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.

# Offense count: 1
# Cop supports --auto-correct.
# Configuration parameters: AlignWith, SupportedStyles, AutoCorrect.
# SupportedStyles: keyword, variable, start_of_line
Lint/EndAlignment:
  Enabled: false

# Offense count: 2
Lint/UselessAssignment:
  Exclude:
    - 'test_align.rb'

# Offense count: 2
# Cop supports --auto-correct.
# Configuration parameters: Width.
Style/IndentationWidth:
  Exclude:
    - 'test_align.rb'

@madwort
Copy link
Contributor

madwort commented Jan 1, 2016

FWIW, it might be possible to trigger similar weirdness with Style/IndentationWidth (and maybe others):
https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/style/indentation_width.rb#L91
Is there a general policy to ignore the configuration of cops if they're disabled (which is logical, but will lead to this weirdness when using --auto-gen)?

For contrast, here there is no test for whether the cop is enabled:
https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/style/indentation_width.rb#L68

@maxjacobson
Copy link
Contributor Author

I think @madwort has done a nice job reproducing it. I tried but couldn't

madwort added a commit to madwort/rubocop that referenced this issue Jan 31, 2016
bbatsov added a commit that referenced this issue Jan 31, 2016
…-disabled-cops

[Fixes #2228] Use the config of a related cop whether it's enabled or not
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 a pull request may close this issue.

4 participants