Performance/PushSplat autocorrect can lead to runtime error

[savef]

Actual behavior

With a push splat where the argument may be nil autocorrect will still change it and then the code will throw a TypeError (as Array#concat cannot handle nil arguments).

Expected behavior

Autocorrect should not cause breaking changes to codebase. Therefore when auto-correcting a push splat where the argument may be nil it should either not correct it, or maybe coerce it to an array with #to_a.

Steps to reproduce the problem

1. Put this code somewhere:

   x = nil
   y = []
   y.push(*x)
   

2. Run the code, see that it doesn't raise an exception.

3. Run RuboCop with autocorrect, see it makes 1 change.

4. Run the code again, see that it now raises a TypeError.

RuboCop version

$ rubocop -V
0.41.1 (using Parser 2.3.1.2, running on ruby 2.3.1 x86_64-linux)

I've tested this on master too, still applies.

[Drenmi]

So the main problem here is that, by definition, every variable may be nil, and there is no way for us to infer what it will be at runtime.

So the options are then:

1. Leave as is, and expect developers to design their code to avoid nil references.
2. Change the cop to always auto-correct to the nil safe x.concat(y.to_a). (Since this is a performance cop, the overhead of calling #to_a would need to be taken into consideration.)
3. Consider the existence of auto-correct for this cop altogether.

[savef]

and expect developers to design their code to avoid nil references.

Impossible. 😛
I think leaving it comes down to whether the intention of autocorrect is to only correct code style without making changes to what the code does or not... I had always expected it to behave that way but if that's not the opinion of @bbatsov or a majority of the user base then leaving it as it is now is fine.

Consider the existence of auto-correct for this cop altogether.

It could still work for literals.

[Drenmi]

It could still work for literals.

Yes. Literals are the exception.

[Drenmi]

Hm. I've been benchmarking this, and no matter what parameters I use, the "fast" version is consistently 4% slower. Might be that Ruby internals changed and this is no longer relevant.

Since the performance is minuscule, I'd suggest we drop this cop altogether. (Unless we want to move the cop to Style, and invert it, to suggest using the nil safe style.)

@bbatsov, @jonas054.

[ragesoss]

It's not just nil that can cause problems. In my app, making this autocorrection causes this error:

TypeError:
       no implicit conversion of Hash into Array

While push(*x) does convert a hash into an array of arrays, concat does not.

[jonas054]

I've confirmed that the cop can produce code that's slower and/or crashes. I agree that we have to remove it. PR coming up.