Feature #16955: A new mode `Warning[:keyword_into_rest_arg] = true` for 2.7

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

[mame (Yusuke Endoh)]

Problem

Please see #16954. This ticket is for discussing the second solution.

Proposal

The problem is caused by automatic conversion from keywords to positional arguments, especially, when the keyword is implicitly absorbed into rest arguments.

def app_proxy(*args)
  # args.last is a normal (non-ruby2_keyword-flagged) Hash, { :k => 42 }, converted from the keyword "k: 42"
  ...
end

app_proxy(k: 42)     # this call is troublesome

So I propose a new mode Warning[:keyword_into_rest_arg] = true to notice such an event.

1: Warning[:keyword_into_rest_arg] = true
2:
3: def app_proxy(*args)
4: end
5:
6: app_proxy(k: 42)

# t.rb:6: warning: The keyword argument is absorbed into the rest parameter
# t.rb:3: warning: The called method `app_proxy' is defined here

An experimental patch is here. (The new mode is enabled by default in the patch just to make the experiment easy.)

Important note: false positives

Unfortunately, this brings many false positives. Typically, the following code is completely innocent and will work great in 3.0, but is warned:

1: def foo(*args)
2:   opt = args.pop if args.last.is_a?(Hash)
3:   ...
4: end
5:
6: foo(k: 42)

# t.rb:6: warning: The keyword argument is absorbed into the rest parameter
# t.rb:1: warning: The called method `foo' is defined here

By an experiment with make test-all, tons of warnings are observed. Note that duplicated warings are already filtered out. (Some tests generates a code dynamically, which makes the duplication filter not work.)

Many warnings occur within standard libraries. I think that it is possible to fix these warnings, but we need to introduce tons of fixes in stdlib of 2.7.2. I'm afraid if it is acceptable.

However, @Eregon (Benoit Daloze) said that it might be still usable with combination of a dedicated warning filter mechanism (such as deprecation_toolkit).

Discussion points

• Is this really helpful?
• We need to add this change into Ruby 2.7.2. Is it acceptable? (Matz had already agreed with this change. But after that, it turned out that it brings so many false positives.)
• Is the name :keyword_into_rest_arg okay?
• Should the commandline option -W:keyword_into_rest_arg be added?

[jeremyevans0 (Jeremy Evans)]

I'm against adding this. This will generate way too many false positions, not just in stdlib, but in many gems. It defeats the purpose of all of the work done to ensure that code that doesn't use keyword arguments doesn't need to change. The approach in #16954 seems like a far better way to help debug keyword argument separation issues, since it will only print warning backtrace or raise an error in cases that are actually problematic.

[Eregon (Benoit Daloze)]

I think it can be useful to debug delegation issues,
and the killer feature is this will show the warning on the first conversion,
which means the warning if not a false positive will show the line to fix, and not having to guess which of the many callers forgot to use ruby2_keywords.

IMHO, opt = args.pop if args.last.is_a?(Hash) is very old style and old Ruby code, which would be much nicer as def foo(*args, **kwargs).
Of course such code doesn't need to change, but I think it's OK to warn it in this "debug delegation mode".

I took a look at the first few warnings, and they all seem like they could all make better use of keyword arguments and are very old code.

Regarding duplicated warnings from generated code, how does it look like if we override Warning.warn to remove all duplicates?

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-2:

IMHO, opt = args.pop if args.last.is_a?(Hash) is very old style and old Ruby code, which would be much nicer as def foo(*args, **kwargs).
Of course such code doesn't need to change, but I think it's OK to warn it in this "debug delegation mode".

I took a look at the first few warnings, and they all seem like they could all make better use of keyword arguments and are very old code.

Care must be taken when doing so, especially in master when doing so is an API break. For backwards compatibility, if no keyword arguments are present, you need to keep accepting a positional hash, issue a warning, then use the hash in place of keywords. It's a significant amount of work per method, for little benefit.

Personally, I see no need to change the old code if it works. It does make sense to use keywords for new code written in the master branch.

Regarding duplicated warnings from generated code, how does it look like if we override Warning.warn to remove all duplicates?

The warning gem supports that using Warning.dedup.

[Eregon (Benoit Daloze)]

jeremyevans0 (Jeremy Evans) wrote in #note-3:

Care must be taken when doing so, especially in master when doing so is an API break.

Could you clarify that?
Do you mean calling foo(1, 2, {option: true}) (seems very unlikely) / foo(1, 2, h) (more realistic) would no longer work?

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-4:

jeremyevans0 (Jeremy Evans) wrote in #note-3:

Care must be taken when doing so, especially in master when doing so is an API break.

Could you clarify that?
Do you mean calling foo(1, 2, {option: true}) (seems very unlikely) / foo(1, 2, h) (more realistic) would no longer work?

Correct, foo(1, 2, h) would no longer work. Or foo(1, 2, CONSTANT) (often done to reduce allocations). Or when delegating, such as foo(*args) where last element of args is a hash.

[mame (Yusuke Endoh)]

• Status changed from Open to Rejected

I explained this at the developers' meeting, and matz rejected. This feature might be useful if it is used appropriately, but it is somewhat difficult to use it appropriately. And, according to recent observation, some people seem to care warnings too much, so this feature may worry such people rather than help them (even if it is opt-in). It is difficult to add such a controversial new feature to the already-released version.