Project

General

Profile

Actions

Bug #6203

closed

Array#values_at does not handle ranges with end index past the end of the array

Added by ferrous26 (Mark Rada) almost 12 years ago. Updated over 11 years ago.

Status:
Closed
Target version:
ruby -v:
trunk
Backport:
[ruby-core:43678]

Description

=begin
When I use Array#values_at I expect that it would be the same as successive calls to (({Array#[]})).

There is one case where this does not hold:

a = [0,1,2,3,4,5]
a[4..6] # => [4, 5]
a.values_at(4..6) # => [4,5,nil]

I think this is an inconsistency in the design of (({Array#values_at})). We can look at a more extreme case:

a[4..100] # => [4, 5]
a.values_at 4..100 # => [4, 5, nil]

And now it doesn't make any sense.

I think the best solution would be to make (({Array#values_at})) be equivalent to successive calls to (({Array#[]})). I have patched (({rb_range_beg_len()})) to handle the extra case and opened a pull request on github.
=end

Actions #1

Updated by nobu (Nobuyoshi Nakada) almost 12 years ago

  • Tracker changed from Bug to Feature

It's definitely not a bug.

Updated by nobu (Nobuyoshi Nakada) almost 12 years ago

  • Description updated (diff)

Updated by marcandre (Marc-Andre Lafortune) almost 12 years ago

Hi,

nobu (Nobuyoshi Nakada) wrote:

It's definitely not a bug.

It's not?
How do you explain:

[1, 2, 3].values_at(2...42) # => [3]
[1, 2, 3].values_at(2..41)  # => [3, nil]

I feel that both must return the same result.
Moreover, the only acceptable results are either [3] or [3, nil, nil, ... nil]

Actions #4

Updated by marcandre (Marc-Andre Lafortune) almost 12 years ago

  • Tracker changed from Feature to Bug

Moving back to "bug", as no explanation from Nobu.

Updated by marcandre (Marc-Andre Lafortune) almost 12 years ago

  • Assignee set to marcandre (Marc-Andre Lafortune)
  • Target version changed from 1.9.3 to 2.0.0
  • ruby -v set to trunk

The patch from Mark Rada never made it through, but I concur that the problem is in rb_range_beg_len.
I'll commit the fix below unless there's objection.
I've already fixed RubySpec:
https://github.com/rubyspec/rubyspec/commit/558a40edd5cd43a503641238cabe9f481ce9a723

diff --git a/range.c b/range.c
index 61fb643..aa0628d 100644
--- a/range.c
+++ b/range.c
@@ -746,16 +746,16 @@ rb_range_beg_len(VALUE range, long *begp, long *lenp, long len, int err)
if (beg < 0)
goto out_of_range;
}

  • if (end < 0)
  •   end += len;
    
  • if (!excl)
  •   end++;                  /* include end point */
    
    if (err == 0 || err == 2) {
    if (beg > len)
    goto out_of_range;
    if (end > len)
    end = len;
    }
  • if (end < 0)
  •   end += len;
    
  • if (!excl)
  •   end++;                  /* include end point */
    
    len = end - beg;
    if (len < 0)
    len = 0;

Updated by ferrous26 (Mark Rada) almost 12 years ago

marcandre (Marc-Andre Lafortune) wrote:

The patch from Mark Rada never made it through, but I concur that the problem is in rb_range_beg_len.

Hello Marc,

Sorry for not linking to the pull request here, though I did open a pull request on Github here: https://github.com/ruby/ruby/pull/109

Though I think your solution is much nicer. Perhaps the tests I added are still useful.

Updated by naruse (Yui NARUSE) almost 12 years ago

  • Status changed from Open to Assigned
  • Assignee changed from marcandre (Marc-Andre Lafortune) to matz (Yukihiro Matsumoto)

Is this a bug?

Updated by marcandre (Marc-Andre Lafortune) almost 12 years ago

Hi,

naruse (Yui NARUSE) wrote:

Is this a bug?

I'm very curious: how could it not be bug?
How would you explain that values_at(2...42) != values_at(2..41)?

I simply want to lighten the load for Matz; he has so many issues assigned to him or waiting on him already.

Updated by naruse (Yui NARUSE) almost 12 years ago

marcandre (Marc-Andre Lafortune) wrote:

I simply want to lighten the load for Matz; he has so many issues assigned to him or waiting on him already.

You must wait (or press) matz if a fix of an issue is clear and no other option of the new behavior.

Updated by ferrous26 (Mark Rada) almost 12 years ago

naruse (Yui NARUSE) wrote:

marcandre (Marc-Andre Lafortune) wrote:

I simply want to lighten the load for Matz; he has so many issues assigned to him or waiting on him already.

You must wait (or press) matz if a fix of an issue is clear and no other option of the new behavior.

It has been 2 weeks since the last update. Has this issue been forgotten?

Updated by matz (Yukihiro Matsumoto) over 11 years ago

  • Assignee changed from matz (Yukihiro Matsumoto) to nobu (Nobuyoshi Nakada)

The trailing nil must be a bug.

Matz.

Actions #12

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r36393.
Mark, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


array.c: fill with nil

Updated by marcandre (Marc-Andre Lafortune) over 11 years ago

matz (Yukihiro Matsumoto) wrote:

The trailing nil must be a bug.

Was there a reason to change the behavior of [].values_at(42..100)? Could this not lead to incompatibilities?

Nobu: is there a reason not to fix rb_range_beg_len also as per [ruby-core:43811]?

Updated by marcandre (Marc-Andre Lafortune) over 11 years ago

  • Status changed from Closed to Open

Marking as open, since fix doesn't seem to correspond to what Matz wrote.

marcandre (Marc-Andre Lafortune) wrote:

matz (Yukihiro Matsumoto) wrote:

The trailing nil must be a bug.

Was there a reason to change the behavior of [].values_at(42..100)? Could this not lead to incompatibilities?

Nobu: is there a reason not to fix rb_range_beg_len also as per [ruby-core:43811]?

Updated by marcandre (Marc-Andre Lafortune) over 11 years ago

Matz, could you please confirm what return value is correct for [].values_at(1..3)?

  1. It could be []. This would be compatible with Ruby 1.8 & 1.9.
  2. It could be [nil, nil, nil]. This is current behavior in trunk after Nobu's commit, but with potential incompatibility.

If 1, trunk must be changed
If 2, NEWS must list incompatibility.

Nobu: in either case, any reason not to commit my patch in [#5]?

Thanks

Updated by matz (Yukihiro Matsumoto) over 11 years ago

This is a spec change in 2.0 unless any big compatibility problem arise.
Array#values_at should be define as:

def values_at(*idx)
idx.map{|i| self[i]}
end

So it should be written in the NEWS file.

Matz.

Updated by marcandre (Marc-Andre Lafortune) over 11 years ago

  • Status changed from Open to Closed

Great, thanks.

NEWS updated.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0