https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?14253458252013-01-12T18:34:56ZRuby Issue Tracking SystemRuby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=353722013-01-12T18:34:56ZKenta Muratamuraken@gmail.com
<ul><li><strong>Category</strong> set to <i>core</i></li><li><strong>Assignee</strong> set to <i>Yukihiro Matsumoto</i></li><li><strong>Target version</strong> changed from <i>2.0.0</i> to <i>next minor</i></li></ul> Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=353732013-01-12T18:35:08ZKenta Muratamuraken@gmail.com
<ul><li><strong>Tracker</strong> changed from <i>Bug</i> to <i>Feature</i></li></ul> Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=389522013-04-27T18:28:32ZBenoit Daloze
<ul></ul><p>Hello,</p>
<p>I think this is really a bug: error hiding <em>is</em> harmful.<br>
Anyway, is it OK to commit this to trunk now that 2.0 is released and in a separate branch?</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=391092013-05-03T22:23:01ZYukihiro Matsumotomatz@ruby-lang.org
<ul></ul><p>Show us the patch first. I am afraid I misunderstand you.</p>
<p>Matz</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=391112013-05-04T06:23:00ZBenoit Daloze
<ul></ul><p>matz (Yukihiro Matsumoto) wrote:</p>
<blockquote>
<p>Show us the patch first. I am afraid I misunderstand you.</p>
</blockquote>
<p>Sorry, I was not clear.</p>
<p>My intent is to remove error hiding, that is not reporting in any way exceptions.</p>
<p>In the case of Range checking and coerce, it simply calls rb_check_funcall() instead of rb_funcall() + rb_rescue() so exceptions raised in these methods are not swallowed (but no exception is raised if #coerce is not defined or returns an invalid result as before, that behavior is preserved).<br>
For #&lt;=&gt;, errors are no more caught silently by rb_rescue(), so bugs lurking in #&lt;=&gt; methods are shown.<br>
Only some RDoc tests do not pass (because of bugs of #&lt;=&gt;). And one test for Comparable is logically changed.</p>
<p>Patches can be seen at <a href="https://github.com/eregon/ruby/compare/no_hidden_rescue_essential">https://github.com/eregon/ruby/compare/no_hidden_rescue_essential</a><br>
or <a href="https://github.com/eregon/ruby/compare/no_hidden_rescue_essential.patch">https://github.com/eregon/ruby/compare/no_hidden_rescue_essential.patch</a> .</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=391232013-05-04T23:22:15ZYukihiro Matsumotomatz@ruby-lang.org
<ul></ul><p>I agree with most of your changes in the patch, especially using rb_check_funcall instead of rb_rescue.<br>
But I personally dislike the equal operator (==) to raise error, since equal comparison is so fundamental, and most of us write code that do not expect exceptions from == operator.</p>
<p>Matz.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=391252013-05-05T01:24:01ZBenoit Daloze
<ul></ul><p>Hello,</p>
<p>matz (Yukihiro Matsumoto) wrote:</p>
<blockquote>
<p>I agree with most of your changes in the patch, especially using rb_check_funcall instead of rb_rescue.<br>
But I personally dislike the equal operator (==) to raise error, since equal comparison is so fundamental, and most of us write code that do not expect exceptions from == operator.</p>
<p>Matz.</p>
</blockquote>
<p>Many classes including Comparable already define #== themselves and most definitions of #== are made by the user or libraries (not using Comparable#==), so I think this rescue clause protects only few users and I believe #== methods should in any case be written to support comparison with other objects without raising an exception (unless explicitly intended).</p>
<p>On the other hand, if #&lt;=&gt; method does raise an exception, I would find it useful as it tells me I am probably comparing things I did not intend to (e.g.: objects of different classes/hierarchies). And it is more consistent with other uses of #&lt;=&gt; (and other definitions of #==).</p>
<p>Finding why my objects are never #== because I made a typo or some sensible error in #&lt;=&gt; might require a lot more time to debug than just seeing the exception reported in the #&lt;=&gt; of #==, which is straightforward to fix.</p>
<p>About writing code not expecting exceptions, I think most code is in this case and the fail-fast principle is great in Ruby. As #== is a method, I think it should not be treated specially even if from a mathematical or logical point of view it should never raise any exception (that being left to the programmer).</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=392762013-05-13T00:17:23ZBenoit Daloze
<ul></ul><p>matz, what do you think?</p>
<p>Are you against introducing the change for Comparable#== ?</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=405042013-07-15T20:12:27ZJulien Aschmurfy@gmail.com
<ul></ul><p>I just got bitten by this problem and I agree there should not be any hidden rescue misleading you on what is really happening in your code, that&#39;s an horrible idea....<br>
As pointed you don&#39;t usually except an exception to be raised anywhere in the code that&#39;s why they are called exceptions after all, I don&#39;t see why == should be any different since it&#39;s just a method like any other.</p>
<p>I think the bare minimum is to add a big or even huge warning in the Comparable module documentation so anyone who find this behavior stupid can use something else instead.</p>
<p>PS: If I sound a little aggressive that&#39;s because I just spent half an hour looking for a problem which was not at all where I was looking thanks to this hidden rescue...</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=423252013-10-08T09:27:05ZPatrick Touektoric@yahoo.com
<ul></ul><p><a href="http://rubyforge.org/tracker/index.php?func=detail&amp;aid=17368&amp;group_id=426&amp;atid=1698">http://rubyforge.org/tracker/index.php?func=detail&amp;aid=17368&amp;group_id=426&amp;atid=1698</a></p>
<p>Things have &quot;improved&quot; since 2008. If the &lt;=&gt; includes a raise, #== now also raises the exception. Unfortunately, if there is some other exception (such as a syntax error!), it continues to hide this and coerce it down to false.</p>
<p>Note that other Comparable methods (Comparable#&gt;=, #&gt;, #&lt;, etc.), all those others do raise the error. Only #== hides this and silently returns false.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=423332013-10-08T21:14:21ZBenoit Daloze
<ul></ul><p>ektoric (Patrick Tou) wrote:</p>
<blockquote>
<p><a href="http://rubyforge.org/tracker/index.php?func=detail&amp;aid=17368&amp;group_id=426&amp;atid=1698">http://rubyforge.org/tracker/index.php?func=detail&amp;aid=17368&amp;group_id=426&amp;atid=1698</a></p>
</blockquote>
<p>Thanks for the link.</p>
<blockquote>
<p>Things have &quot;improved&quot; since 2008. If the &lt;=&gt; includes a raise, #== now also raises the exception. Unfortunately, if there is some other exception (such as a syntax error!), it continues to hide this and coerce it down to false.</p>
</blockquote>
<p>Seems to me &lt;=&gt; including a &quot;raise&quot; still hides it:</p>
<pre>class C; include Comparable; def &lt;=&gt;(o); raise &#39;stop!&#39;; end; end; C.new == C.new # =&gt; false
</pre>
<blockquote>
<p>Note that other Comparable methods (Comparable#&gt;=, #&gt;, #&lt;, etc.), all those others do raise the error. Only #== hides this and silently returns false.</p>
</blockquote>
<p>Yes, indeed, it makes no sense to me.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=434262013-12-05T20:58:31ZYukihiro Matsumotomatz@ruby-lang.org
<ul></ul><p>It&#39;s quite difficult to predict what would happen if we remove error hiding.<br>
So I agree with starting experiment to see if we will have any problem.</p>
<p>But I think it&#39;s too late to add that kind of change for Ruby 2.1, so how about starting experiment soon after 2.1 release in later this month.</p>
<p>Matz.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=434682013-12-07T05:47:44ZBenoit Daloze
<ul></ul><p>OK, I will commit this as an experiment in early 2014.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=434692013-12-07T05:48:25ZBenoit Daloze
<ul><li><strong>Assignee</strong> changed from <i>Yukihiro Matsumoto</i> to <i>Benoit Daloze</i></li></ul> Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=441982014-01-10T05:13:52ZAaron Pattersontenderlove@ruby-lang.org
<ul></ul><p><a href="https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/44502" class="changeset" title="* compar.c (cmp_equal): remove error hiding in Comparable#==. Comparable#== no longer rescues e...">r44502</a> makes the Rails tests fail spectacularly. We have &lt;=&gt; implementations that raise exceptions and expect == to swallow them. We probably shouldn&#39;t be raising exceptions in these methods, but this change definitely breaks our tests.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=441992014-01-10T06:02:59ZBenoit Daloze
<ul></ul><p>@tenderlove These are probably bugs then, is it not? I will try to have a look.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=442002014-01-10T07:23:24ZAaron Pattersontenderlove@ruby-lang.org
<ul></ul><p>On Fri, Jan 10, 2014 at 06:03:03AM +0900, Eregon (Benoit Daloze) wrote:</p>
<blockquote>
<p>Issue <a href="https://bugs.ruby-lang.org/issues/7688" class="issue tracker-2 status-1 priority-4 priority-default" title="Error hiding with rb_rescue() on Comparable#==, #coerce and others (Open)">#7688</a> has been updated by Eregon (Benoit Daloze).</p>
<p>@tenderlove These are probably bugs then, is it not? I will try to have a look.</p>
</blockquote>
<p>I can&#39;t say for sure whether or not it&#39;s bugs, but I can say I don&#39;t<br>
really like this change.</p>
<p>Say you write a class like this:</p>
<p>class MyObject<br>
include Comparable<br>
def &lt;=&gt; other<br>
raise ArgumentError unless other.is_a?(MyObject)<br>
# Do some comparisons<br>
end<br>
end</p>
<p>I raise an argument error because they are not comparable. To me it<br>
implies that <code>self</code> and <code>other</code> are also not equal, but not that <code>==</code><br>
should raise an exception.</p>
<p>I&#39;d never expect this to raise an exception regardless of the<br>
implementation of &lt;=&gt;:</p>
<p>MyObject.new != 10</p>
<p>IOW it seems like &lt;=&gt; is to ==, what respond_to? is to method_missing.<br>
Anyway, I&#39;m not a <em>huge</em> fan, but this does break our tests (though I<br>
can fix them). It seems like I would have to implement <code>==</code> with<br>
exactly the same logic as <code>&lt;=&gt;</code>, except return nil (to indicate it isn&#39;t<br>
comparable) instead of raise an exception (which is exactly what == does<br>
before this change).</p>
<p>Could we find a middle ground and just rescue ArgumentError? Or some<br>
sort of NonComparable error?</p>
<p>-- <br>
Aaron Patterson<br>
<a href="http://tenderlovemaking.com/">http://tenderlovemaking.com/</a></p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=442012014-01-10T07:35:13ZMarc-Andre Lafortuneruby-core@marc-andre.ca
<ul></ul><p>The method <code>&lt;=&gt;</code> should return <code>nil</code> for objects that are not comparable, not raise errors.</p>
<p>So this seems to be a misunderstanding/bug in Rails.</p>
<p>It might be best to add a warning to Ruby 2.2 if an exception is caught by <code>==</code> and we can not intercept it in 2.3?</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=442022014-01-10T07:42:40ZMarc-Andre Lafortuneruby-core@marc-andre.ca
<ul></ul><p>tenderlovemaking (Aaron Patterson) wrote:</p>
<blockquote>
<p>It seems like I would have to implement <code>==</code> with<br>
exactly the same logic as <code>&lt;=&gt;</code>, except return nil (to indicate it isn&#39;t<br>
comparable) instead of raise an exception (which is exactly what == does<br>
before this change).</p>
</blockquote>
<p>Not sure I follow... You don&#39;t have to implement <code>==</code> at all in your example.<br>
Moreover <code>==</code> should not return <code>nil</code>, it is <code>&lt;=&gt;</code> that should return <code>nil</code>.<br>
Neither <code>==</code>, <code>!=</code> nor <code>&lt;=&gt;</code> should ever raise exceptions.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=443822014-01-16T22:07:08ZAaron Pattersontenderlove@ruby-lang.org
<ul></ul><p>This also broke the RDoc tests. Since RDoc was broken, I couldn&#39;t install gems:</p>
<p><a href="https://github.com/rdoc/rdoc/issues/284">https://github.com/rdoc/rdoc/issues/284</a></p>
<p>Can we <em>please</em> consider issuing a warning in trunk rather than raising? I think raising is fine in the future, but removing functionality without giving people notice seems bad. I understand that some people don&#39;t read warnings, but I don&#39;t think throwing our hands in the air and giving up on warnings is the right course of action.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=443902014-01-17T01:35:48ZZachary Scotte@zzak.io
<ul></ul><blockquote>
<p>Can we please consider issuing a warning in trunk rather than raising?</p>
</blockquote>
<p>+1, I&#39;m glad this was only added to trunk and not released by 2.1, we should be warning before completely changing the feature.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=443982014-01-17T12:09:40ZBenoit Daloze
<ul></ul><p>Aaron Patterson wrote:</p>
<blockquote>
<p>This also broke the RDoc tests. Since RDoc was broken, I couldn&#39;t install gems:</p>
<p><a href="https://github.com/rdoc/rdoc/issues/284">https://github.com/rdoc/rdoc/issues/284</a></p>
<p>Can we <em>please</em> consider issuing a warning in trunk rather than raising? I think raising is fine in the future, but removing functionality without giving people notice seems bad. I understand that some people don&#39;t read warnings, but I don&#39;t think throwing our hands in the air and giving up on warnings is the right course of action.</p>
</blockquote>
<p>I will add a warning then and still rescue until next release, thanks for the feedback!</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=443992014-01-17T17:42:11ZAaron Pattersontenderlove@ruby-lang.org
<ul></ul><p>On Fri, Jan 17, 2014 at 12:09:41PM +0000, <a href="mailto:eregontp@gmail.com">eregontp@gmail.com</a> wrote:</p>
<blockquote>
<p>Issue <a href="https://bugs.ruby-lang.org/issues/7688" class="issue tracker-2 status-1 priority-4 priority-default" title="Error hiding with rb_rescue() on Comparable#==, #coerce and others (Open)">#7688</a> has been updated by Benoit Daloze.</p>
<p>Aaron Patterson wrote:</p>
<blockquote>
<p>This also broke the RDoc tests. Since RDoc was broken, I couldn&#39;t install gems:</p>
<p><a href="https://github.com/rdoc/rdoc/issues/284">https://github.com/rdoc/rdoc/issues/284</a></p>
<p>Can we <em>please</em> consider issuing a warning in trunk rather than raising? I think raising is fine in the future, but removing functionality without giving people notice seems bad. I understand that some people don&#39;t read warnings, but I don&#39;t think throwing our hands in the air and giving up on warnings is the right course of action.</p>
</blockquote>
<p>I will add a warning then and still rescue until next release, thanks for the feedback!</p>
</blockquote>
<p>Thank you so much! I <em>really</em> appreciate it!</p>
<p>&quot;&lt;3&lt;3&lt;3&lt;3&quot; * 5000</p>
<p>:-D</p>
<p>-- <br>
Aaron Patterson<br>
<a href="http://tenderlovemaking.com/">http://tenderlovemaking.com/</a></p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=444222014-01-18T21:48:41ZBenoit Daloze
<ul></ul><p>Changed to a warning and rescuing standard exceptions like before in <a href="https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/44646" class="changeset" title="* compar.c (cmp_equal): warn for this release and still rescue standard exceptions for a nicer ...">r44646</a>.</p>
<p>Sorry for the problems, at least the experiment showed<br>
we need a nicer transition and the error hiding does happen quite often.</p>
<p>I will be away for a week, so do not hesitate to fix if I forgot something.</p>
Ruby trunk - Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and othershttps://bugs.ruby-lang.org/issues/7688?journal_id=470762014-06-07T13:01:10ZBenoit Daloze
<ul></ul><p>I had another look at the other cases mentioned above.</p>
<ul>
<li><p>Comparable#==: A warning has been added when rescuing an exception of #&lt;=&gt;. There should be no more &quot;rescue&quot; after 2.2.0.</p></li>
<li><p>numeric.c and #coerce: The cases where an error is not raised in do_coerce() yet coercion failed are handled by their (transitive) callers which all raise other exceptions (it would be nice to make the coercion failure exception the cause of the caller exception).<br>
The possible exception thrown by #coerce was silently ignored. A warning has been added with plans in the next minor to not rescue the possible exceptions of #coerce anymore.</p></li>
<li><p>range.c and range_init(): A good solution for this would be to make the exception in #&lt;=&gt; the <em>cause</em> of the &quot;bad value for range&quot; argument error currently raised. This already works by <a href="https://bugs.ruby-lang.org/issues/8257" class="issue tracker-2 status-5 priority-4 priority-default closed" title="Exception#cause to carry originating exception along with new one (Closed)">#8257</a> but the <em>cause</em> is not shown (see <a href="https://bugs.ruby-lang.org/issues/9918" class="issue tracker-2 status-1 priority-4 priority-default" title="Exception#cause should be shown in output and #inspect (Open)">#9918</a>).</p></li>
</ul>