Feature #16954: A new mode `Warning[:deprecated] = :error` for 2.7

• Status: Rejected
• Assignee: -
• Target version: -

[mame (Yusuke Endoh)]

Problem

(This is what I already wrote in rubyonrails discourse but I repeat this to make this discussion easy.)

The plan of 3.0 keyword change (#14183) will prohibit the automatic conversion from the last Hash positional argument to keyword arguments, but will keep one from keywords to a positional argument. This asymmetry made it difficult to fix code of argument forwarding. @kamipo and @matsuda (Akira Matsuda), Rails developers who did much work to support the 3.0 keyword change, found the following (simplified) case where a warning is emitted within Rails code although Rails is innocent.

Rails code:

def target(**opt)       # warning: The called method `foo' is defined here
end

ruby2_keywords def lib_proxy(*args)
  target(*args)         # warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
end

User application code (or gem code other than Rails):

def app_proxy(*args) # actually, ruby2_keywords is required here
  lib_proxy(*args)
end

app_proxy(k: 42)

The user code attempts to pass keywords to Rails' target method via argument forwarding methods app_proxy and lib_proxy. Rails appropriately annotates lib_proxy with ruby2_keywords, but user code does not yet for app_proxy. Calling app_proxy converts the keywords to a normal Hash, so the final target method accepts them as a positional Hash, which leads to the warning. Unfortunately, the warning points only Rails code. So, the user sends a bug report to Rails and there is no good way for Rails developer to diagnose the issue.

@kamipo said that actual cases include rails/rails #39562 and #39227.

Proposal

We can think of two solutions for this issue:

• Make the warning print a full backtrace (including app_proxy in the above case) so that they can debug the issue.
• Warn an event of automatic conversion from keywords to a positional one, This will point the call of app_proxy, though it brings false positives.

Focus on the first solution in this ticket. (I'll create another ticket for the second one.)

There are two (orthogonal) approaches to make the first solution:

1. Warning[:deprecated] = :error, to make the warning into an error which produces a full backtrace (and stops the execution).
2. Warning[:deprecated] = :debug, to make the warning print a full backtrace (and continues the execution).

The former one is like Thread.abort_on_exception = true, and the latter is like Thread.report_on_exception = true.

A patch is not availrable yet. (I'm asking @nobu (Nobuyoshi Nakada) to create a patch.)

Discussion points

• We need to add this change into Ruby 2.7.2. Is it acceptable? (Matz has already agreed with this change, but we need to get approval from @nagachika (Tomoyuki Chikanaga), the 2.7 branch maintainer.)
• Which approach is preferrable? Both?
• Are the names :error and :debug okay? Or :abort and :report like Thread.*_on_exception? Or other names?
• Should the commandline option -W:deprecated support the new modes? Like -W:deprecated=error?

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote:

There are two (orthogonal) approaches to make the first solution:

1. Warning[:deprecated] = :error, to make the warning into an error which produces a full backtrace (and stops the execution).
2. Warning[:deprecated] = :debug, to make the warning print a full backtrace (and continues the execution).

Note that by overriding Warning.warn (e.g. by using the warning gem), both behaviors can be implemented in pure Ruby, though that approach does require parsing the warning message.

require 'warning' # warning gem
Warning.process do |message|
  if message =~ /: warning: (?:Using the last argument (?:for `.+' )?as keyword parameters is deprecated; maybe \*\* should be added to the call|Passing the keyword argument (?:for `.+' )?as the last hash parameter is deprecated|Splitting the last argument (?:for `.+' )?into positional and keyword parameters is deprecated)\n\z/
    if false # :error
      raise message
    else # :debug
      $stderr.puts message
      $stderr.puts caller
    end
  else
    $stderr.puts message
  end
end

def a(a, b: 1); end
a(b: 2) # keyword to positional

def a(a=1, b: 1); end
a({b: 1, 'a'=>1}) # split positional
a(b: 1, 'a'=>1) # split keyword

def a(b: 1); end
a({b: 1}) # positional to keyword

[mame (Yusuke Endoh)]

@jeremyevans0 (Jeremy Evans) I have never thought of it. It may lead to a better solution:

1. We publish a gem namely warning_diagnosis
2. A user installs the gem and runs their code with environment variable:

$ WARNING_DIAGNOSIS="/path/to/ruby/file.rb:100: warning: Using the last argument" ruby user_code.rb
...full backtrace whose warning matches WARNING_DIAGNOSIS...

@kamipo @matsuda (Akira Matsuda) What do you think?

[decuplet (Nikita Shilnikov)]

For the record, I used Jeremy's gem to deal with all 2.7 warnings, it saved me a lot of time. My typical setup in tests looks like

Warning.ignore(/some-gem-issuing-warnings-i-don-t-care-about/)
Warning.ignore(/another-gem/)
Warning.process { |w| raise RuntimeError, w } unless ENV['NO_RAISE_ON_WARNING']

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote in #note-2:

@jeremyevans0 (Jeremy Evans) I have never thought of it. It may lead to a better solution:

1. We publish a gem namely warning_diagnosis
2. A user installs the gem and runs their code with environment variable:

$ WARNING_DIAGNOSIS="/path/to/ruby/file.rb:100: warning: Using the last argument" ruby user_code.rb
...full backtrace whose warning matches WARNING_DIAGNOSIS...

@kamipo @matsuda (Akira Matsuda) What do you think?

I've released version 1.1.0 of the warning gem, which allows what you want:

Warning.process('/path/to/ruby/file.rb:100'){:backtrace}

It can also easily handle this more globally:

# Print backtrace for keyword separation warnings by default
Warning.process('', keyword_separation: :backtrace)

# For keyword separation warnings in files in app directory, raise them as RuntimeErrors
Warning.process('/path/to/app/', keyword_separation: :raise)

[mame (Yusuke Endoh)]

• Status changed from Open to Rejected

I explained this proposal at the developers' meeting, and we agreed that it is not necessary. It does not make sense to commit a unnecessary change to the released version 2.7, so this ticket was rejected.

@jeremyevans0 (Jeremy Evans) I'd appreciate if you could explain how to use your gem in the Rails discourse thread.

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote in #note-5:

@jeremyevans0 (Jeremy Evans) I'd appreciate if you could explain how to use your gem in the Rails discourse thread.

I attempted to post a detailed response, but apparently it wasn't accepted and my account there now states it is temporarily on hold.