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 does not work with Time objects #8334

Closed
Aqualon opened this issue Jul 14, 2020 · 7 comments
Closed

Style/ArrayCoercion does not work with Time objects #8334

Aqualon opened this issue Jul 14, 2020 · 7 comments
Labels

Comments

@Aqualon
Copy link
Contributor

Aqualon commented Jul 14, 2020

The Style/ArrayCoercion cop introduced in #8295 has some unexpected auto-correct behavior if dealing with Time objects.

Expected behavior

Style/ArrayCoercion auto-correct should not break code if dealing with Time objects.

Actual behavior

Style/ArrayCoercion auto-correct breaks if the wrapped variable is a Time object.

Steps to reproduce the problem

We have code like this to wrap an Time object called inserted_at in an Array

inserted_at = [inserted_at] unless inserted_at.is_a? Array

This was replaced by auto-correct with

inserted_at = Array(inserted_at)

This leads to an array with each part of the Time object as value

inserted_at = Time.now
2020-07-14 14:54:33.339436 +0200
Array(inserted_at)
[33, 54, 14, 14, 7, 2020, 2, 196, true, "CEST"]

I don't know why this behavior happens, but it seems just calling Array on some object, my lead to unexpected breaks.

RuboCop version

0.88.0 (using Parser 2.7.1.4, rubocop-ast 0.1.0, running on ruby 2.7.1 x86_64-darwin19)
@Aqualon
Copy link
Contributor Author

Aqualon commented Jul 14, 2020

https://ruby-doc.org/core-2.7.1/Kernel.html#method-i-Array describes Array

First tries to call to_ary on arg, then to_a. If arg does not respond to to_ary or to_a, returns an Array of length 1 containing arg.

The Time class provides to_a: https://ruby-doc.org/core-2.7.1/Time.html#method-i-to_a

So I assume for any object providing a to_a method, there may be unexpected behavior, because instead of [my_object] you could get whatever my_object returns from its to_a method.

@marcandre
Copy link
Contributor

Indeed. I think the equivalent of Array(foo) is [*foo].

@marcandre marcandre added the bug label Jul 14, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 15, 2020

Well, I guess it's safe to say that this cop is unsafe. Still, most of the time it does improve the code a bit.

@Aqualon
Copy link
Contributor Author

Aqualon commented Jul 25, 2020

I'm not sure I'd call it an improvement if it may change the code to something, that behaves differently than before.

@marcandre
Copy link
Contributor

See also #8391

koic added a commit to koic/rubocop that referenced this issue Sep 25, 2020
Fixes rubocop#8783, rubocop#8391, and rubocop#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 pushed 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.
@dvandersluis
Copy link
Member

I think this should probably be closed now that #8792 is merged?

@marcandre
Copy link
Contributor

Indeed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants