[#JRUBY-1148] Array#== doesn't use custom to_ary method for RHS arguments

JRuby

Array#== doesn't use custom to_ary method for RHS arguments

Created: 15/Jun/07 08:12 AM Updated: 22/Dec/07 06:26 AM

Component/s:

Core Classes/Modules

Affects Version/s:

JRuby 1.0.0RC3

Fix Version/s:

JRuby 1.0.3, JRuby 1.1RC2

Time Tracking:

Not Specified

File Attachments:	1. jruby-1148.patch (0.4 kb) 2. RubyArray-JRUBY-1148.patch (0.6 kb)
Environment:	Solaris 10, Java 1.5, JRuby 2007-06-14 rev 3876

Description

« Hide

Array#== is supposed to use an object's to_ary method if it has one for purposes of Array#==:

class Foo
   def to_ary
      [1,2,3]
   end
end

f = Foo.new

[1,2,3] == f # Should be true

This is also a bug in MRI 1.8.6 p36. See the slightly mis-titled bug #11585 on RubyForge for more info.

All

Comments

Work Log

Change History

Sort Order:

Ascending order - Click to sort in descending order

[ Permlink | « Hide ]

Marcin Mielzynski - 17/Jun/07 03:22 PM

Shall it fail with NoMethodError or silently return false when to_ary not supplied for non Array arguments ?

[ Permlink | « Hide ]

Charles Oliver Nutter - 22/Oct/07 12:38 AM

Please re-test against trunk, 1.0 branch...if it's still an issue, we should fix it on both.

[ Permlink | « Hide ]

Daniel Berger - 23/Oct/07 07:19 AM

It's fixed in 1.1. Dunno about 1.0, since I don't have that version any longer.

Marcin, it should just return false. As a rule, boolean methods don't raise an error in MRI on an dumb comparison. They just return false.

[ Permlink | « Hide ]

Charles Oliver Nutter - 27/Oct/07 12:00 AM

It doesn't appear to be fixed in 1.1 for me. I hate diverging from MRI, even for bugs, but this one seems safe. I'll try to make the change now.

[ Permlink | « Hide ]

Hung Huynh - 28/Oct/07 03:17 PM

It's a simple patch. I just called to_ary on the RHS object before comparing it (only if it responses to "to_ary")

[ Permlink | « Hide ]

Marcin Mielzynski - 29/Oct/07 11:10 AM - edited

Hmm, the patch is ok as it matches Daniel's intentions but it breaks following test:

def test_ary
o = Object.new
def o.to_ary; end
def o.==(o); true; end
assert_equal(true, [].==(o))
end

Which assumes that having to_ary method is enough and then it goes for "==" comparison (but not on to_ary result, just on the argument itself).

[ Permlink | « Hide ]

Charles Oliver Nutter - 30/Oct/07 02:11 AM

Bumping off 1.0.2 since we don't have a safe, working patch.

[ Permlink | « Hide ]

Wirianto Djunaidi - 30/Oct/07 11:39 AM

It seems the patch work to me. I am not an expert in Ruby, so I'm still not sure what is the issue that Marcin mentioned.

From reading through the description of the bug it seems the expected behavior is when an array is test for equal against an object and the object has to_ary then it should compare against the result of the to_ary.

In the test_ary that Marcin mentioned being broken by the patch, I see 2 possibilites.
1. The patch is working as expected where [].==(o) is calling o.to_ary for the comparison.
Since o.to_ary is defined as def o.to_ary; end. It has nothing, so it must be returning nil which evaluate to false for the comparison.

2. Marcin mentioned that having to_ary method is enough and it should go for "==" comparison which I assume he expected the assertion to return true because it was defined in def o.==(o); true; end. But the definition is a singleton method for 'o' while the assertion is calling [].==(o) which will call '==' method of Array. If I assumed Marcin correctly, then the assertion should be assert_equal(true, o.==([]))

[ Permlink | « Hide ]

Marcin Mielzynski - 30/Oct/07 01:56 PM

One thing is sure, MRI is wrong, and we've followed MRI writing Array implementation. The puzzling thing is why MRI tests rely on such weird behavior.
Take a look at to_ary Rubinius implementation: http://git.rubini.us/?p=code;a=blob;f=kernel/core/array.rb;h=91ca08c67ca1ef7fe48ea81746c0c6cb8ed76022;hb=HEAD
I'd opt for changing the test and applying the patch.

[ Permlink | « Hide ]

Charles Oliver Nutter - 30/Oct/07 05:22 PM

Ok, this is going to stay bumped off 1.0.2, since the patch doesn't apply cleanly and I don't have time to monkey with it. I concur about changing the test and going with the patch, if that's appropriate. Can someone prepare patches for trunk and 1.0 branch that will do this?

[ Permlink | « Hide ]

Wirianto Djunaidi - 30/Oct/07 08:42 PM

How should the test be changed?

1. Change "def to_ary; end;" to "def to_ary; []; end"
or
2. Change the assertion "[].==(o)" to "o.==([])"

My guess will be option 1 since this is for testing the array comparison utilizing to_ary method.

[ Permlink | « Hide ]

Charles Oliver Nutter - 30/Oct/07 10:15 PM

I'd say option 1.

[ Permlink | « Hide ]

Wirianto Djunaidi - 26/Nov/07 12:39 AM

Attached 2nd patch file for changing the test case in test_array.rb as discussed and decided by Charles.

[ Permlink | « Hide ]

Thomas E Enebo - 26/Nov/07 03:31 PM

Fixed in commit 5058 on trunk and 5059 on 1.0 branch (patch by Hung Huynh, test patch by Wirianto Djunaidi)