Bug #22007: Inconsistent type checking on rescue

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 4.0.2
• Backport: 3.3: UNKNOWN, 3.4: UNKNOWN, 4.0: UNKNOWN

[zenspider (Ryan Davis)]

this works fine (but I don't think it should):

begin
  raise "nope"
rescue RuntimeError, /why am I allowed?/, "or me?" => e
  # yay
end

whereas this version shows a type error, which seems right:

begin
  begin
    raise "nope"
  rescue /why am I NOT allowed/, "if I was allowed before?" => e
    p e
  end
rescue TypeError => te
  p te
end

I think all the args on rescue should be checked to be Module

[Eregon (Benoit Daloze)]

The reason is rescue clauses are evaluated lazily, and stop at the first matching one.
I believe this is by design for efficiency.
Evaluating every rescue clause when not necessary would be some overhead and have potentially unwanted side effects (which could even cause compatibility issues).

rescue clauses are arbitrary expressions, so they can have arbitrary side effects.
We can have e.g. rescue RuntimeError, some_method_call_returning_a_class => e and I don't think many would expect some_method_call_returning_a_class to be called needlessly.

We can also think of the case of not raising any exception:

  begin
    p :NO_EXCEPTION
  rescue /why am I NOT allowed/, "if I was allowed before?" => e
    p e
  end

Should that raise TypeError?
If yes the overhead of evaluating these rescue clauses when there is no exception would be very significant.
I would say unacceptable and unexpected.

It would be nice indeed if such invalid rescue clauses are caught early, but the performance cost seems clearly not worth it.
With appropriate test coverage this shouldn't be much of an issue in practice, it's also rather rare to have multiple rescue clauses.

[Eregon (Benoit Daloze)]

I think a better fix, related to this issue, would be to stop checking the class of rescue clauses, I think any expression should be allowed and just call === on them.

So one could do e.g.:

begin
  raise "nope"
rescue -> e { e.message.include? "o" }
  p :OK
end

[headius (Charles Nutter)]

I think the better fix would be to reject literal types that are clearly not going to match. Ideally, the only cases that should be admitted to a rescue would be constant accesses, or other expressions that could potentially resolve a type. That cuts out a huge range of literals we can prove will never match an exception.

...unless the goal is to allow rescue to match anything anywhere anytime, in which case every rescue clause must evaluate every expression and make a dynamic method call even when there's practically no chance of it matching.

Personally, I would prefer if the arguments to rescue were either a constant or an opaque expression. 99% of cases will be the constant and we can much more easily optimize for that case.

[zenspider (Ryan Davis)]

The actual place where this showed up is in assert_raises(*exp) (edited for succinctness):

    def assert_raises *exp
      msg = "#{exp.pop}.\n" if String === exp.last
      exp << StandardError if exp.empty?

      begin
        yield
      rescue *exp => e
        pass # count assertion
        return e
      # ...

where the bug report in question passed: assert_raises(StandardError, %regexp%) { ... } and the regexp went off into the aether w/o a word.

so in the case of lazy evaluation, you're already evaluating exp and then splatting the results into the rescue. At that point, does it actually cost much more to determine that one of the args is a literal (or whatever sort of checks we want to do)?

[zenspider (Ryan Davis)]

Eregon (Benoit Daloze) wrote in #note-2:

I think a better fix, related to this issue, would be to stop checking the class of rescue clauses, I think any expression should be allowed and just call === on them.

So one could do e.g.:

begin
  raise "nope"
rescue -> e { e.message.include? "o" }
  p :OK
end

so what should rescue /regexp/ do?

[Eregon (Benoit Daloze)]

zenspider (Ryan Davis) wrote in #note-5:

so what should rescue /regexp/ do?

It would do /regexp/ === exception, which is not useful but consistent.

The assert_raises example is interesting, I recall https://github.com/test-unit/test-unit/issues/347.
Maybe Minitest could have the intuitive semantics so that assert_raises when it sees a non-Module matches that against the exception message?

[kddnewton (Kevin Newton)]

We can't change this without breaking user code. Things like

irb(main):003> def resolve; SyntaxError; end
=> :resolve
irb(main):004> begin; raise SyntaxError, 'lol'; rescue resolve; end
=> nil
irb(main):005> begin; raise ArgumentError, 'lol'; rescue resolve; end
(irb):5:in '<main>': lol (ArgumentError)

exist. Most expression can potentially resolve to a value, so eliminating the ones that don't doesn't really help. You'd really only be getting rid of things like while loops and that'd be about it.

[byroot (Jean Boussier)]

Eregon (Benoit Daloze) wrote in #note-2:

I think any expression should be allowed and just call === on them.

Agreed. To me rescue is just a shorthand for rescue e; case e; when but that is strangely limited.

I suspect removing that limitation wouldn't prevent JITs from optimizing the overwhelming majority of rescue statements as they'd still only list one of a few classes.

[headius (Charles Nutter)]

byroot (Jean Boussier) wrote in #note-8:

I suspect removing that limitation wouldn't prevent JITs from optimizing the overwhelming majority of rescue statements as they'd still only list one of a few classes.

Well, it wouldn't be any worse than it is now, but optimizing execution handling is not high on my priority list.

If the rescue clause only has constants, you can guard on those constants staying the same, and whether they use the standard Class#=== implementation and maybe not intercept objects that are not is_a?.

People shouldn't be relying on exceptions to be fast in any case though, because just generating a backtrace is an enormous amount of work.

[matz (Yukihiro Matsumoto)]

• Status changed from Open to Closed

The current behavior follows from lazy evaluation of rescue clauses, which is intentional. As @kddnewton (Kevin Newton) noted in #note-7, the arguments are expressions and can't be classified statically, so an eager check means evaluating them all, with overhead and side effects.

Generalizing to === on arbitrary expressions is consistent with case/when, but it removes a check that catches real mistakes. The Minitest case in #note-4 is exactly that: a regexp silently never matching. I'd rather keep the TypeError.

For assert_raises, I agree with #note-6 that Minitest can handle non-Module arguments itself.

I will keep the current behavior as is.

Matz.