Feature #7688

Error hiding with rb_rescue() on Comparable#==, #coerce and others

Added by Benoit Daloze over 1 year ago. Updated 3 months ago.

[ruby-core:51389]
Status:Open
Priority:Normal
Assignee:Benoit Daloze
Category:core
Target version:next minor

Description

Hello,

I believe error hiding is harmful because very dangerous
(it forgets errors which is likely unexpected) and hard to debug.

But I guess the compatibility is the main reason to keep these cases.

In the cases of Comparable#== and #coerce, I believe it is not worth to be compatible with this dangerous behavior
as it will at worse help to discover bugs in #<=> and #coerce methods which would raise an exception.

I doubt anyone rely on this and the #coerce spec (see #7645) itself makes me think this is unexpected behavior.
It would also simplify the spec, and no specific #coerce behavior would be needed to be defined as it would behave as a simple method call without edge cases.

So I think rbrescue() or rbrescue2(..., GenericErrorClass, ...) should be avoided if possible.
I analyzed these in the code base and it is used in a couple places:
* compar.c in cmpequal(): this is the main offender in this regard with #coerce
* numeric.c in rb
numcoerce{cmp,relop}() which call do_coerce(,,FALSE): This is the current subject of #7645.

  • io.c in io_close(): to avoid raising if #close fails, which is likely less problematic, although it would be nicer to rescue only IO-related errors and warn when an exception is raised.
  • range.c in range_init(): this is to provide a clearer error. I think it would be nice to show the original error as well.

Removing the general rescue in cmp_equal() revealed a couple bugs in RDoc due to this problem. I guess there are many others in the wild.

Can we please remove this anti-pattern?
I believe impact is only positive and that it should be done as soon as possible.

What do you think?


Related issues

Related to ruby-trunk - Bug #7940: Mistaken use of inline rescues in stdlib Closed 02/24/2013
Related to ruby-trunk - Bug #7645: BigDecimal#== slow when compared to true/false Closed 01/02/2013

Associated revisions

Revision 44453
Added by Benoit Daloze 4 months ago

  • compar.c (cmpeqrecursive): Fix the return value, the value for failed #<=> should be nil. It was raising a NoMethodError for the test case TestComparable#testnocmp (undefined method `>' for false:FalseClass). Yet one more reason for #7688.

Revision 44502
Added by Benoit Daloze 3 months ago

  • compar.c (cmp_equal): remove error hiding in Comparable#==. Comparable#== no longer rescues exceptions silently. This was the cause of quite a couple bugs. See #7688. [EXPERIMENTAL]
  • test/ruby/test_comparable.rb: adapt assertion to match new behavior.
  • lib/rdoc/method_attr.rb: fix bugs discovered by this change.
  • test/rdoc/testrdocnormal_class.rb: fix bugs in tests.

Revision 44646
Added by Benoit Daloze 3 months ago

  • compar.c (cmp_equal): warn for this release and still rescue standard exceptions for a nicer transition. See #7688. Partly reverts r44502.
  • test/ruby/test_comparable.rb: adapt assertion to match new behavior.

History

#1 Updated by Kenta Murata over 1 year ago

  • Category set to core
  • Assignee set to Yukihiro Matsumoto
  • Target version changed from 2.0.0 to next minor

#2 Updated by Kenta Murata over 1 year ago

  • Tracker changed from Bug to Feature

#3 Updated by Benoit Daloze 12 months ago

Hello,

I think this is really a bug: error hiding is harmful.
Anyway, is it OK to commit this to trunk now that 2.0 is released and in a separate branch?

#4 Updated by Yukihiro Matsumoto 12 months ago

Show us the patch first. I am afraid I misunderstand you.

Matz

#5 Updated by Benoit Daloze 12 months ago

matz (Yukihiro Matsumoto) wrote:

Show us the patch first. I am afraid I misunderstand you.

Sorry, I was not clear.

My intent is to remove error hiding, that is not reporting in any way exceptions.

In the case of Range checking and coerce, it simply calls rbcheckfuncall() instead of rbfuncall() + rbrescue() 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).
For #<=>, errors are no more caught silently by rb_rescue(), so bugs lurking in #<=> methods are shown.
Only some RDoc tests do not pass (because of bugs of #<=>). And one test for Comparable is logically changed.

Patches can be seen at https://github.com/eregon/ruby/compare/no_hidden_rescue_essential
or https://github.com/eregon/ruby/compare/no_hidden_rescue_essential.patch .

#6 Updated by Yukihiro Matsumoto 12 months ago

I agree with most of your changes in the patch, especially using rbcheckfuncall instead of rb_rescue.
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.

Matz.

#7 Updated by Benoit Daloze 12 months ago

Hello,

matz (Yukihiro Matsumoto) wrote:

I agree with most of your changes in the patch, especially using rbcheckfuncall instead of rb_rescue.
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.

Matz.

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).

On the other hand, if #<=> 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 #<=> (and other definitions of #==).

Finding why my objects are never #== because I made a typo or some sensible error in #<=> might require a lot more time to debug than just seeing the exception reported in the #<=> of #==, which is straightforward to fix.

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).

#8 Updated by Benoit Daloze 11 months ago

matz, what do you think?

Are you against introducing the change for Comparable#== ?

#9 Updated by Julien A 9 months ago

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's an horrible idea....
As pointed you don't usually except an exception to be raised anywhere in the code that's why they are called exceptions after all, I don't see why == should be any different since it's just a method like any other.

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.

PS: If I sound a little aggressive that'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...

