Feature #7688

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

Added by Benoit Daloze over 2 years ago. Updated 11 months ago.

[ruby-core:51389]
Status:Open
Priority:Normal
Assignee:Benoit Daloze

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

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 over 1 year ago

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

Revision 44453
Added by Benoit Daloze over 1 year ago

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

Revision 44502
Added by Benoit Daloze over 1 year 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/test_rdoc_normal_class.rb: fix bugs in tests.

Revision 44502
Added by Benoit Daloze over 1 year 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/test_rdoc_normal_class.rb: fix bugs in tests.

Revision 44646
Added by Benoit Daloze over 1 year 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.

Revision 44646
Added by Benoit Daloze over 1 year 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.

Revision 46375
Added by Benoit Daloze 11 months ago

  • numeric.c (do_coerce): Add a warning when an exception is raised or an invalid value is returned in #coerce called by numeric comparison operators and the exception thrown by the caller has no information on the failure. In the next release such exception should not be rescued or should be the cause of the caller exception. nil is accepted as the "no possible coercion" return value. See #7688.
  • test/ruby/test_numeric.rb: Add corresponding test.

Revision 46375
Added by Benoit Daloze 11 months ago

  • numeric.c (do_coerce): Add a warning when an exception is raised or an invalid value is returned in #coerce called by numeric comparison operators and the exception thrown by the caller has no information on the failure. In the next release such exception should not be rescued or should be the cause of the caller exception. nil is accepted as the "no possible coercion" return value. See #7688.
  • test/ruby/test_numeric.rb: Add corresponding test.

Revision 49570
Added by Benoit Daloze 2 months ago

  • compar.c (cmp_equal): no more error hiding for Comparable#==. It now behaves as other Comparable methods. See #7688.
  • test/ruby/test_comparable.rb: update related test.

History

#1 Updated by Kenta Murata over 2 years 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 2 years ago

  • Tracker changed from Bug to Feature

#3 Updated by Benoit Daloze almost 2 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?

#4 Updated by Yukihiro Matsumoto almost 2 years ago

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

Matz

#5 Updated by Benoit Daloze almost 2 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 .

#6 Updated by Yukihiro Matsumoto almost 2 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.

#7 Updated by Benoit Daloze almost 2 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).

#8 Updated by Benoit Daloze almost 2 years ago

matz, what do you think?

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

#9 Updated by Julien A almost 2 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...

#10 Updated by Patrick Tou over 1 year 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 over 1 year 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 over 1 year 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 over 1 year ago

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

#14 Updated by Benoit Daloze over 1 year ago

  • Assignee changed from Yukihiro Matsumoto to Benoit Daloze

#15 Updated by Aaron Patterson over 1 year 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 over 1 year ago

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

#17 Updated by Aaron Patterson over 1 year 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/

#18 Updated by Marc-Andre Lafortune over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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.

#25 Updated by Benoit Daloze 11 months 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).

Also available in: Atom PDF