Introduce cop to warn against usage of $! and $@.

[ioquatix]

https://bugs.ruby-lang.org/issues/18083#note-21

Introduce a cop to warn against all usage of $! and $@. There is no safe usage except for perhaps expression rescue $!.

Usage of $! can be rewritten:

begin
rescue => error
  raise
ensure
  if error ...
end

But Matz also said he disagree with such an approach. But at least it's more safe than if $!.

[eregon]

s/copy/cop/

[dmarcoux]

I would like to tackle this. Under which cop departments would you suggest having this new cop? I'm unsure between Lint and Style.

[dvandersluis]

I think Lint probably makes the most sense.

[dvandersluis]

You'd probably have to check for $ERROR_INFO and $ERROR_POSITION aliases which are added by require 'English' as well.

[ioquatix]

I did not know about those extra variations, thanks for pointing it out.

[ioquatix]

Would it make sense to include how to do it safely as an example?

[duerst]

But Matz also said he disagree with such an approach. But at least it's more safe than if $!.

Matz says it's not recommended to branch inside ensure. He disagrees with introducing new syntax for ensure.
(I know the distinction is very subtle, but I think it's important.)

[andyw8]

Care should be taken for how this interacts with Style/SpecialGlobalVars.

[ioquatix]

@duerst I agree but the reality is some systems it is unavoidable, for example committing or aborting a database transaction, we need to guarantee at least one option has happened, so:

def with_transaction
  transaction = db.begin_transaction
  yield transaction
rescue Exception
  transaction&.abort!
ensure
  transaction.commit! if transaction&.open?
end

Not sure how else you can express this, and yet this is an incredibly basic operation. Exception handling needs to support this in a straight forward and reliable way. Another example:

begin
  file = File.open(...)
  yield file
ensure
  file.close unless file.nil? || file.closed?
end

I'm almost certain this is how File.open is implemented. But please feel free to prove me wrong or provide some alternative approach.

[eregon]

FYI IO#close does not raise so you can call it whether it's already closed or not.
Caring file being nil is avoided by ensuring only what needs to be in the begin is:

file = File.open(...)
begin
  yield file
ensure
  file.close
end

Same for the transaction example, it should be an explicit begin.
In general ensure/rescue without an explicit begin is a trap and incorrect.

[ydakuka]

The same story.

I have the following code.

class Middleware
  def log_memory
    logger.info("PID: #{$$}")
  end
end

If I run rubocop with autocorrect option, I will get the code:

# frozen_string_literal: true

require 'English'
class Middleware
  def log_memory
    logger.info("PID: #{$PROCESS_ID}")
  end
end

Rubocop:

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop -V
1.56.3 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 2.7.8) [x86_64-linux]
  - rubocop-capybara 2.19.0
  - rubocop-factory_bot 2.24.0
  - rubocop-performance 1.19.1
  - rubocop-rails 2.21.1
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.24.0
  - rubocop-thread_safety 0.5.1

[ioquatix]

Just for reference, we found a good case study on this: ruby/net-ftp#24

[ioquatix]

https://github.com/rubocop/rubocop/blob/master/lib/rubocop/cop/style/global_vars.rb#L60-L62

The GlobalVars explicitly enables things like $! and $ERROR_INFO but I think we should remove these as they are unsafe.

#10211 was proposed as a specific cop for the issues discussed here but it was rejected in favour of modifying GlobalVars.

What's the best way forward?