Feature #18083
openCapture error in ensure block.
Added by ioquatix (Samuel Williams) over 3 years ago. Updated 11 months ago.
Description
As discussed in https://bugs.ruby-lang.org/issues/15567 there are some tricky edge cases.
As a general model, something like the following would be incredibly useful:
begin
...
ensure => error
pp "error occurred" if error
end
Currently you can get similar behaviour like this:
begin
...
rescue Exception => error
raise
ensure
pp "error occurred" if error
end
The limitation of this approach is it only works if you don't need any other rescue
clause. Otherwise, it may not work as expected or require extra care. Also, Rubocop will complain about it.
Using $!
can be buggy if you call some method from rescue
or ensure
clause, since it would be set already. It was discussed extensively in https://bugs.ruby-lang.org/issues/15567 if you want more details.
Updated by ioquatix (Samuel Williams) over 3 years ago
By the way, perhaps we should consider deprecating $!
since its usage can lead to incorrect behaviour.
Updated by Eregon (Benoit Daloze) over 3 years ago
The second snippet seems clear, the first one much less.
In what cases error
is set? If it's a StandardError? If it's an Exception? If it's any kind of "Ruby exception"?
IMHO this pattern is rarely enough needed that the second snippet is good enough.
If you need other rescue
, you can add them in a nested begin/end, or in a different method, no?
BTW I think the second snippet only differentiates Exception
and put together regular execution and control flow exit, is that intended?
Updated by Eregon (Benoit Daloze) over 3 years ago
If you only need to know if there is an Exception
this seems easier:
Using the example from https://bugs.ruby-lang.org/issues/15567#note-27
begin
...
rescue Exception => exception
transaction.abort
raise exception
ensure
transaction.commmit unless transaction.aborted?
end
# or
begin
...
rescue Exception => exception
aborted = true
transaction.abort
raise exception
ensure
transaction.commmit unless aborted
end
# or
begin
...
rescue Exception => exception
transaction.abort
raise exception
ensure
transaction.commmit unless exception
end
Updated by Eregon (Benoit Daloze) over 3 years ago
- Related to Feature #15567: Allow ensure to match specific situations added
Updated by ioquatix (Samuel Williams) over 3 years ago
There are lots of ways to achieve this but the reality is there is a lot of code which uses $!
in the ensure block which is incorrect for the reasons already explained.
Your proposed ideas are fine, except that RuboCop will warn you against rescue Exception
. I agree with the general sentiment around this.
That being said, there are definitely valid use cases where your ensure block wants to know whether it's exiting via a normal code flow or an exceptional one. To answer your question, ensure => error
, error would be the equivalent of a local $!
, so nothing really changes, we are just providing a mechanism to do correctly and safely what people are already doing with $!
. I don't think there is any valid use case for $!
if we introduce the proposed feature here.
Updated by duerst (Martin Dürst) over 3 years ago
Eregon (Benoit Daloze) wrote in #note-3:
If you only need to know if there is an
Exception
this seems easier:
Using the example from https://bugs.ruby-lang.org/issues/15567#note-27begin ... rescue Exception => exception transaction.abort raise exception ensure transaction.commmit unless transaction.aborted? end
What about
begin
...
rescue Exception
transaction.abort
else
transaction.commit
end
It seems shorter and easier to understand.
Having read through most of #15567, my impression is that:
- Ruby already has too many features for exceptional flow control. Adding more stuff that makes this even more complicated doesn't look like an improvement of the language. (Go To Statement Considered Harmful, anybody?)
- All the documentation/books that I can remember explain Exceptions and catch/throw completely separately (although usually in close proximity, because both features don't really fit anywhere). Some improvement seems in order, even if this is only "don't use them together".
-
ensure
contains always executed code. Adding conditions to its syntax seems strange. It would be similar to extending the syntax ofelse
(both inif
expressions as well as inbegin
blocks). Specific conditions should be handled where we already have them, onrescue
, e.g. likerescue when throw
or some such (if at all). - The argument that Rubocop complains about something isn't really an argument. If Rubocop is wrong, it should be fixed.
Updated by ioquatix (Samuel Williams) over 3 years ago
Ruby already has too many features for exceptional flow control. Adding more stuff that makes this even more complicated doesn't look like an improvement of the language. (Go To Statement Considered Harmful, anybody?)
We are not implementing any additional flow control.
ensure contains always executed code. Adding conditions to its syntax seems strange. It would be similar to extending the syntax of else (both in if expressions as well as in begin blocks). Specific conditions should be handled where we already have them, on rescue, e.g. like rescue when throw or some such (if at all).
Whether or not you agree with it, people already do it: https://bugs.ruby-lang.org/issues/15567#note-26
I want to make this use case safer, as I was bitten by this bug. The original discussion is a bit different but the surrounding context is similar.
The argument that Rubocop complains about something isn't really an argument. If Rubocop is wrong, it should be fixed.
rescue Exception
can produce unusual results that users might not expect, so I think RuboCop is correct here and we shouldn't encourage users to write rescue Exception
.
Updated by ioquatix (Samuel Williams) over 3 years ago
Your proposed "shorter and easier to understand" implementation is actually incorrect:
def transaction1
begin
yield
rescue Exception => error
ensure
if error
puts "abort"
else
puts "commit"
end
end
end
def transaction2
begin
yield
rescue Exception
puts "abort"
else
puts "commit"
end
end
catch(:foo) do
transaction1 do
throw :foo
end
end
Try for transaction1
and transaction2
. The flow control is wrong for transaction2
.
Updated by ioquatix (Samuel Williams) over 3 years ago
We want to make it easy for people to write robust Ruby programs, even if there is failure. As discussed in https://bugs.ruby-lang.org/issues/15567 there are some tricky edge cases when using $!
because it's non-local state. As an example:
def do_work
begin # $! may be set on entry to `begin`.
# ... do work ...
ensure
if $!
puts "work failed"
else
puts "work success"
end
end
end
begin
raise "Boom"
ensure
# $! is set.
do_work
end
There are real examples of this kind of code: https://bugs.ruby-lang.org/issues/15567#note-26
There is another kind of error that can occur:
begin
raise "Boom"
rescue => error
# Ignore?
ensure
pp error # <RuntimeError: Boom>
pp $! # nil
end
As a general model, something like the following would be incredibly useful, and help guide people in the right direction:
begin
...
ensure => error
pp "error occurred" if error
end
Currently you can almost achieve this:
begin
...
rescue Exception => error # RuboCop will complain.
raise
ensure
pp "error occurred" if error
end
The limitation of this approach is it only works if you don't need any other rescue clause. Otherwise, it may not work as expected or require extra care (e.g. care with rescue ... => error
naming. Also, Rubocop will complain about it, which I think is generally correct since we shouldn't encourage users to rescue Exception
.
Because $!
can be buggy and encourage incorrect code, I also believe we should consider deprecating it. But in order to do so, we need to provide users with a functional alternative, e.g. ensure => error
.
Another option is to clear $!
when entering begin
block (and maybe restore it on exit?), but this may break existing code.
More Examples¶
Gem: bundler has 1480241823 downloads.
/lib/bundler/vendor/net-http-persistent/lib/net/http/persistent.rb
ensure
return sleep_time unless $!
Gem: rubygems-update has 817517436 downloads.
/lib/rubygems/core_ext/kernel_require.rb
ensure
if RUBYGEMS_ACTIVATION_MONITOR.respond_to?(:mon_owned?)
if monitor_owned != (ow = RUBYGEMS_ACTIVATION_MONITOR.mon_owned?)
STDERR.puts [$$, Thread.current, $!, $!.backtrace].inspect if $!
Gem: launchy has 196850833 downloads.
/lib/launchy.rb
ensure
if $! and block_given? then
yield $!
Gem: spring has 177806450 downloads.
/lib/spring/application.rb
ensure
if $!
lib = File.expand_path("..", __FILE__)
$!.backtrace.reject! { |line| line.start_with?(lib) }
Gem: net-http-persistent has 108254000 downloads.
/lib/net/http/persistent.rb
ensure
return sleep_time unless $!
Gem: open4 has 86007490 downloads.
/lib/open4.rb
ensure
killall rescue nil if $!
Gem: net-ssh-gateway has 74452292 downloads.
/lib/net/ssh/gateway.rb
ensure
close(local_port) if block || $!
Gem: vault has 48165849 downloads.
/lib/vault/persistent.rb
ensure
return sleep_time unless $!
Gem: webrick has 46884726 downloads.
/lib/webrick/httpauth/htgroup.rb
ensure
tmp.close
if $!
Gem: stripe-ruby-mock has 11533268 downloads.
/lib/stripe_mock/api/server.rb
ensure
raise e if $! != e
Gem: logstash-input-udp has 8223308 downloads.
/lib/logstash/inputs/udp.rb
ensure
if @udp
@udp.close_read rescue ignore_close_and_log($!)
@udp.close_write rescue ignore_close_and_log($!)
Gem: shrine has 6332950 downloads.
/lib/shrine/uploaded_file.rb
ensure
tempfile.close! if ($! || block_given?) && tempfile
Discussion:
- nobu:
$!
is thread-local. - ko1: Your proposal is to make shorthand for the following code?
- mame: No
begin
...
ensure
error = $!
...
end
- ko1: FYI
def foo
p $! #=> #<RuntimeError: FOO>
raise SyntaxError
rescue SyntaxError
end
begin
raise 'FOO'
ensure
foo; p $! #=> #<RuntimeError: FOO>
exit
end
- mame: It is surprising to me that
$!
is not a method-local but a thread-local variable
at_exit do # |exception| ???
if $! # exception
puts "Exiting because of error"
end
end
begin
raise "Boom"
ensure
begin
ensure => error # should be nil - yes correct. This is the difference between ko1's workaround and samuel's proposal
p $! #=> #<RuntimeError: Boom>
error = $!
puts "failed" if $!
end
end
- knu:
begin
Integer("foo")
rescue
begin
Integer("1")
ensure => error
# You can't tell `$!` came from this begin block without being rescued.
p $! #=> #<ArgumentError: invalid value for Integer(): "foo">
p error #=> nil
end
end
You can almost achieve the ideal situation with this:
begin
...
rescue Exception => error # RuboCop will complain.
raise
ensure
pp "error occurred" if error
end
Updated by matz (Yukihiro Matsumoto) over 3 years ago
I hesitate to enhance ensure
. It's ugly and against the basic concept of ensure
.
The ensure
behavior was taken from unwind-protect
macro of Lisp, and it does not distinguish between error execution and normal execution. If you check the error condition in the ensure
clause, especially using $!
, I doubt you misuse the ensure
concept.
If you really wanted to check the error condition, you may want to use local variables.
begin
....
no_error = true
ensure
error_handling unless no_error
end
At the same time, I may underestimate the pain of OP. Let me think about it for a while.
Matz.
Updated by ioquatix (Samuel Williams) over 3 years ago
@matz (Yukihiro Matsumoto) I agree with your general feeling completely. But it's too late: We already have "ugly and against the basic concept of ensure" in the form of $!
. As demonstrated, people are checking $!
and unfortunately, in general code, it is not safe to check $!
in an ensure
block because it might come from the parent scope (or some other scope). This puts us in a difficult situation.
I think the decision depends on whether you want to support or deprecate the usage of $!
. Fundamentally, if you think that ensure => error
is ugly, I believe you must agree ensure ... if $!
is equally ugly.
Support $!
If we want to support $!, I would propose that we make it safe, by limiting it to the current block. That means it's effectively reset on entering a begin...ensure...end
block, so we prevent it carrying state from the parent scope (if any). There are some potential compatibility issues, and it's still fundamentally ugly... but it might be the easiest option.
Deprecate $!
If we want to deprecate $!, I would propose that we consider how we want engineers to approach the problem they are currently depending on $!
for.
The most common use case is something like this:
begin
resource = acquire # get a resource from the pool or allocate one if pool is empty.
yield resource
ensure
if $!
free(resource) # resource is broken, get rid of it.
else
release(resource) # resource is okay, return it to the pool.
end
end
The proposal to use else
is a good one, except it doesn't capture all flow controls:
begin
resource = acquire # get a resource from the pool or allocate one if pool is empty.
yield resource
rescue Exception
free(resource) # resource is broken, get rid of it.
else
release(resource) # resource is okay, return it to the pool.
end
The problem with the above code, is there are some cases where you can exit the block and neither the rescue nor the else block are executed. It will result in a resource leak. Also, some people would argue that rescue Exception
is wrong. So, can we make the above better? If we make else
the same as "ensure without exception" then it would be sufficient, but right now it's not. It's also not clear whether the following could ever work:
begin
resource = acquire # get a resource from the pool or allocate one if pool is empty.
yield resource
rescue
free(resource) # resource is broken, get rid of it.
else # Does this mean else if there was no exception, or else if the exception was not handled?
release(resource) # resource is okay, return it to the pool.
end
The next best alternative is something like this:
begin
resource = acquire # get a resource from the pool or allocate one if pool is empty.
yield resource
rescue Exception => error
free(resource) # resource is broken, get rid of it.
raise
ensure
unless error
release(resource) # resource is okay, return it to the pool.
end
end
So the fundamental problem we have is that we don't have any way to guarantee, AFAIK, on error execute this code, otherwise execute this other code, and it turns out that a lot of code needs this, essentially "ensure if no exception was raised" or "ensure if no rescue block was executed", or something similar.
By the way, your proposed solution also falls short:
catch(:finished) do
begin
throw :finished
no_error = true
ensure
p :error_handling unless no_error
end
end
Will execute error_handling
. This is why it's a non-trivial problem. Everyone comes up with the wrong answer (including me). If you look at real code, you see these incorrect patterns repeated over and over.
In summary, I think user code wants the following: Execute some code, if it fails, do this thing, if it succeeds, do this other thing. Right now, we have: Execute some code, if it fails, do this thing. Whether it failed or not, do this other thing. In this case, flow control operations like break
, next
, throw/catch
are NOT considered failures.
Updated by Eregon (Benoit Daloze) over 3 years ago
I think use-cases needing separate cleanups whether there is a Ruby exception or "normal execution or non-local control flow" will always be ugly, fundamentally because they do something hacky and approximative:
the exception that happens inside is not a proof the resource is at fault, it could be anything, including a NoMemoryError or a SystemStackError for instance (which e.g. could be due to a bug in the block, not related to the resource at all).
So in general, there should be one call to e.g. close
in ensure
and code should not attempt to differentiate (e.g., what happens for File.open {}
).
For the rare cases which do need to differentiate, I think rescue Exception => error
+ if error
is fine.
You literally want to test "any Ruby exception" or "any other exit from this block", and then rescue Exception
just makes sense.
It's true one should avoid rescue Exception
in general, or at the very least make sure it's always re-raised to not swallow important and possibly unrelated errors.
So RuboCop is right to warn for such usages as they should be avoided in general, and it's worth double-checking if one really needs to rescue Exception
.
But as every rule, there are exceptions, and this is a good exception to that rule, if you literally want to differentiate "any Ruby exception" or "any other exit from this block", then rescue Exception
is exactly what one wants, just go ahead and suppress the RuboCop warning.
Updated by ioquatix (Samuel Williams) over 3 years ago
the exception that happens inside is not a proof the resource is at fault, it could be anything, including a NoMemoryError or a SystemStackError for instance (which e.g. could be due to a bug in the block, not related to the resource at all).
Yes, it's a fair assessment. But the key point is there is a different from normal exit (which can include throw
) and exceptional exit (exception was raised during user code). The reality is, as shown by the examples, lots of code does need to make this differentiation.
You literally want to test "any Ruby exception" or "any other exit from this block", and then rescue Exception just makes sense.
Actually, I'm less concerned about the "any Ruby exception" and more concerned with splitting execution between "normal code flow" and "exceptional code flow", and even that is just as an example. Clearly, lots of people are using $!
in an unsafe way, and that's the key point I want to fix. My initial proposal is one way to fix it, by introducing explicit ensure => error
. However, I agree with Matz, that it can be considered kind of ugly. But if that's true, why don't we get rid of $!
, surely every case for $!
can be better handled by explicit rescue
? If not, why is $!
necessary? If we can answer that question, then I think we will know what path to take.
Personally, I'd vote for the path to get rid of $!
because I do agree with Matz that its fundamentally pretty ugly both sematically, as well as practically. I looked at a lot of code that uses $!
and it's often used as a poor replacement for the implicit exception cause
argument, or for implicit passing error information between code where it should really be explicit.
Updated by Eregon (Benoit Daloze) over 3 years ago
lots of code does need to make this differentiation.
A few places, but in relative usages probably 1/1000 of begin/rescue or ensure/end patterns, right?
Agreed $!
should be deprecated and then removed.
The only usage of $!
I know of which can't be trivially replaced by => exc
is p((abc rescue $!))
and that's 1) unpretty (double parens, modifier rescue) 2) only good for debugging / printing an exception in a quick script.
It's easy enough to begin; expr; rescue => exc; p exc; end
(or better, on multiple lines).
Changing $!
to be thread+frame local would likely be too incompatible so I think deprecating + removing it is better.
Updated by ioquatix (Samuel Williams) over 3 years ago
@matz (Yukihiro Matsumoto) based on the above discussion, do you agree to deprecate and remove $!
if possible?
Updated by Eregon (Benoit Daloze) over 3 years ago
BTW, deprecating and removing $!
has another advantage:
a raised Ruby exception wouldn't immediately leak to the current thread/fiber and more optimizations might be possible, including skipping to compute the exception backtrace (the most expensive part of Ruby exceptions) if e.g. a JIT can prove the exception or its backtrace is never used.
That analysis would only require tracking usages of exc
in rescue => exc
, and not any call in the rescue
that might use $!
.
Of course, the related $@
(which is $?.backtrace
) should be deprecated and removed too.
Updated by ioquatix (Samuel Williams) over 3 years ago
A few places, but in relative usages probably 1/1000 of begin/rescue or ensure/end patterns, right?
I scanned top 10,000 gems by download count:
Checking for "$!":
Found 459 gems with this pattern.
Found 2624 instances of the pattern.
Checking for "ensure":
Found 3221 gems with this pattern.
Found 28306 instances of the pattern.
Checking for /(.*ensure(.*\n){0,4}.*\$!.*)/
:
Found 35 gems with potential issues.
Found 46 instances of the pattern.
Any usage of $! is potentially suspect, but at least 35 gems out of 10,000 are definitely incorrect.
Updated by ko1 (Koichi Sasada) about 3 years ago
Another proposal is change $!
semantics to `begin/rescue/ensure' clause local.
Does it solve the issues?
Updated by Eregon (Benoit Daloze) about 3 years ago
ko1 (Koichi Sasada) wrote in #note-18:
Another proposal is change
$!
semantics to `begin/rescue/ensure' clause local.
Does it solve the issues?
That seems elegant actually, because
begin
...
rescue
foo($!)
end
would behave like:
begin
...
rescue => exc
foo(exc)
end
And it would avoid escaping the Exception instance to the thread, which mean it can be escaped analyzed.
Also, if the rescue
doesn't use =>
or $!
, there is no need to generate a backtrace which is a huge performance gain!
With current semantics that's difficult to know as any call inside the rescue clause might use $!
.
I"m not sure regarding compatibility if e.g. $!
is accessed inside a method called from the rescue clause. But that's probably rare and seems bad code, so I think fine to try/experiment.
Updated by ko1 (Koichi Sasada) about 3 years ago
Sorry, my proposal is introducing dynamic scope, not syntactical scope like:
def foo
p $! #=> A
begin
# no raise
ensure
p $! #=> nil
end
end
begin
raise A
ensure
foo
end
We can implement raise
like method:
def my_raise
# DO SOMETHING
raise $!
end
Updated by matz (Yukihiro Matsumoto) about 3 years ago
First, I am not going to change the syntax for the ensure
clause. The ensure
clause is to unwind-protect so it is not recommended to branch depending on exceptions.
I admit the current $!
behavior is error-prone. But considering compatibility, I am not sure we can fix the problem. For the workaround at the moment, avoid using $!
. If it is possible to address the issue (e.g. @ko1's proposal of dynamic scoping or giving a warning for the bad usage), I agree with experimenting with them.
Matz.
Updated by Eregon (Benoit Daloze) about 3 years ago
ko1 (Koichi Sasada) wrote in #note-20:
Sorry, my proposal is introducing dynamic scope, not syntactical scope like:
Ah, I misunderstood.
Then I think deprecating $!
is the right way, since that seems the only way to be able to analyze decently if an exception's backtrace is needed and do the important optimizations I mentioned in earlier comments.
This would help a lot for making Ruby exceptions faster.
I'm pretty sure @headius (Charles Nutter) would agree since he did a similar optimization, but currently quite limited because of $!
being accessible inside calls.
And of course it would avoid the problem of $!
having confusing semantics, by telling users to use rescue => exc
instead which is clear, simple and clean.
If needed maybe we can avoid warning for rescue $!
since that's fairly harmless, but probably that's very rarely used in library/app code (i.e., only used in REPL and for trying things).
We could also warn whenever $!
is not used syntactically in the rescue
clause, but IMHO better deprecate it entirely.
Updated by ioquatix (Samuel Williams) about 3 years ago
Proposal to RuboCop to introduce warning: https://github.com/rubocop/rubocop/issues/10204
I support deprecating $!
and $@
.
One way: for Ruby 3.x we can create deprecation warning, and remove it in Ruby 4.x.
Updated by ioquatix (Samuel Williams) about 3 years ago
Regarding
foo rescue $!
Maybe we can improve this:
foo rescue => error
I'm not sure it's worth the effort...
Surprisingly this does work:
foo rescue $! => error
Updated by nobu (Nobuyoshi Nakada) about 3 years ago
ioquatix (Samuel Williams) wrote in #note-24:
Surprisingly this does work:
foo rescue $! => error
It is a pattern-matching.
Updated by ioquatix (Samuel Williams) 11 months ago
Here is a good example of why $!
is dangerous:
https://github.com/ruby/net-ftp/pull/24
I believe the merged fix is also wrong, since flow control can exit without closing the connection.
Updated by kddnewton (Kevin Newton) 11 months ago
If you're already using global variables, we could add another global variable that gets set in ensure
blocks that determines if an error was raised. That way you could safely check $!
. That wouldn't require a syntax change.
Updated by Dan0042 (Daniel DeLorme) 11 months ago
ko1 (Koichi Sasada) wrote in #note-20:
my proposal is introducing dynamic scope, not syntactical scope like:
I think this is a good idea, definitely an improvement over the current situation. $!
is already always correct inside a rescue
clause, and that would make it always correct inside the ensure
clause. But I want to confirm if this would be the expected behavior:
#1
p $! #=> A
begin
# no raise
ensure
p $! #=> nil (unlike A currently)
end
p $! #=> A (restored after previous begin-ensure block?)
#2
p $! #=> A
begin
raise "B"
rescue
p $! #=> B
#not re-raised
ensure
p $! #=> B (unlike A currently)
end
p $! #=> A (restored after previous begin-ensure block?)
Updated by ioquatix (Samuel Williams) 11 months ago
$!
is non-trivial to implement - can have performance implications, and dynamic scope might make it more expensive. cc @Eregon (Benoit Daloze) @headius (Charles Nutter) can we get your input?
Giving $!
dynamic scope mitigates the problem but still seems to encourage an approach that @matz (Yukihiro Matsumoto) has said he was against. Specifically, he was against ensure => exception
and this is just a variant of that approach.
In the first instance, the question we need to answer is: "Is there a valid use case for $! (assuming dynamic scope as proposed) that isn't better addressed using existing techniques?".
If the answer is YES, we should implement it.
If the answer is NO, we should deprecate and remove $!
.
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
ioquatix (Samuel Williams) wrote in #note-29:
In the first instance, the question we need to answer is: "Is there a valid use case for $! (assuming dynamic scope as proposed) that isn't better addressed using existing techniques?".
If the answer is YES, we should implement it.
If the answer is NO, we should deprecate and remove
$!
.
This perspective ignores backwards compatibility. If the answer is NO, we would still need to determine if the benefit of removing $!
exceeds the cost of the loss of backwards compatibility.
Updated by ioquatix (Samuel Williams) 11 months ago
Giving $!
dynamic scope is equally likely to cause subtle bugs, so no matter what we do here, there is a chance to cause compatibility issues. Deprecation is explicit and gives people a chance to fix the code, which in most cases that I reviewed, is buggy.
Updated by Dan0042 (Daniel DeLorme) 11 months ago
Giving
$!
dynamic scope mitigates the problem but still seems to encourage an approach that @matz (Yukihiro Matsumoto) has said he was against. Specifically, he was againstensure => exception
and this is just a variant of that approach.
I believe Matz said he was against adding extra syntax, and that he was open to experimenting with ko1's proposal.
Giving $! dynamic scope is indeed likely to cause subtle bugs, just less than currently. I think that almost all code currently written with $!
in the ensure clause assume the exception is triggered within the same begin-end block. Using a dynamic scope would "magically" fix all of this currently-incorrect code. So it's a net win.
And considering backwards compatibility I think it's unrealistic to consider deprecation. $!
is used fairly often in gems. I also use it fairly often in my code, usually in some variants of the form expr rescue $!