Remove trailing named variables

[rrosenblum]

This turned into a larger commit than I originally thought it would be.

There are 3 modification to Style/TrailingUnderscorevariable.

• Register an offense for trailing *_
  • this was never covered by the cop initially
• Add a configuration to remove named trailing underscore variables.
  • I set this to be defaulted to true. Looking through RuboCop as an example, the vast majority of trailing named variables do not provide any useful information.
• Do not register an offense for a trailing underscore variables that are preceeded by a splat variable
  • This has been a bug in the cop since it was first created.
  • a, *b, = foo() is a syntax error

[rrosenblum]

I forgot to add some of the performance notes about this PR. When running my normal memory profile on rubocop lib/rubocop/cop/performance lib/rubocop/cop/rails lib/rubocop/cop/lint, I saw a 1.5% memory drop going from 58893570 bytes to 58010356 bytes.

It is also faster to not assign underscore variables

ARRAY = *1..10

def with_trailing
  a, b, c, d, e, *_f = *ARRAY
  nil
end  

def without_trailing
  a, b, c, d, e, = *ARRAY
  nil
end  

Benchmark.ips do |x|
  x.report('with trailing') { with_trailing }
  x.report('without trailing') { without_trailing }
  x.compare!
end  

Calculating -------------------------------------
       with trailing    94.549k i/100ms
    without trailing   116.251k i/100ms
-------------------------------------------------
       with trailing      1.877M (± 5.5%) i/s -      9.360M
    without trailing      2.986M (± 4.5%) i/s -     14.996M

Comparison:
    without trailing:  2985706.0 i/s
       with trailing:  1876711.2 i/s - 1.59x slower

[bbatsov]

Sorry that I'm responding to this PR just now. Was a bit short on time and wanted to push a bugfix only release before starting to merge new features, anyways.

My biggest doubt is what should be default now that the cop is configurable. I'd love to hear what other think about this.

[rrosenblum]

It is fine that it took a while to comment. I realize that this is a rather large commit and goes against the original design of this cop. I debated as to whether the configuration should be defaulted to on, or defaulted to off and turned on for this project.

From time to time when I have to modify the code having the variables around already is helpful (saves looking into the structure of the node, as always forget it).

I agree with you that it is useful to know the structure of the node. A vast majority of the _ named variables in RuboCop are all named _args. I was not sure if _args was providing value to anyone or not.

[rrosenblum]

I am fine with leaving the _ named variables for documentation purposes. What do you think about leaving the configuration option in and defaulting it to not remove named underscore variables?

[bbatsov]

What do you think about leaving the configuration option in and defaulting it to not remove named underscore variables?

That'd be great.

[rrosenblum]

I changed the configuration name to AllowNamedUnderscoreVariables. It is defaulted it to true, the initial behavior. I also removed the commit that removes all of the trailing named underscore variables.

[bbatsov]

This has to be rebased.

[rrosenblum]

The code has been rebased.

[bbatsov]

👍