From: Benoit Daloze Date: 2014-01-10T22:14:56+01:00 Subject: [ruby-core:59686] Re: [ruby-trunk - Feature #7688] Error hiding with rb_rescue() on Comparable#==, #coerce and others --f46d044471272783e104efa43a12 Content-Type: text/plain; charset=ISO-8859-1 On 10 January 2014 18:30, Aaron Patterson wrote: > Ya, it makes sense. It seems the <=> in Rails is just blindly calling a > method on the parameter without checking that it's possible to compare. > It does make more sense to just return nil from <=>. > > FWIW, I just had to do this: > > https://github.com/rails/rails/commit/b0acc77edced44e47c8570bf7dddd4ce19f06cb0 Great! I managed to run and make the tests pass as well and made one of the possible fixes: https://gist.github.com/eregon/969d6d7afbf069d8b4d1. For the first case I would consider the new behavior to be a strict improvement (throwing exceptions and hiding is not only slow but likely hard to track down as I think you experienced in http://tenderlovemaking.com/2013/05/21/one-danger-of-freedom-patches.html). I think comparing things that should not be compared (for instance a Time instance with nil or false) is a right opportunity to raise an exception to warn you about a possible bug, and rescuing like it was done before would just hide you the fact and most likely have as a consequence a longer than usual debugging session. For the second case there are many possible ways to avoid comparing the TimeWithZone and the String, and I think your check makes it more robust. In my patch I made the conversion String->Zone in the calling method to keep the possible optimization if the String happened to be the already assigned zone (the conversion is done in #in_time_zone just after anyway) but I am not sure at all if it is worth the added complexity. This change is not an easy change, it is annoying to have your code broken and it might impact some code (although I think there are not so many usages of including Comparable, not defining #== and calling #== with a quite different object). Yet I think it is a good change because it reveals either bugs or bad practices. Raising exceptions in #<=> is a bad practice to me for the reasons mentioned above. And finally the most compelling reason is avoiding the exception hiding consequences, like for instance a simple mistyped variable in #<=> could now make all your instances "!=" and you have about no other way than "-d" to know about it. The fix is not always trivial either, I had to learn quite a bit about the context to fix the rdoc cases. But the reasoning (thinking about why we are comparing these different types of objects) likely makes the code better as it ensures the comparisons are now intended and meaningful. --f46d044471272783e104efa43a12 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On 10 January 2014 18:30, Aaron Patterson wrote:
> Y= a, it makes sense.=A0 It seems the <=3D> in Rails is just blindly cal= ling a
> method on the parameter without checking that it's possi= ble to compare.
> It does make more sense to just return nil from <=3D>.
>> FWIW, I just had to do this:
>
>=A0=A0 h= ttps://github.com/rails/rails/commit/b0acc77edced44e47c8570bf7dddd4ce19f06c= b0

Great! I managed to run and make the tests pass as well and made one of= the possible fixes: https://gist.github.com/eregon/969d6d7afbf069d8b4d1.

For the first case I would consider the new behavior to be a strict improve= ment (throwing exceptions and hiding is not only slow but likely hard to tr= ack down as I think you experienced in http://tenderlovemaking.c= om/2013/05/21/one-danger-of-freedom-patches.html).

I think comparing things that should not be compared (for instance a Ti= me instance with nil or false) is a right opportunity to raise an exception= to warn you about a possible bug, and rescuing like it was done before wou= ld just hide you the fact and most likely have as a consequence a longer th= an usual debugging session.

For the second case there are many possible ways to avoid comparing the= TimeWithZone and the String, and I think your check makes it more robust. = In my patch I made the conversion String->Zone in the calling method to = keep the possible optimization if the String happened to be the already ass= igned zone (the conversion is done in #in_time_zone just after anyway) but = I am not sure at all if it is worth the added complexity.

This change is not an easy change, it is annoying to have your code bro= ken and it might impact some code (although I think there are not so many u= sages of including Comparable, not defining #=3D=3D and calling #=3D=3D wit= h a quite different object). Yet I think it is a good change because it rev= eals either bugs or bad practices. Raising exceptions in #<=3D> is a = bad practice to me for the reasons mentioned above. And finally the most co= mpelling reason is avoiding the exception hiding consequences, like for ins= tance a simple mistyped variable in #<=3D> could now make all your in= stances "!=3D" and you have about no other way than "-d"= ; to know about it.

The fix is not always trivial either, I had to learn quite a bit about = the context to fix the rdoc cases. But the reasoning (thinking about why we= are comparing these different types of objects) likely makes the code bett= er as it ensures the comparisons are now intended and meaningful.
--f46d044471272783e104efa43a12--