Feature #16954
closedA new mode `Warning[:deprecated] = :error` for 2.7
Description
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 (Ryuta Kamizono) 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 (Ryuta Kamizono) 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:
-
Warning[:deprecated] = :error
, to make the warning into an error which produces a full backtrace (and stops the execution). -
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
likeThread.*_on_exception
? Or other names? - Should the commandline option
-W:deprecated
support the new modes? Like-W:deprecated=error
?
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
mame (Yusuke Endoh) wrote:
There are two (orthogonal) approaches to make the first solution:
Warning[:deprecated] = :error
, to make the warning into an error which produces a full backtrace (and stops the execution).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
Updated by mame (Yusuke Endoh) over 4 years ago
@jeremyevans0 (Jeremy Evans) I have never thought of it. It may lead to a better solution:
- We publish a gem namely
warning_diagnosis
- 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 (Ryuta Kamizono) @matsuda (Akira Matsuda) What do you think?
Updated by decuplet (Nikita Shilnikov) over 4 years ago
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']
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
mame (Yusuke Endoh) wrote in #note-2:
@jeremyevans0 (Jeremy Evans) I have never thought of it. It may lead to a better solution:
- We publish a gem namely
warning_diagnosis
- 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 (Ryuta Kamizono) @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)
Updated by mame (Yusuke Endoh) over 4 years ago
- 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.
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
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.