Bug #6408

DelegateClass#eql? and <=> don't work as expected

Added by Aaron Patterson almost 2 years ago. Updated almost 2 years ago.

[ruby-core:44911]
Status:Assigned
Priority:Normal
Assignee:Aaron Patterson
Category:-
Target version:Next Major
ruby -v:ruby 2.0.0dev (2012-05-06 trunk 35548) [x86_64-darwin11.3.0] Backport:

Description

It seems these two methods aren't delegating to the delegate object when compared against itself.

I've attached a patch with tests and a fix. It seems nobody is the maintainer for delegate.rb (according to the wiki), so if nobody objects, I will apply this patch.

fix.patch Magnifier (1.45 KB) Aaron Patterson, 05/07/2012 04:48 AM

noname (500 Bytes) Anonymous, 05/07/2012 08:23 AM

noname (500 Bytes) Anonymous, 05/20/2012 05:29 AM


Related issues

Duplicated by ruby-trunk - Bug #7045: DelegateClass array subtraction Closed 09/22/2012

History

#1 Updated by Marc-Andre Lafortune almost 2 years ago

Hi,

In your patch, for both if obj.equal? self, shouldn't it be if obj.is_a? Delegator?

For many classes that define their comparison operators, this might not be sufficient if you want comparison operators to work well, as they typically will first check on the class of the object to compare with.

Defining is_a? to return true for either Delegator or the class delegated to might be more helpful.

tenderlovemaking (Aaron Patterson) wrote:

It seems these two methods aren't delegating to the delegate object when compared against itself.

I've attached a patch with tests and a fix. It seems nobody is the maintainer for delegate.rb (according to the wiki), so if nobody objects, I will apply this patch.

#2 Updated by Marc-Andre Lafortune almost 2 years ago

marcandre (Marc-Andre Lafortune) wrote:

Hi,

In your patch, for both if obj.equal? self, shouldn't it be if obj.is_a? Delegator?

Or if you did mean obj.equal? self, then you can return true / 0, at least that what != and == are doing. Better not delegate to NaN :-)

#3 Updated by Anonymous almost 2 years ago

On Mon, May 07, 2012 at 07:15:34AM +0900, marcandre (Marc-Andre Lafortune) wrote:

Issue #6408 has been updated by marcandre (Marc-Andre Lafortune).

Hi,

In your patch, for both if obj.equal? self, shouldn't it be if obj.is_a? Delegator?

For many classes that define their comparison operators, this might not be sufficient if you want comparison operators to work well, as they typically will first check on the class of the object to compare with.

Defining is_a? to return true for either Delegator or the class delegated to might be more helpful.

I was thinking that too, but the current implementation of != and ==
don't do the is_a? check. Maybe those should be changed?

Anyway, I don't know about other changes, but the failing tests I have
in this patch are causing issues for some rails users:

https://github.com/rails/rails/issues/5974

I feel like we probably need better tests surrounding the "correct"
behavior of DelegateClass, but I'd rather not shave that yak at the
moment and just fix the issues we're having today. :)

--
Aaron Patterson
http://tenderlovemaking.com/

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

Hi,

I was thinking that too, but the current implementation of != and ==
don't do the is_a? check. Maybe those should be changed?

Anyway, I don't know about other changes, but the failing tests I have
in this patch are causing issues for some rails users:

https://github.com/rails/rails/issues/5974

I feel like we probably need better tests surrounding the "correct"
behavior of DelegateClass, but I'd rather not shave that yak at the
moment and just fix the issues we're having today. :)

Sounds good. In any case, +1 from me.

== and != should also be modified to call self == self and self != self in case other.equal?(self), even though the only object I know of that is not == to itself is NaN...

#5 Updated by Jeremy Evans almost 2 years ago

On 05/07 07:15, marcandre (Marc-Andre Lafortune) wrote:

Hi,

In your patch, for both if obj.equal? self, shouldn't it be if obj.is_a? Delegator?

For many classes that define their comparison operators, this might not be sufficient if you want comparison operators to work well, as they typically will first check on the class of the object to compare with.

Defining is_a? to return true for either Delegator or the class delegated to might be more helpful.

When I use DelegateClass, it's often because I want a proxy object
that mostly acts like the given class, but is specifically not treated
as the given class (in terms of the is_a? and === methods).

Changing this behavior would likely break existing code. So if this to
be considered, it should probably not change until 3.0 (unless matz
makes an exception for this case).

Jeremy

#6 Updated by Marc-Andre Lafortune almost 2 years ago

Hi,

jeremyevans (Jeremy Evans) wrote:

Changing this behavior would likely break existing code. So if this to
be considered, it should probably not change until 3.0 (unless matz
makes an exception for this case).

I had misunderstood that the goal was to make equality symmetric. In any case, you're right, I don't think it should be considered.

#7 Updated by Yusuke Endoh almost 2 years ago

  • Status changed from Open to Assigned
  • Assignee set to Aaron Patterson
  • Target version set to Next Major

Hello, Aaron

What do you think about Jeremy's opinion?

I'm just wondering but why do you want to delegate #eql? ?
I guess that is because you are inserting Delegate objects
to a Hash. Such a code is still dangerous even if the
patch is applied:

require "delegate"

class Foo; end
class Bar < DelegateClass(Foo); end
foo = Foo.new
bar = Bar.new(foo)

p foo.eql?(foo) #=> true
p bar.eql?(foo) #=> true
p bar.eql?(bar) #=> false (this returns true with your patch)

p foo.eql?(bar) #=> false (this is NOT fixed)

h = { bar => 42 }
p h[foo] #=> nil, not 42

I think it is difficult to "fix."

The same holds for #<=>.
You should not sort an Array that includes Delegate objects.

Yusuke Endoh mame@tsg.ne.jp

#8 Updated by Anonymous almost 2 years ago

On Thu, May 17, 2012 at 12:10:56AM +0900, mame (Yusuke Endoh) wrote:

Issue #6408 has been updated by mame (Yusuke Endoh).

Status changed from Open to Assigned
Assignee set to tenderlovemaking (Aaron Patterson)
Target version set to 3.0

Hello, Aaron

What do you think about Jeremy's opinion?

I agree with Jeremy's opinion. I think that changing to is_a? would
probably be be better, but I think my patch should be applied for Ruby
2.0.

I'm just wondering but why do you want to delegate #eql? ?
I guess that is because you are inserting Delegate objects
to a Hash. Such a code is still dangerous even if the
patch is applied:

require "delegate"

class Foo; end
class Bar < DelegateClass(Foo); end
foo = Foo.new
bar = Bar.new(foo)

p foo.eql?(foo) #=> true
p bar.eql?(foo) #=> true
p bar.eql?(bar) #=> false (this returns true with your patch)

Yes, this is the bug I'm trying to fix.

p foo.eql?(bar) #=> false (this is NOT fixed)

Yes, you are correct. I'm not sure how or if we should fix this.

h = { bar => 42 }
p h[foo] #=> nil, not 42

I think it is difficult to "fix."

It's difficult to "fix" 100%, yes. However, we can at least reduce our
broken windows. :-)

The same holds for #<=>.
You should not sort an Array that includes Delegate objects.

I agree. The problem is that people can create delegate objects, then
feed the delegate object to third party code (say some gem, or some
library in stdlib) and expect it to work.

I think we should either make it work as best we can, or remove
DelegateClass from stdlib. It should not be a requirement that
DelegateClass users understand how all third party code interacts with
their delegate object (I think).

--
Aaron Patterson
http://tenderlovemaking.com/

Also available in: Atom PDF