[ruby-core:122104] [Ruby Feature#21279] Bare "rescue" should not rescue NameError

Issue #21279 has been updated by Earlopain (Earlopain _).


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

```rb
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.

----------------------------------------
Feature #21279: Bare "rescue" should not rescue NameError
https://bugs.ruby-lang.org/issues/21279#change-113257

* Author: AMomchilov (Alexander Momchilov)
* Status: Open
----------------------------------------
# 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`](https://docs.ruby-lang.org/en/master/NameError.html) is a subclass of [`StandardError`](https://docs.ruby-lang.org/en/master/StandardError.html), so it's caught whenever you use a "bare" `rescue` block. 

```ruby
begin
  DoesNotExist
rescue => e
  p e # => #<NameError: uninitialized constant DoesNotExist>
end
```

Similarly, [`NoMethodError`](https://docs.ruby-lang.org/en/master/NoMethodError.html) is also rescued, because it's a subclass of `NameError`.

```ruby
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`](https://docs.ruby-lang.org/en/master/SyntaxError.html), 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`](https://docs.ruby-lang.org/en/master/ScriptError.html), 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

<details><summary>fun example</summary>
The worst case I've seen of this came from a unit tesat like so:

```ruby
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`.
</details>

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



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- [e-mail]
 To unsubscribe send an email to [e-mail]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/