Feature #21279: Bare "rescue" should not rescue NameError

• Status: Open
• Assignee: -
• Target version: -

[AMomchilov (Alexander Momchilov)]

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 NameErrors 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.

[Eregon (Benoit Daloze)]

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.

[matz (Yukihiro Matsumoto)]

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.

[mame (Yusuke Endoh)]

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?

[p8 (Petrik de Heus)]

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.

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.

[byroot (Jean Boussier)]

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.

[Dan0042 (Daniel DeLorme)]

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

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.

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.

Fully agree with the above.

I understand the rationale regarding typos, but 99% of the time (at least for me) NoMethodError is due not to a typo but because the receiver is not the expected type. Very often because it's nil. And that is very much what I would consider a StandardError.

Just one example of incompatibility: Rack::ShowException rescues StandardError, LoadError, SyntaxError to display a nice error page; if NoMethodError was no longer rescued that would be quite a problem.

[Earlopain (Earlopain _)]

I wondered how it looks in the real world (not really, I only ran against tests) so I wrote some code:

require "prism"
by_message = Hash.new(0)
by_location = Hash.new(0)

class Visitor < Prism::Visitor
  attr_reader :hit

  def initialize(lineno)
    super()
    @lineno = lineno
    @hit = false
  end

  def visit_rescue_node(node)
    if node.exceptions.empty? || node.exceptions.any? { it.is_a?(Prism::ConstantReadNode) && it.name == :StandardError }
      @hit = true if @lineno == node.keyword_loc.start_line
    end
  end

  def visit_rescue_modifier_node(node)
    @hit = true if @lineno == node.keyword_loc.start_line
  end
end

TracePoint.new(:rescue) do |tp|
  if tp.raised_exception.is_a?(NameError)
    visitor = Visitor.new(tp.lineno)
    Prism.parse_file(tp.path).value.accept(visitor)
    if visitor.hit
      by_message["#{tp.path}:#{tp.lineno} #{tp.raised_exception}"] += 1
      by_location["#{tp.path}:#{tp.lineno}"] += 1
    end
  end
end.enable

END {
  pp by_message
  puts "--------------------"
  pp by_location
}

You can put it into a test helper or something like that. I ran it over activerecord and unsurprisingly it found the code that @p8 linked: the rails activerecord test suite raises a NameError there 450 times with a bunch of different classes that don't respond to to_i.

Seemingly it only finds 4 places in activerecord with bare rescues, but then again like said previously you can't really say if that is representative of actual usage since I got this number with tests only (or my code above is buggy). I do also like the semantics of modifier rescue right now.