Feature #21279
openBare "rescue" should not rescue NameError
Description
Abstract¶
Bare rescue
keywords (either as a modifier like foo rescue bar
or as clause of a begin
block) should not rescue NameError
or NoMethodError
.
This behaviour is unexpected and hides bugs.
Background¶
Many Rubyists are surprised to learn that NameError
is a subclass of StandardError
, so it's caught whenever you use a "bare" rescue
block.
begin
DoesNotExist
rescue => e
p e # => #<NameError: uninitialized constant DoesNotExist>
end
Similarly, NoMethodError
is also rescued, because it's a subclass of NameError
.
begin
does_not_exist()
rescue => e
p e # => #<NoMethodError: undefined method `does_not_exist' for main>
end
This is almost never expected behaviour. NameError
/NoMethodError
is usually the result of a typo in the Ruby source, that cannot be reasonably recovered from at runtime. It's a programming error just like a SyntaxError
, which isn't a StandandError
.
Proposal¶
No matter the solution, solving this problem will require a breaking change. Perhaps this could be part of Ruby 4?
The most obvious solution is to change the superclass of NameError
from StandardError
to Exception
(or perhaps ScriptError
, similar to SyntaxError
).
Alternatives considered¶
If we want to avoid changing the inheritance hierarchy of standard library classes, we could instead change the semantics of bare rescue
from "rescues any subtype of StandardError
", to instead be "rescues any subtype of StandardError
except NameError
or its subtypes". This is worse in my opinion, as it complicates the semantics for no good reason.
Use cases¶
fun example
The worst case I've seen of this came from a unit tesat like so:test "aborts if create_user returns error" do
mock_user_action(data: {
user: { id: 123, ... },
errors: [{ code: "foo123" }]
})
ex = assert_raises(StandardError) do
CreateUser.perform(123)
end
assert_match(/foo123/, ex.message)
end
This test passes, but not for the expected reason. It turns out that inside of the business logic of CreateUser
, the error code data was accessed as a method call like error.code
, rather than a key like error[:code]
. This lead to:
NoMethodError (undefined method `code' for {:code=>"foo123"}:Hash)
The NoMethodError
is a StandardError
, and even more insidious, because foo123
is part of the NoMethodError's default message, the assert_match(/foo123/, ex.message)
also mathches!
The correct fix here would be to introduce a specific error like UserCreationError
that can be rescued specifically, with a field like code
that can be matched instead of the message. Regardless, this illustrates the kind of confusion that comes from NoMethodError
being a StandardError
.
Discussion¶
It might be useful to distinguish between NameError
s made in "static" code like DoesNotExist
or does_not_exist()
, versus those encountered dynamically via Object.const_get(dynamic_value)
or object.send(dynamic_value)
. In those metaprogramming cases, the error could be a consequence of bad runtime data, which is more recoverable than just some fundamental error with your Ruby code.
Updated by Eregon (Benoit Daloze) 6 days ago
I think the proposal makes sense and would be worth to try it experimentally (i.e. merge to master and see if it actually breaks things, and how badly), if matz agrees to it.
I recall being bitten by this a couple times as well, it's all too simple to accidentally catch typos in method names as StandardError with bare rescue
.
Updated by matz (Yukihiro Matsumoto) 4 days ago
During Ruby 1.6 - 1.8, NameError was a subclass of Exception, then moved under the StandardError (on 2001-07-05). Unfortunately, I don't remember the reason behind the change.
But since the change was intentional at least, we need to investigate the rationale first.
Matz.
Updated by mame (Yusuke Endoh) 3 days ago
matz (Yukihiro Matsumoto) wrote in #note-2:
During Ruby 1.6 - 1.8, NameError was a subclass of Exception, then moved under the StandardError (on 2001-07-05). Unfortunately, I don't remember the reason behind the change.
But since the change was intentional at least, we need to investigate the rationale first.
I couldn't find the rationale, but found interesting discussion:
https://blade.ruby-lang.org/ruby-list/29079 2001-04-04 wada: There is nil.to_i
. I want nil.to_f
too
https://blade.ruby-lang.org/ruby-list/29088 2001-04-04 matz: Type conversion should be explicit. I want to remove nil.to_i
if possible. Please use d.to_f rescue 0.0
https://blade.ruby-lang.org/ruby-list/29100 2001-04-04 wada: It does not work
https://blade.ruby-lang.org/ruby-list/29101 2001-04-04 matz: Sorry, NameError
is not rescue'ed by default. Please use rescue NameError
https://blade.ruby-lang.org/ruby-dev/12763 2001-04-04 matz: TypeError
is rescue'ed by default, but NameError
is not. If feel it inconsistent. I want to restore NameError < StandardError
back.
https://blade.ruby-lang.org/ruby-dev/12765 2001-04-04 kosako: A typo will be rescue'ed by default. Isn't it useful?
https://blade.ruby-lang.org/ruby-dev/12767 2001-04-05 matz: We cannot tell whether a.foobar
is a typo or a TypeError
https://blade.ruby-lang.org/ruby-dev/12774 2001-04-05 aamine: I see an advantage in reverting. NameError
occurs frequently, so we want to rescue it easily.
https://blade.ruby-lang.org/ruby-dev/12787 2001-04-06 kosako: Ok, if so it would be good to raise a TypeError
https://blade.ruby-lang.org/ruby-dev/12789 2001-04-06 matz: I am thinking to separate a NameError
to rescue-able one and non-rescue-able one
https://blade.ruby-lang.org/ruby-dev/12839 2001-04-10 matz: I would introduce NoMethodError
, which is a rescue-able variant of NameError
. "undefined local variable or method" should be kept NameError
, which is non-rescue-able by default.
...
b12904e85f5a9cc6c82f0fd06783ba219f334932 2001-06-05 matz: Change to NoMethodError < NameError < StandardError
For a long time, I didn't understand why NoMethodError
, NameError
(and TypeError
) were separated. However, this history suggests they were originally introduced to distinguish between exceptions that are rescued by default and those that are not. That distinction, though, became unclear or perhaps meaningless after only two months.
I couldn't find any discussion leading up to the 2001-06-05 commit in the ruby-dev, ruby-list, or ruby-talk mailing lists (ruby-core did not exist at the time).
My guess is that @matz (Yukihiro Matsumoto) may have forgotten the subtle distinction between NoMethodError
and NameError
within those two months and decided they should inherit from one another due to their apparent similarity.
Anyway.
I couldn't find the precise rationale, but @matz (Yukihiro Matsumoto) seemed to believe (at least at the time) that code like d.to_f rescue 0.0
should work even when d
is nil. This appears to be one of the reasons why NoMethodError
was made a subclass of StandardError
.
Personally, I feel that code like d.to_f rescue 0.0
are not very modern. However, given that such ideas were common at the time, there is a compatibility concern.
@matz (Yukihiro Matsumoto), do you think such code should work even now?
Updated by p8 (Petrik de Heus) 3 days ago
ยท Edited
I couldn't find the precise rationale, but @matz (Yukihiro Matsumoto) seemed to believe (at least at the time) that code like
d.to_f rescue 0.0
should work even whend
is nil. This appears to be one of the reasons whyNoMethodError
was made a subclass ofStandardError
.Personally, I feel that code like
d.to_f rescue 0.0
are not very modern. However, given that such ideas were common at the time, there is a compatibility concern.
Just wanted to mention that Rails uses something similar:
https://github.com/rails/rails/blob/bc7f77b32d7f3f26db654cae506c26a959852506/activemodel/lib/active_model/type/integer.rb#L109
I'm not a fan of this type of code because it might hide an underlying problem, for example an undefined method deeper down the stack.
Updated by byroot (Jean Boussier) 3 days ago
Maybe I'm simply used to how this worked until now, but I don't see a problem with NoMethodError
being handled by bare rescue
.
99% of the time, when I use a bare rescue
it's because I'll call raise
again, or calling unknown code and I need to not crash, etc.
So to me it makes sense to rescue about everything except "system" issues like NoMemoryError
etc.
merge to master and see if it actually breaks things, and how badly
I can already tell this is going to break a lot of things. Worse I'm pretty sure it's going to break things in production, but not be caught in test because these codepaths aren't necessarily tested for NoMethodError
specifically.
So I'm personally very opposed to this change.