Project

General

Profile

Actions

Feature #7688

closed

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

Added by Eregon (Benoit Daloze) about 11 years ago. Updated almost 7 years ago.

Status:
Closed
Target version:
-
[ruby-core:51389]

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


Related issues 2 (0 open2 closed)

Related to Ruby master - Bug #7940: Mistaken use of inline rescues in stdlibCloseddblack (David Black)02/24/2013Actions
Related to Ruby master - Bug #7645: BigDecimal#== slow when compared to true/falseClosedmrkn (Kenta Murata)01/02/2013Actions

Updated by mrkn (Kenta Murata) about 11 years ago

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

Updated by mrkn (Kenta Murata) about 11 years ago

  • Tracker changed from Bug to Feature

Updated by Eregon (Benoit Daloze) almost 11 years 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?

Updated by matz (Yukihiro Matsumoto) almost 11 years ago

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

Matz

Updated by Eregon (Benoit Daloze) almost 11 years 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 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 .

Updated by matz (Yukihiro Matsumoto) almost 11 years ago

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.

Updated by Eregon (Benoit Daloze) almost 11 years ago

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

Updated by Eregon (Benoit Daloze) almost 11 years ago

matz, what do you think?

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

Updated by schmurfy (Julien A) over 10 years 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...

Updated by ektoric (Patrick Tou) over 10 years 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.

Updated by Eregon (Benoit Daloze) over 10 years 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.

Updated by matz (Yukihiro Matsumoto) over 10 years 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.

Updated by Eregon (Benoit Daloze) over 10 years ago

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

Updated by Eregon (Benoit Daloze) over 10 years ago

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

Updated by tenderlovemaking (Aaron Patterson) about 10 years 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.

Updated by Eregon (Benoit Daloze) about 10 years ago

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

Updated by tenderlovemaking (Aaron Patterson) about 10 years 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 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/

Updated by marcandre (Marc-Andre Lafortune) about 10 years 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?

Updated by marcandre (Marc-Andre Lafortune) about 10 years 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.

Actions #20

Updated by tenderlovemaking (Aaron Patterson) about 10 years 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.

Actions #21

Updated by zzak (zzak _) about 10 years 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.

Updated by Eregon (Benoit Daloze) about 10 years 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!

Updated by tenderlovemaking (Aaron Patterson) about 10 years ago

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

Updated by Eregon (Benoit Daloze) about 10 years 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.

Updated by Eregon (Benoit Daloze) almost 10 years ago

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

Updated by j15e (Jean-Philippe Doyle) over 8 years ago

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

Updated by marcandre (Marc-Andre Lafortune) over 8 years ago

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

Actions #28

Updated by Eregon (Benoit Daloze) almost 7 years ago

  • 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]

Updated by Eregon (Benoit Daloze) almost 7 years ago

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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0