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

• Status: Closed
• Assignee: Eregon (Benoit Daloze)
• Target version: -

[Eregon (Benoit Daloze)]

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 rb_rescue() or rb_rescue2(..., GenericErrorClass, ...) should be avoided if possible.
I analyzed these in the code base and it is used in a couple places:

• compar.c in cmp_equal(): this is the main offender in this regard with #coerce

• numeric.c in rb_num_coerce_{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?

[mrkn (Kenta Murata)]

• Category set to core
• Assignee set to matz (Yukihiro Matsumoto)
• Target version changed from 2.0.0 to 2.6

[mrkn (Kenta Murata)]

• Tracker changed from Bug to Feature

[Eregon (Benoit Daloze)]

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?

[matz (Yukihiro Matsumoto)]

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

Matz

[Eregon (Benoit Daloze)]

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

[matz (Yukihiro Matsumoto)]

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

[Eregon (Benoit Daloze)]

Hello,

matz (Yukihiro Matsumoto) wrote:

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

[Eregon (Benoit Daloze)]

matz, what do you think?

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

[schmurfy (Julien A)]

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

[ektoric (Patrick Tou)]

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.

[Eregon (Benoit Daloze)]

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.

[matz (Yukihiro Matsumoto)]

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.

[Eregon (Benoit Daloze)]

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

[Eregon (Benoit Daloze)]

• Assignee changed from matz (Yukihiro Matsumoto) to Eregon (Benoit Daloze)

[tenderlovemaking (Aaron Patterson)]

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.

[Eregon (Benoit Daloze)]

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

[tenderlovemaking (Aaron Patterson)]

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 respond_to? is to method_missing.
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/

[marcandre (Marc-Andre Lafortune)]

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?

[marcandre (Marc-Andre Lafortune)]

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.

[tenderlovemaking (Aaron Patterson)]

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.

[zzak (zzak _)]

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.

[Eregon (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!

[tenderlovemaking (Aaron Patterson)]

On Fri, Jan 17, 2014 at 12:09:41PM +0000, [e-mail] 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/

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

I had another look at the other cases mentioned above.

• Comparable#==: A warning has been added when rescuing an exception of #<=>. There should be no more "rescue" after 2.2.0.

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

• range.c and range_init(): A good solution for this would be to make the exception in #<=> the cause of the "bad value for range" argument error currently raised. This already works by #8257 but the cause is not shown (see #9918).

[j15e (Jean-Philippe Doyle)]

It seems this change was introduced in 2.3.0-preview2 but the documentation was not updated accordingly yet.

http://ruby-doc.org/core-2.3.0_preview2/Comparable.html#method-i-3D-3D

I think the last sentence should simply be removed ("Even if obj <=> other raised an exception, the exception is ignored and returns false.").

[marcandre (Marc-Andre Lafortune)]

Good catch!
I fixed the doc and also added that to NEWS, thanks :-)

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed

Applied in changeset trunk|r58474.

---

no longer rescue exceptions in numeric comparison operations

• numeric.c (do_coerce): no more error hiding.
• test/ruby/test_numeric.rb: follow the change.
  [Feature #7688]

[Eregon (Benoit Daloze)]

All rb_rescue() hiding user exceptions and/or re-raising a generic errors (mentioned above) have now been removed.

Please do not introduce rb_rescue() call in MRI, hiding user exceptions is harmful.

This changes make it much easier to debug as for instance one gets the raised by the user code
(such a a NoMethodError because of a typo) instead of a generic "bad value for range" exception.

I updated NEWS, tests and ruby/spec accordingly.