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.

Sign up for GitHub

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

Jump to bottom

Accept a nil argument for Array#flatten. #3175

Closed
wants to merge 1 commit into from
Closed

Accept a nil argument for Array#flatten. #3175

wants to merge 1 commit into from

Conversation

dubek
Copy link
Contributor

@dubek dubek commented Nov 3, 2014

In order to mirror MRI's behaviour, nil is allowed as the argument of
Array#flatten and Array#flatten!. It is treated as -1 which means
endless recursive flatenning of the Array.

MRI:

> RUBY_DESCRIPTION
 => "ruby 2.1.3p242 (2014-09-19 revision 47630) [x86_64-linux]" 
> [].flatten(nil)
 => []

Rubinius 2.3.0 before this fix:

> RUBY_DESCRIPTION
 => "rubinius 2.3.0.n307 (2.1.0 5cd0c42e 2014-11-03) [x86_64-linux-gnu]" 
> [].flatten(nil)
TypeError: Coercion error: nil.to_int => Fixnum failed
    from kernel/common/type.rb:36:in `coerce_to_failed'
    from kernel/common/type.rb:132:in `coerce_to_collection_index'
    from kernel/common/array.rb:748:in `flatten'
    from (irb):1
    from kernel/common/block_environment.rb:53:in `call_on_instance'
    from kernel/common/eval.rb:176:in `eval'
    from kernel/common/kernel.rb:478:in `loop'
    from kernel/bootstrap/proc.rb:20:in `call'
    from kernel/common/throw_catch.rb:30:in `catch'
    from kernel/common/throw_catch.rb:8:in `register'
    from kernel/common/throw_catch.rb:29:in `catch'
    from kernel/bootstrap/proc.rb:20:in `call'
    from kernel/common/throw_catch.rb:30:in `catch'
    from kernel/common/throw_catch.rb:8:in `register'
    from kernel/common/throw_catch.rb:29:in `catch'
    from kernel/delta/code_loader.rb:66:in `load_script'
    from kernel/delta/code_loader.rb:152:in `load_script'
    from kernel/loader.rb:645:in `script'
    from kernel/loader.rb:799:in `main'

OS:

Linux dmurik 2.6.32-431.30.1.el6.x86_64 #1 SMP Wed Jul 30 14:44:26 EDT 2014 x86_64 x86_64 x86_64 GNU/Linux

@dubek
Accept a nil argument for Array#flatten.
76e7b71 
In order to mirror MRI's behaviour, nil is allowed as the argument of
Array#flatten and Array#flatten!.  It is treated as -1 which means
endless recursive flatenning of the Array.
@yorickpeterse
Copy link
Contributor

While the C source code of MRI explicitly checks for a nil value (http://rxr.whitequark.org/mri/source/array.c#4284) I suspect this is an artifact of the C function taking a variable number of arguments (http://rxr.whitequark.org/mri/source/array.c#5548, -1 indicates a variable argument number). Looking at the tests for Array#flatten and Array#flatten! (https://github.com/ruby/ruby/blob/eeb05e8c119f8cab6434d90f21551b6bb2954778/test/ruby/test_array.rb#L761) I can't find anything that would indicate this behaviour is by design.

If this is an artifact, and not a design choice, I don't think this would be something we'd want to merge in. I checked JRuby 1.7.12's behaviour and that currently matches Rubinius' behaviour.

In case this is something we do want to merge the spec changes would have to be extracted into a separate commit. Doing so makes it much easier to synchronize these changes back into RubySpec (said process is already painful enoguh).

@dubek
Copy link
Contributor Author

dubek commented Nov 3, 2014

I agree that Array#flatten is a lot clearer if it allows only an integer argument (or none at all). I just encountered a gem that "relies" on flatten(nil) and therefore fails in Rubinius.

Who decides in such matters?

@chuckremes
Copy link
Member

Send a pull request / fix to the gem in question.

@dubek
Copy link
Contributor Author

dubek commented Nov 3, 2014

Thanks. If so - you may close this or mark it as "won't fix" / "not a bug".

@yorickpeterse
Copy link
Contributor

Hm, I'd be interested in knowing what the rationale would be for using Array#flatten(nil) instead of just Array#flatten(0). What particular Gem is this?

@dubek
Copy link
Contributor Author

dubek commented Nov 3, 2014

https://github.com/hamstergem/hamster/blob/master/lib/hamster/vector.rb#L425 - in this case, modifying the default value for the method argument from nil to -1 solves the issue (works both on Rubinius and MRI).

@yorickpeterse
Copy link
Contributor

@dubek Ah, yeah it seems the only reason nil is passed to Array#flatten there is due to it being a default argument value (and not because there's some hard requirement on nil being passed). In that case it would make more sense to fix the Gem instead.

I'll close this pull request then, feel free to re-open whenever needed.

@dubek dubek mentioned this pull request Nov 4, 2014
Closed
@brixen
Copy link
Member

brixen commented Nov 4, 2014

FWIW, someone should file a bug with MRI that 1. clarifies the documentation (I don't see any mention of -1 in ri flatten); and 2. clarifies the argument parsing that is allowing nil to be accepted.

@dubek
Copy link
Contributor Author

dubek commented Nov 4, 2014

MRI bug report regarding Array#flatten behaviour and documentation: https://bugs.ruby-lang.org/issues/10475

@brixen
Copy link
Member

brixen commented Nov 5, 2014

@dubek thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dubek @yorickpeterse @chuckremes @brixen