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

Style/ArrayCoercion auto-correction not equal for nil values #8391

Closed
andrewngo opened this issue Jul 24, 2020 · 8 comments
Closed

Style/ArrayCoercion auto-correction not equal for nil values #8391

andrewngo opened this issue Jul 24, 2020 · 8 comments

Comments

@andrewngo
Copy link

@andrewngo andrewngo commented Jul 24, 2020

If a value can be nil, then ArrayCoercion auto-corrected change does not produce an equivalent change. It's a good suggestion but not always valid.

Expected behavior

Don't auto-correct unless certain

Actual behavior

Changes code

Steps to reproduce the problem

test_value = nil

original_result = [test_value] unless test_value.is_a?(Array)
new_result = Array(test_value)

original_result == new_result # false

# original_result [nil]
# new_result []

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:

$ [bundle exec] rubocop -V
0.88.0 (using Parser 2.7.1.4, rubocop-ast 0.2.0, running on ruby 2.6.5 x86_64-darwin16)
@marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 24, 2020

Agreed, this cop is, at the very least, not safe. Thanks for raising this issue.

Any object that responds_to? :to_a is an issue:

Array({a: 1}) # => [[:a, 1]]  != [{a: 1}]
Array(Set[1, 2, 3]) # => [1, 2, 3]  != [Set[1, 2, 3]]
Array(1..) # => RangeError ! = [1..]

I'll make it as unsafe but the issue should remain open: does this cop deserve its badge?

marcandre added a commit to marcandre/rubocop that referenced this issue Jul 24, 2020
marcandre added a commit that referenced this issue Jul 24, 2020
@0x0badc0de
Copy link

@0x0badc0de 0x0badc0de commented Jul 27, 2020

What about using Array.new(1, value)?

irb(main):001:0> Array.new(1, {a: 1})
=> [{:a=>1}]
irb(main):002:0> Array.new(1, Set[1, 2, 3])
=> [#<Set: {1, 2, 3}>]
irb(main):003:0> Array.new(1, 1..)
=> [1..]
@0x0badc0de
Copy link

@0x0badc0de 0x0badc0de commented Jul 27, 2020

Ah, it won't work for arrays args as expected :(

@0x0badc0de
Copy link

@0x0badc0de 0x0badc0de commented Jul 27, 2020

This one seems to work https://stackoverflow.com/a/29431497

irb(main):001:0> [{a: 1}].flatten(1)
=> [{:a=>1}]
irb(main):002:0> [Set[1, 2, 3]].flatten(1)
=> [#<Set: {1, 2, 3}>]
irb(main):003:0> [1..].flatten(1)
=> [1..]
irb(main):004:0> [Time.now].flatten(1)
=> [2020-07-27 17:07:10.792690033 +0300]
irb(main):005:0> [[1, 2]].flatten(1)
=> [1, 2]
irb(main):006:0> [[{a:1}, Time.now]].flatten(1)
=> [{:a=>1}, 2020-07-27 17:07:32.01765441 +0300]
@marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 27, 2020

There just isn't a reliable builtin way to do this.

Maybe this cop could enforce the use of proper duck-typing:

# bad?
x = [x] unless x.is_a?(Array)
# good
x = [x] unless x.respond_to? :to_ary

Note: I did mean to use to_ary (implicit conversion to Array), and to_a (explicit conversion) is not good.

Given the fact that to_ary (and how it differs from to_a) is pretty obscure, I think that the best solution is that this cop should be entirely retired. There's nothing inherently wrong with x = [x] unless x.is_a?(Array) and there is not any better and reliable alternative.

@sos4nt
Copy link

@sos4nt sos4nt commented Aug 6, 2020

Just checking whether x responds to to_ary might not be enough, you probably also have to invoke it:

x = x.respond_to?(:to_ary) ? x.to_ary : [x]

IMO, x = [x] unless x.is_a?(Array) is much easier to understand. It almost literally says "put x in an array unless x is an array". I don't think that's "bad" at all.

+1 for retiring this cop

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 25, 2020

There's also the option of simply disabling the cop by default, which is a bit less extreme (at least people can run it on demand then). The fact that relatively few people complained about running into conversion issues leads me to believe that's not a very common problem, but that' definitely a cop that can't be made safe.

koic added a commit to koic/rubocop that referenced this issue Sep 25, 2020
Fixes rubocop-hq#8783, rubocop-hq#8391, and rubocop-hq#8334.

This PR disables `Style/ArrayCoercion` cop by default.
Because false positive will occur if the argument of `Array()` is not an array (e.g. Hash, Set),
an array will be returned as an incompatibility result.
bbatsov added a commit that referenced this issue Sep 25, 2020
Fixes #8783, #8391, and #8334.

This PR disables `Style/ArrayCoercion` cop by default.
Because false positive will occur if the argument of `Array()` is not an array (e.g. Hash, Set),
an array will be returned as an incompatibility result.
@marcandre
Copy link
Contributor

@marcandre marcandre commented Oct 4, 2020

Now disabled by default #8792

@marcandre marcandre closed this Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.