JRuby

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

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.0.0RC3
  • Fix Version/s: JRuby 1.0.3, JRuby 1.1RC2
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    Solaris 10, Java 1.5, JRuby 2007-06-14 rev 3876
  • Number of attachments :
    2

Description

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.

Attachments

  1. Text File
    jruby-1148.patch
    26/Nov/07 12:39 AM
    0.4 kB
    Wirianto Djunaidi
  2. Text File
    RubyArray-JRUBY-1148.patch
    28/Oct/07 3:17 PM
    0.6 kB
    Hung Huynh

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Marcin Mielzynski added a comment -

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

Show
Marcin Mielzynski added a comment - Shall it fail with NoMethodError or silently return false when to_ary not supplied for non Array arguments ?
Hide
Permalink
Charles Oliver Nutter added a comment -

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

Show
Charles Oliver Nutter added a comment - Please re-test against trunk, 1.0 branch...if it's still an issue, we should fix it on both.
Hide
Permalink
Daniel Berger added a comment -

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.

Show
Daniel Berger added a comment - 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.
Hide
Permalink
Charles Oliver Nutter added a comment -

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.

Show
Charles Oliver Nutter added a comment - 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.
Hide
Permalink
Hung Huynh added a comment -

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

Show
Hung Huynh added a comment - It's a simple patch. I just called to_ary on the RHS object before comparing it (only if it responses to "to_ary")
Hide
Permalink
Marcin Mielzynski added a comment - - 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).

Show
Marcin Mielzynski added a comment - - 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).
Hide
Permalink
Charles Oliver Nutter added a comment -

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

Show
Charles Oliver Nutter added a comment - Bumping off 1.0.2 since we don't have a safe, working patch.
Hide
Permalink
Wirianto Djunaidi added a comment -

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.==([]))

Show
Wirianto Djunaidi added a comment - 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.==([]))
Hide
Permalink
Marcin Mielzynski added a comment -

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.

Show
Marcin Mielzynski added a comment - 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.
Hide
Permalink
Charles Oliver Nutter added a comment -

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?

Show
Charles Oliver Nutter added a comment - 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?
Hide
Permalink
Wirianto Djunaidi added a comment -

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.

Show
Wirianto Djunaidi added a comment - 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.
Hide
Permalink
Charles Oliver Nutter added a comment -

I'd say option 1.

Show
Charles Oliver Nutter added a comment - I'd say option 1.
Hide
Permalink
Wirianto Djunaidi added a comment -

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

Show
Wirianto Djunaidi added a comment - Attached 2nd patch file for changing the test case in test_array.rb as discussed and decided by Charles.
Hide
Permalink
Thomas E Enebo added a comment -

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

Show
Thomas E Enebo added a comment - Fixed in commit 5058 on trunk and 5059 on 1.0 branch (patch by Hung Huynh, test patch by Wirianto Djunaidi)

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.