Feature #20484: A new pragma for eager resolution of classes referenced in rescue clauses.

• Status: Feedback
• Assignee: -
• Target version: -

[jfrisby (Jon Frisby)]

I've been using Ruby for 20 years, and just today learned (the hard way) that the class name(s) referenced in a rescue clause are not resolved until an exception occurs.

Upon reflection, this behavior probably makes sense in a lot of situations. Late resolution may simplify code loading for the developer.

I would, however, love to see an opt-in feature (a la frozen-string-literal) to force resolution when the code is loaded/parsed.

[duerst (Martin Dürst)]

• Status changed from Open to Feedback

Can you give an example with actual code to show where you get problems?

[jfrisby (Jon Frisby)]

duerst (Martin Dürst) wrote in #note-1:

Can you give an example with actual code to show where you get problems?

The problem I ran into is that the exception class was properly named SomeModule::SomeClass, but my rescue block had rescue SomeClass => e. Of course, this didn't turn up until an actual exception occurred so it slipped by into production.

Ultimately, I could of course have prevented it if I had a test that forced an exception but that's often quite a lot of work. Having a pragma that resolves class names earlier would have surfaced the error much more quickly with much less effort.

[zverok (Victor Shepelev)]

Honestly, it is no different than any name used in a branch that wasn’t tested. E.g. if you have this:

if foo
  OneClass.new
else
  OtherClass.new
end

...and the else branch wasn’t covered by tests, and the OtherClass isn’t defined, then it will struck in production. The rescue clauses can be seen as kind of “syntax sugar” upon this:

# ...
rescue => e
  case e
  when SomeClass
    # ...
  when OtherClass
    # ...
  else
    # ...
  end
end

Which makes it more obvious that unless you ran into the rescue branch, any name inside it wouldn’t be evaluated, and no name errors would be caught.

So the common answer is “it should be covered by testing and/or static code analysis.”

Though theoretically, “opt-in forced resolution of everything that looks like a module name” might be an interesting idea to play with (but it probably shouldn’t be limited to rescue blocks).

[byroot (Jean Boussier)]

Eager resolution of referenced constants at parse time would be very painful in many case as it would add some extra constraints on load order, likely causing circular dependency issues, even before considering things such as autoload.

Ultimately I think this sort of errors is better handled by static analysis. For instance this type of error is caught by sorbet by default without needing any type annotations. I don't know if steep does too, but I suspect it can.

I'm also not fond of the idea of adding new pragmas, if anything I'd prefer if we eliminated some.

[jfrisby (Jon Frisby)]

zverok (Victor Shepelev) wrote in #note-3:

Honestly, it is no different than any name used in a branch that wasn’t tested. E.g. if you have this:

if foo
  OneClass.new
else
  OtherClass.new
end

...and the else branch wasn’t covered by tests, and the OtherClass isn’t defined, then it will struck in production. The rescue clauses can be seen as kind of “syntax sugar” upon this:

# ...
rescue => e
  case e
  when SomeClass
    # ...
  when OtherClass
    # ...
  else
    # ...
  end
end

Which makes it more obvious that unless you ran into the rescue branch, any name inside it wouldn’t be evaluated, and no name errors would be caught.

So the common answer is “it should be covered by testing and/or static code analysis.”

Though theoretically, “opt-in forced resolution of everything that looks like a module name” might be an interesting idea to play with (but it probably shouldn’t be limited to rescue blocks).

I would agree that any such opt-in feature should probably catch other cases. I'd say that there's a bit of nuance here about the testing situation: I feel like, in practice, rescue blocks are much more likely to be neglected in test coverage than else blocks. That is mostly an ergonomic issue around testing, of course. Of course, bigger picture, anything that can be detected at parse-time is one less than for which a test case needs to be written. That constitutes a win for programmer productivity.

To address byroot's comments: I don't follow how it could lead to circular dependency issues, but I confess I have probably not thought it through as deeply so I defer to your thinking on that. I do see how it could make load ordering concerns more in absence of autoload, which is why I suggested it be a purely opt-in feature. Certainly if autoload adds another dimension of complexity, that could make even an opt-in feature useful in significantly fewer cases -- to the point that it might not be worth implementing. That said, I was unaware that sorbet (and potentially steep) addressed this. Thank you! I will check those out.