#10 Updated by Patrick Tou 6 months ago

http://rubyforge.org/tracker/index.php?func=detail&aid=17368&group_id=426&atid=1698

Things have "improved" since 2008. If the <=> 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.

Note that other Comparable methods (Comparable#>=, #>, #<, etc.), all those others do raise the error. Only #== hides this and silently returns false.

#11 Updated by Benoit Daloze 6 months ago

ektoric (Patrick Tou) wrote:

http://rubyforge.org/tracker/index.php?func=detail&aid=17368&group_id=426&atid=1698

Thanks for the link.

Things have "improved" since 2008. If the <=> 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.

Seems to me <=> including a "raise" still hides it:

class C; include Comparable; def <=>(o); raise 'stop!'; end; end; C.new == C.new # => false

Note that other Comparable methods (Comparable#>=, #>, #<, etc.), all those others do raise the error. Only #== hides this and silently returns false.

Yes, indeed, it makes no sense to me.

#12 Updated by Yukihiro Matsumoto 4 months ago

It's quite difficult to predict what would happen if we remove error hiding.
So I agree with starting experiment to see if we will have any problem.

But I think it'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.

Matz.

#13 Updated by Benoit Daloze 4 months ago

OK, I will commit this as an experiment in early 2014.

#14 Updated by Benoit Daloze 4 months ago

  • Assignee changed from Yukihiro Matsumoto to Benoit Daloze

#15 Updated by Aaron Patterson 3 months ago

r44502 makes the Rails tests fail spectacularly. We have <=> implementations that raise exceptions and expect == to swallow them. We probably shouldn't be raising exceptions in these methods, but this change definitely breaks our tests.

#16 Updated by Benoit Daloze 3 months ago

@tenderlove These are probably bugs then, is it not? I will try to have a look.

#17 Updated by Aaron Patterson 3 months ago

On Fri, Jan 10, 2014 at 06:03:03AM +0900, Eregon (Benoit Daloze) wrote:

Issue #7688 has been updated by Eregon (Benoit Daloze).

@tenderlove These are probably bugs then, is it not? I will try to have a look.

I can't say for sure whether or not it's bugs, but I can say I don't
really like this change.

Say you write a class like this:

class MyObject
include Comparable
def <=> other
raise ArgumentError unless other.is_a?(MyObject)
# Do some comparisons
end
end

I raise an argument error because they are not comparable. To me it
implies that self and other are also not equal, but not that ==
should raise an exception.

I'd never expect this to raise an exception regardless of the
implementation of <=>:

MyObject.new != 10

IOW it seems like <=> is to ==, what respondto? is to methodmissing.
Anyway, I'm not a huge fan, but this does break our tests (though I
can fix them). It seems like I would have to implement == with
exactly the same logic as <=>, except return nil (to indicate it isn't
comparable) instead of raise an exception (which is exactly what == does
before this change).

Could we find a middle ground and just rescue ArgumentError? Or some
sort of NonComparable error?

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

#18 Updated by Marc-Andre Lafortune 3 months ago

The method <=> should return nil for objects that are not comparable, not raise errors.

So this seems to be a misunderstanding/bug in Rails.

It might be best to add a warning to Ruby 2.2 if an exception is caught by == and we can not intercept it in 2.3?

#19 Updated by Marc-Andre Lafortune 3 months ago

tenderlovemaking (Aaron Patterson) wrote:

It seems like I would have to implement == with
exactly the same logic as <=>, except return nil (to indicate it isn't
comparable) instead of raise an exception (which is exactly what == does
before this change).

Not sure I follow... You don't have to implement == at all in your example.
Moreover == should not return nil, it is <=> that should return nil.
Neither ==, != nor <=> should ever raise exceptions.

#20 Updated by Aaron Patterson 3 months ago

This also broke the RDoc tests. Since RDoc was broken, I couldn't install gems:

https://github.com/rdoc/rdoc/issues/284

Can we please 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't read warnings, but I don't think throwing our hands in the air and giving up on warnings is the right course of action.

#21 Updated by Zachary Scott 3 months ago

Can we please consider issuing a warning in trunk rather than raising?

+1, I'm glad this was only added to trunk and not released by 2.1, we should be warning before completely changing the feature.

#22 Updated by Benoit Daloze 3 months ago

Aaron Patterson wrote:

This also broke the RDoc tests. Since RDoc was broken, I couldn't install gems:

https://github.com/rdoc/rdoc/issues/284

Can we please 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't read warnings, but I don't think throwing our hands in the air and giving up on warnings is the right course of action.

I will add a warning then and still rescue until next release, thanks for the feedback!

#23 Updated by Aaron Patterson 3 months ago

On Fri, Jan 17, 2014 at 12:09:41PM +0000, eregontp@gmail.com wrote:

Issue #7688 has been updated by Benoit Daloze.

Aaron Patterson wrote:

This also broke the RDoc tests. Since RDoc was broken, I couldn't install gems:

https://github.com/rdoc/rdoc/issues/284

Can we please 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't read warnings, but I don't think throwing our hands in the air and giving up on warnings is the right course of action.

I will add a warning then and still rescue until next release, thanks for the feedback!

Thank you so much! I really appreciate it!

"<3<3<3<3" * 5000

:-D

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

#24 Updated by Benoit Daloze 3 months ago

Changed to a warning and rescuing standard exceptions like before in r44646.

Sorry for the problems, at least the experiment showed
we need a nicer transition and the error hiding does happen quite often.

I will be away for a week, so do not hesitate to fix if I forgot something.

Also available in: Atom PDF