Bug #6203

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

Added by Mark Rada about 2 years ago. Updated over 1 year ago.

[ruby-core:43678]
Status:Closed
Priority:Normal
Assignee:Nobuyoshi Nakada
Category:core
Target version:2.0.0
ruby -v:trunk Backport:

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#valuesat})) be equivalent to successive calls to (({Array#[]})). I have patched (({rbrangebeglen()})) to handle the extra case and opened a pull request on github.
=end

Associated revisions

Revision 36393
Added by Nobuyoshi Nakada almost 2 years ago

array.c: fill with nil

  • array.c (rbgetvalues_at): fill with nil out of range. [Bug #6203]

Revision 37769
Added by Marc-Andre Lafortune over 1 year ago

  • NEWS: List incompatible change for Array#values_at [#6203]

Revision 37775
Added by Marc-Andre Lafortune over 1 year ago

  • range.c (rbrangebeg_len): Fix potential bug for limit case [#6203]

History

#1 Updated by Nobuyoshi Nakada about 2 years ago

  • Tracker changed from Bug to Feature

It's definitely not a bug.

#2 Updated by Nobuyoshi Nakada about 2 years ago

  • Description updated (diff)

#3 Updated by Marc-Andre Lafortune about 2 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]

#4 Updated by Marc-Andre Lafortune about 2 years ago

  • Tracker changed from Feature to Bug

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

#5 Updated by Marc-Andre Lafortune about 2 years ago

  • Assignee set to 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 rbrangebeg_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 @@ rbrangebeglen(VALUE range, long *begp, long *lenp, long len, int err)
if (beg < 0)
goto out
ofrange;
}
+ 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;

#6 Updated by Mark Rada about 2 years ago

marcandre (Marc-Andre Lafortune) wrote:

The patch from Mark Rada never made it through, but I concur that the problem is in rbrangebeg_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.

#7 Updated by Yui NARUSE about 2 years ago

  • Status changed from Open to Assigned
  • Assignee changed from Marc-Andre Lafortune to Yukihiro Matsumoto

Is this a bug?

#8 Updated by Marc-Andre Lafortune about 2 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.

#9 Updated by Yui NARUSE about 2 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.

#10 Updated by Mark Rada about 2 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?

#11 Updated by Yukihiro Matsumoto almost 2 years ago

  • Assignee changed from Yukihiro Matsumoto to Nobuyoshi Nakada

The trailing nil must be a bug.

Matz.

#12 Updated by Nobuyoshi Nakada almost 2 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

  • array.c (rbgetvalues_at): fill with nil out of range. [Bug #6203]

#13 Updated by Marc-Andre Lafortune almost 2 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 ?

#14 Updated by Marc-Andre Lafortune almost 2 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 ?

#15 Updated by Marc-Andre Lafortune over 1 year 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

#16 Updated by Yukihiro Matsumoto over 1 year 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.

#17 Updated by Marc-Andre Lafortune over 1 year ago

  • Status changed from Open to Closed

Great, thanks.

NEWS updated.

Also available in: Atom PDF