Issue #7688 has been reported by Eregon (Benoit Daloze). ---------------------------------------- Bug #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: Category: Target version: 2.0.0 ruby -v: ruby 2.0.0dev (2012-07-15 trunk 36395) [x86_64-darwin10.8.0] 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?
on 2013-01-11 22:38
on 2013-01-12 10:35
Issue #7688 has been updated by mrkn (Kenta Murata). Category set to core Assignee set to matz (Yukihiro Matsumoto) Target version changed from 2.0.0 to next minor ---------------------------------------- Bug #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-35372 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor ruby -v: ruby 2.0.0dev (2012-07-15 trunk 36395) [x86_64-darwin10.8.0] 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?
on 2013-04-27 11:29
Issue #7688 has been updated by 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? ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-38952 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
on 2013-05-03 15:23
Issue #7688 has been updated by matz (Yukihiro Matsumoto). Show us the patch first. I am afraid I misunderstand you. Matz ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-39109 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
on 2013-05-03 23:23
Issue #7688 has been updated by 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_r... or https://github.com/eregon/ruby/compare/no_hidden_r... . ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-39111 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
on 2013-05-04 16:22
Issue #7688 has been updated by 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. ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-39123 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
on 2013-05-04 18:24
Issue #7688 has been updated by 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). ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-39125 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
on 2013-05-12 17:17
Issue #7688 has been updated by Eregon (Benoit Daloze). matz, what do you think? Are you against introducing the change for Comparable#== ? ---------------------------------------- Feature #7688: Error hiding with rb_rescue() on Comparable#==, #coerce and others https://bugs.ruby-lang.org/issues/7688#change-39276 Author: Eregon (Benoit Daloze) Status: Open Priority: Normal Assignee: matz (Yukihiro Matsumoto) Category: core Target version: next minor 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?
Enable email notification
Enable multi-page viewPlease log in before posting. Registration is free and takes only a minute.
Existing account
(Switch to SSL-encrypted connection)
NEW: Do you have a Google/GoogleMail or Yahoo account? No registration required!
Log in with Google account | Log in with Yahoo account
Log in with Google account | Log in with Yahoo account
No account? Register here.