[ruby-core:98764] [Ruby master Feature#16955] A new mode `Warning[:keyword_into_rest_arg] = true` for 2.7

Issue #16955 has been updated by 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`.

----------------------------------------
Feature #16955: A new mode `Warning[:keyword_into_rest_arg] = true` for 2.7
https://bugs.ruby-lang.org/issues/16955#change-86122

* Author: mame (Yusuke Endoh)
* Status: Open
* Priority: Normal
----------------------------------------
## 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.

```ruby
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.

```ruby
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](https://gist.githubusercontent.com/mame/d5fbddc94a4a02b708653d1948836bb9/raw/cb89be7eaa20eb57be8811d3617663d7459e80d7/keyword_into_rest_arg.patch).  (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:

```ruby
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](https://gist.github.com/mame/d5fbddc94a4a02b708653d1948836bb9).  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 said that it might be still usable with combination of a dedicated warning filter mechanism (such as [deprecation_toolkit](https://github.com/Shopify/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?




-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>