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

Rubocop crashes on splat operator in case/when block when in auto-correct mode #3597

Closed
rpherbig opened this issue Oct 11, 2016 · 2 comments · Fixed by #3602
Closed

Rubocop crashes on splat operator in case/when block when in auto-correct mode #3597

rpherbig opened this issue Oct 11, 2016 · 2 comments · Fixed by #3602

Comments

@rpherbig
Copy link


Expected behavior

Running rubocop in auto-correct mode should behave the same as normal mode, but also fix as many issues as possible. This is the output in normal mode:

$ rubocop crash.rb
Inspecting 1 file
W

Offenses:

crash.rb:1:11: C: Use %w or %W for an array of words.
results = ['a', 'b']
          ^^^^^^^^^^
crash.rb:2:6: W: Literal 'a' appeared in a condition.
case 'a'
     ^^^
crash.rb:3:1: C: Place when conditions with a splat at the end of the when branches.
when *results, 'c'
^^^^^^^^^^^^^
crash.rb:7:4: C: Final newline missing.
end

Actual behavior

However, rubocop crashes on the same code in auto-correct mode:

$ rubocop --auto-correct crash.rb
Inspecting 1 file


0 files inspected, no offenses detected
undefined method `source_range' for nil:NilClass
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/performance/case_when_splat.rb:126:in `when_branch_range'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/performance/case_when_splat.rb:110:in `reorder_condition'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/performance/case_when_splat.rb:86:in `block in autocorrect'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/corrector.rb:57:in `call'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/corrector.rb:57:in `block (2 levels) in rewrite'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/parser-2.3.1.4/lib/parser/source/rewriter.rb:194:in `transaction'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/corrector.rb:56:in `block in rewrite'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/corrector.rb:54:in `each'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/corrector.rb:54:in `rewrite'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:145:in `autocorrect_all_cops'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:77:in `autocorrect'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:105:in `block in offenses'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:122:in `investigate'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:101:in `offenses'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cop/team.rb:52:in `inspect_file'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:244:in `inspect_file'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:191:in `block in do_inspection_loop'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:223:in `block in iterate_until_no_changes'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:216:in `loop'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:216:in `iterate_until_no_changes'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:187:in `do_inspection_loop'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:102:in `block in file_offenses'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:112:in `file_offense_cache'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:100:in `file_offenses'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:91:in `process_file'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:69:in `block in each_inspected_file'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:66:in `each'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:66:in `reduce'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:66:in `each_inspected_file'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:58:in `inspect_files'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/runner.rb:37:in `run'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cli.rb:72:in `execute_runner'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/lib/rubocop/cli.rb:28:in `run'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/bin/rubocop:14:in `block in <top (required)>'
c:/Ruby200/lib/ruby/2.0.0/benchmark.rb:296:in `realtime'
c:/Ruby200/lib/ruby/gems/2.0.0/gems/rubocop-0.43.0/bin/rubocop:13:in `<top (required)>'
c:/Ruby200/bin/rubocop:23:in `load'
c:/Ruby200/bin/rubocop:23:in `<main>'

Steps to reproduce the problem

I made a simple ruby file that demonstrates the problem. Run Rubocop on it twice to see the problem (once with auto-correct, once without):

results = ['a', 'b']
case 'a'
when *results, 'c'
  puts 'true'
else
  puts 'false'
end

RuboCop version

$ rubocop -V
0.43.0 (using Parser 2.3.1.4, running on ruby 2.0.0 i386-mingw32)
@rpherbig
Copy link
Author

It appears to be CaseWhenSplat which is causing the crash.

@rrosenblum
Copy link
Contributor

@rpherbig, I was not able to reproduce the reported issue with the provided example. I was however able to reproduce the reported issue when there are 2 when branches.

results = ['a', 'b']
case 'a'
when *results, 'c'
  puts 'true'
when results, 'c'
  puts 'true'
else
  puts 'false'
end

I noticed a different issue for the example that you provided. CaseWhenSplat is not auto-correcting the offense that it is registering. I found some examples in the tests for this same scenario. There is a TODO to see if reordering the splat after the other variable will speed up the case statement. I have run a benchmark and determined that it is faster to list all splat expanded variables last.

[8] pry(main)> def splat_first(num)
[8] pry(main)*   case num
[8] pry(main)*   when *FOO, 6, 7, 8  
[8] pry(main)*     true
[8] pry(main)*   else  
[8] pry(main)*     false
[8] pry(main)*   end  
[8] pry(main)* end  
=> :splat_first
[9] pry(main)> def splat_last(num)
[9] pry(main)*   case num
[9] pry(main)*   when 6, 7, 8, *FOO  
[9] pry(main)*     true
[9] pry(main)*   else  
[9] pry(main)*     false
[9] pry(main)*   end  
[9] pry(main)* end  
=> :splat_last

[17] pry(main)> Benchmark.ips do |x|
[17] pry(main)*   x.report('splat first value in splat') { splat_first(3) }
[17] pry(main)*   x.report('splat first value not in splat') { splat_first(7) }
[17] pry(main)*   x.report('splat first no match') { splat_first(0) }
[17] pry(main)*   x.report('splat last value in splat') { splat_last(3) }
[17] pry(main)*   x.report('splat last value not in splat') { splat_last(7) }
[17] pry(main)*   x.report('splat last no match') { splat_last(0) }
[17] pry(main)*   x.compare!
[17] pry(main)* end  
Warming up --------------------------------------
splat first value in splat
                        58.742k i/100ms
splat first value not in splat
                        57.867k i/100ms
splat first no match    53.724k i/100ms
splat last value in splat
                       111.219k i/100ms
splat last value not in splat
                       135.851k i/100ms
 splat last no match   113.707k i/100ms
Calculating -------------------------------------
splat first value in splat
                        708.937k (± 9.9%) i/s -      3.525M in   5.023715s
splat first value not in splat
                        652.417k (± 9.5%) i/s -      3.241M in   5.013931s
splat first no match    631.900k (± 9.0%) i/s -      3.170M in   5.062059s
splat last value in splat
                          1.618M (± 6.4%) i/s -      8.119M in   5.039173s
splat last value not in splat
                          1.907M (± 8.3%) i/s -      9.510M in   5.023153s
 splat last no match      1.483M (± 6.9%) i/s -      7.391M in   5.007247s

Comparison:
splat last value not in splat:    1906993.2 i/s
splat last value in splat:        1618336.7 i/s - 1.18x slower
splat last no match:              1483380.3 i/s - 1.29x slower
splat first value in splat:       708936.7 i/s - 2.69x slower
splat first value not in splat:   652417.0 i/s - 2.92x slower
splat first no match:             631900.4 i/s - 3.02x slower

tldr: There are two things that should be fixed in this cop. The first is the NoMethodError. The second is that we need to auto-correct all splat variables to the end of each when statement.

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.

2 participants