[ruby-core:59686] Re: [ruby-trunk - Feature #7688] Error hiding with rb_rescue() on Comparable#==, #coerce and others

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.