https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112012-05-07T07:15:33ZRuby Issue Tracking SystemRuby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=264982012-05-07T07:15:33Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<p>In your patch, for both <code>if obj.equal? self</code>, shouldn't it be <code>if obj.is_a? Delegator</code>?</p>
<p>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.</p>
<p>Defining <code>is_a?</code> to return true for either Delegator or the class delegated to might be more helpful.</p>
<p>tenderlovemaking (Aaron Patterson) wrote:</p>
<blockquote>
<p>It seems these two methods aren't delegating to the delegate object when compared against itself.</p>
<p>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.</p>
</blockquote> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=264992012-05-07T07:24:27Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>marcandre (Marc-Andre Lafortune) wrote:</p>
<blockquote>
<p>Hi,</p>
<p>In your patch, for both <code>if obj.equal? self</code>, shouldn't it be <code>if obj.is_a? Delegator</code>?</p>
</blockquote>
<p>Or if you did mean <code>obj.equal? self</code>, then you can return true / 0, at least that what != and == are doing. Better not delegate to NaN :-)</p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=265002012-05-07T08:23:22ZAnonymous
<ul><li><strong>File</strong> <a href="/attachments/2664">noname</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/2664/noname">noname</a> added</li></ul><p>On Mon, May 07, 2012 at 07:15:34AM +0900, marcandre (Marc-Andre Lafortune) wrote:</p>
<blockquote>
<p>Issue <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: DelegateClass#eql? and <=> don't work as expected (Closed)" href="https://bugs.ruby-lang.org/issues/6408">#6408</a> has been updated by marcandre (Marc-Andre Lafortune).</p>
<p>Hi,</p>
<p>In your patch, for both <code>if obj.equal? self</code>, shouldn't it be <code>if obj.is_a? Delegator</code>?</p>
<p>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.</p>
<p>Defining <code>is_a?</code> to return true for either Delegator or the class delegated to might be more helpful.</p>
</blockquote>
<p>I was thinking that too, but the current implementation of != and ==<br>
don't do the is_a? check. Maybe those should be changed?</p>
<p>Anyway, I don't know about other changes, but the failing tests I have<br>
in this patch are causing issues for some rails users:</p>
<p><a href="https://github.com/rails/rails/issues/5974" class="external">https://github.com/rails/rails/issues/5974</a></p>
<p>I feel like we probably need better tests surrounding the "correct"<br>
behavior of DelegateClass, but I'd rather not shave that yak at the<br>
moment and just fix the issues we're having today. :)</p>
<p>--<br>
Aaron Patterson<br>
<a href="http://tenderlovemaking.com/" class="external">http://tenderlovemaking.com/</a></p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=265012012-05-07T08:34:57Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<blockquote>
<p>I was thinking that too, but the current implementation of != and ==<br>
don't do the is_a? check. Maybe those should be changed?</p>
<p>Anyway, I don't know about other changes, but the failing tests I have<br>
in this patch are causing issues for some rails users:</p>
<p><a href="https://github.com/rails/rails/issues/5974" class="external">https://github.com/rails/rails/issues/5974</a></p>
<p>I feel like we probably need better tests surrounding the "correct"<br>
behavior of DelegateClass, but I'd rather not shave that yak at the<br>
moment and just fix the issues we're having today. :)</p>
</blockquote>
<p>Sounds good. In any case, +1 from me.</p>
<p>== and != should also be modified to call self == self and self != self in case <code>other.equal?(self)</code>, even though the only object I know of that is not == to itself is NaN...</p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=265022012-05-07T09:53:16Zjeremyevans (Jeremy Evans)code@jeremyevans.net
<ul></ul><p>On 05/07 07:15, marcandre (Marc-Andre Lafortune) wrote:</p>
<blockquote>
<p>Hi,</p>
<p>In your patch, for both <code>if obj.equal? self</code>, shouldn't it be <code>if obj.is_a? Delegator</code>?</p>
<p>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.</p>
<p>Defining <code>is_a?</code> to return true for either Delegator or the class delegated to might be more helpful.</p>
</blockquote>
<p>When I use DelegateClass, it's often because I want a proxy object<br>
that mostly acts like the given class, but is specifically not treated<br>
as the given class (in terms of the is_a? and === methods).</p>
<p>Changing this behavior would likely break existing code. So if this to<br>
be considered, it should probably not change until 3.0 (unless matz<br>
makes an exception for this case).</p>
<p>Jeremy</p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=265032012-05-07T10:20:57Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<p>jeremyevans (Jeremy Evans) wrote:</p>
<blockquote>
<p>Changing this behavior would likely break existing code. So if this to<br>
be considered, it should probably not change until 3.0 (unless matz<br>
makes an exception for this case).</p>
</blockquote>
<p>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.</p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=266582012-05-17T00:10:51Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Assigned</i></li><li><strong>Assignee</strong> set to <i>tenderlovemaking (Aaron Patterson)</i></li><li><strong>Target version</strong> set to <i>3.0</i></li></ul><p>Hello, Aaron</p>
<p>What do you think about Jeremy's opinion?</p>
<p>I'm just wondering but why do you want to delegate #eql? ?<br>
I guess that is because you are inserting Delegate objects<br>
to a Hash. Such a code is still dangerous even if the<br>
patch is applied:</p>
<p>require "delegate"</p>
<p>class Foo; end<br>
class Bar < DelegateClass(Foo); end<br>
foo = Foo.new<br>
bar = Bar.new(foo)</p>
<p>p foo.eql?(foo) #=> true<br>
p bar.eql?(foo) #=> true<br>
p bar.eql?(bar) #=> false (this returns true with your patch)</p>
<p>p foo.eql?(bar) #=> false (this is NOT fixed)</p>
<p>h = { bar => 42 }<br>
p h[foo] #=> nil, not 42</p>
<p>I think it is difficult to "fix."</p>
<p>The same holds for #<=>.<br>
You should not sort an Array that includes Delegate objects.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=267202012-05-20T05:29:13ZAnonymous
<ul><li><strong>File</strong> <a href="/attachments/2702">noname</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/2702/noname">noname</a> added</li></ul><p>On Thu, May 17, 2012 at 12:10:56AM +0900, mame (Yusuke Endoh) wrote:</p>
<blockquote>
<p>Issue <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: DelegateClass#eql? and <=> don't work as expected (Closed)" href="https://bugs.ruby-lang.org/issues/6408">#6408</a> has been updated by mame (Yusuke Endoh).</p>
<p>Status changed from Open to Assigned<br>
Assignee set to tenderlovemaking (Aaron Patterson)<br>
Target version set to 3.0</p>
<p>Hello, Aaron</p>
<p>What do you think about Jeremy's opinion?</p>
</blockquote>
<p>I agree with Jeremy's opinion. I think that changing to is_a? would<br>
probably be be better, but I think my patch should be applied for Ruby<br>
2.0.</p>
<blockquote>
<p>I'm just wondering but why do you want to delegate #eql? ?<br>
I guess that is because you are inserting Delegate objects<br>
to a Hash. Such a code is still dangerous even if the<br>
patch is applied:</p>
<p>require "delegate"</p>
<p>class Foo; end<br>
class Bar < DelegateClass(Foo); end<br>
foo = Foo.new<br>
bar = Bar.new(foo)</p>
<p>p foo.eql?(foo) #=> true<br>
p bar.eql?(foo) #=> true<br>
p bar.eql?(bar) #=> false (this returns true with your patch)</p>
</blockquote>
<p>Yes, this is the bug I'm trying to fix.</p>
<blockquote>
<p>p foo.eql?(bar) #=> false (this is NOT fixed)</p>
</blockquote>
<p>Yes, you are correct. I'm not sure how or if we should fix this.</p>
<blockquote>
<p>h = { bar => 42 }<br>
p h[foo] #=> nil, not 42</p>
<p>I think it is difficult to "fix."</p>
</blockquote>
<p>It's difficult to "fix" 100%, yes. However, we can at least reduce our<br>
broken windows. :-)</p>
<blockquote>
<p>The same holds for #<=>.<br>
You should not sort an Array that includes Delegate objects.</p>
</blockquote>
<p>I agree. The problem is that people can create delegate objects, then<br>
feed the delegate object to third party code (say some gem, or some<br>
library in stdlib) and expect it to work.</p>
<p>I think we should either make it work as best we can, or remove<br>
DelegateClass from stdlib. It should not be a requirement that<br>
DelegateClass users understand how all third party code interacts with<br>
their delegate object (I think).</p>
<p>--<br>
Aaron Patterson<br>
<a href="http://tenderlovemaking.com/" class="external">http://tenderlovemaking.com/</a></p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=593172016-06-23T03:52:41Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>I was bitten by something similar, expecting equal? to work correctly between non-delegate and delegate instances. Where are we at with this patch?</p> Ruby master - Bug #6408: DelegateClass#eql? and <=> don't work as expectedhttps://bugs.ruby-lang.org/issues/6408?journal_id=813682019-09-03T01:05:37Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>Status</strong> changed from <i>Assigned</i> to <i>Closed</i></li></ul><p>Delegate now handles <code>eql?</code> after changes from <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Delegator#eql? missing (Closed)" href="https://bugs.ruby-lang.org/issues/12684">#12684</a>. However, it always returns true if the object is the same:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">nan</span> <span class="o">=</span> <span class="mi">0</span><span class="o">/</span><span class="mf">0.0</span>
<span class="n">nan</span><span class="p">.</span><span class="nf">eql?</span> <span class="n">nan</span> <span class="c1"># false</span>
<span class="n">s</span> <span class="o">=</span> <span class="no">SimpleDelegator</span><span class="p">.</span><span class="nf">new</span><span class="p">(</span><span class="n">nan</span><span class="p">)</span>
<span class="n">s</span><span class="p">.</span><span class="nf">eql?</span> <span class="n">s</span> <span class="c1"># true</span>
</code></pre>
<p>I'm not sure it is worth it to add explicit support for making it so <code>eql?</code> can return false if the target doesn't consider itself to be equal to itself.</p>
<p>As mame mentioned, <code><=></code> is for sorting, and it probably isn't helpful to have a sorting method that handles multiple copies of the same delegate object in an array, but not multiple different delegates of the same target in an array.</p>
<p>Considering the <code>eql?</code> issue has been fixed, which was the underlying cause of the Rails issue that caused this ticket to be filed, I think this is safe to close. If I'm wrong, please reopen.</p>