Issue #16891 has been updated by jeremyevans0 (Jeremy Evans).


Dan0042 (Daniel DeLorme) wrote in #note-12:
> An idea: it may be worth sub-dividing "Positional Hash Argument to Keyword Argument Conversion" into
> 
> ### a) Overflow Positional Hash Argument to Keyword Argument Conversion
> 
> When number of positional arguments is greater than maximum arity, keeping as positional would result in ArgumentError. Converting it to Keyword Argument is relatively safe.
> ```ruby
> def foo(key: 42); end
> foo({key: 42})
> ```
> 
> ### b) Optional Positional Hash Argument to Keyword Argument Conversion
> 
> When number of positional arguments is greater than minimum arity but less or equal to maximum arity, it introduces the conditions for issues #14130 etc. Keeping those positional hash arguments as positional would prevent those issues. `*rest` delegation would convert the keywords to positional (unless you use #16463 or #16511) but then at the end of the delegation chain I think in many/most cases it would be re-converted to keywords because of a), as in this example:
> ```ruby
> def bar(*a)
>   foo(*a)
> end
> bar({key: 42})
> ```
> 
> This is hard to quantify, but my intuition is that reverting a) while keeping b) would handle a large part of that "95% of keyword argument separation issues/warnings".
> Just my intuition though.

I could be wrong, but I get the feeling the distinction you are trying to make is roughly the same as @sawa's `argument precedence` argument:

sawa (Tsuyoshi Sawada) wrote in #note-2:
> The real problem that lies there is the **argument precedence**. When there is a method call like:
> 
> ```ruby
> def foo(bar = {}, **baz)
>   #...
> end
> 
> foo(a: 1)
> ```
> 
> in previous behavior, the `**` applied before the optionality in `= {}`, and gobbled the argument `{a: 1}`, making `bar` to be always `{}`. But now, that problem has been solved by distinguishing `{a: 1}` and `a: 1`.

This argument wouldn't apply to `def bar(*a)` as that doesn't accept keywords, but would apply to `def bar(*a, **kw)` or `def bar(a={}, **kw)`.

Implementing this behavior is a simple modification to my patch, though it would make the 2.7 behavior more challenging since you would have to verbose warn in some cases and non-verbose warn in others.

This behavior can make more sense in some cases, but makes less sense in others.  Consider `def foo(a = true, **kw)`.  `foo({bar: 1})` in earlier Ruby versions would treat `bar: 1` as keywords, and arguably that makes more sense for this method definition if you are doing positional hash to keyword conversion.

Changing the argument precedence when restoring behavior makes the behavior different from 2.7. I think if we are going to restore this behavior to keep backwards compatibility, we should probably do so in a manner compatible with 2.7.  However, I don't have strong feelings about it.

----------------------------------------
Feature #16891: Restore Positional Argument to Keyword Conversion
https://bugs.ruby-lang.org/issues/16891#change-85712

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
----------------------------------------
Based on feedback from Rails-core, Matz has decided to postpone full separation of keyword and positional arguments (see https://discuss.rubyonrails.org/t/new-2-7-3-0-keyword-argument-pain-point/74980).  I think most Ruby-core developers were aware that fixing keyword argument separation issues can be fairly painful for libraries and applications that use keyword arguments extensively.

If we are going to discuss reversion or partial reversion of keyword argument separation, I think it would be best to address each aspect of keyword argument separation separately and decide how we want to handle each.  There are 4 aspects I am aware of.

## Positional Hash Argument to Keyword Argument Conversion

This is by far the most common issue when dealing with keyword argument separation.  I would estimate 95% of keyword argument separation issues/warnings are related to this change, and if we are to restore any behavior it should be this behavior.  Restoring positional hash argument to keyword argument conversion would allow the following code to continue to work:

```ruby
def foo(key: 42); end
foo({key: 42})

def bar(*a)
  foo(*a)
end
bar({key: 42})
```

Attached is a patch that restores the behavior (see below).

## Keyword Argument to Mandatory Positional Argument Conversion

This is probably the next most common issue with dealing with keyword argument separation.  I'm on the fence about whether we should try to restore this behavior. Restoring keyword argument to mandatory positional argument conversion would allow the following code to continue to work:

```ruby
def foo(h, **kw); end
foo(key: 42)
```

I don't think it would be difficult to restore this behavior, but I'm not sure it is worth doing so.

## Splitting of Keyword or Positional Hash Argument During Conversion

In Ruby 2.0-2.6, Ruby would split a hash with both symbol keys and non-symbol keys into two hashes, one symbol keyed hash used for keyword arguments, and one non-symbol keyed hash to be passed as a positional argument.

The need for this splitting appears to be rare. I have heard the splitting was not matz's intended behavior originally.  I know of only one user relying on this.  @mame's original keyword argument separation commit removed the splitting, but I added it back with deprecation warnings to ease the transition.  There's actually two types of splitting, one type when splitting keyword argument to positional argument, and another type when splitting positional argument to keyword argument.  Restoring the splitting would allow this code to continue to work:

```ruby
def foo(h={}, key: 42); [h, key] end
foo("key" => 43, key: 42)
foo({"key" => 43, key: 42})
```

Due to the very low usage of this behavior and questionable semantics, I recommend we do not restore this behavior.  Especially because the splitting no longer happens in 2.7 if arbitrary keywords are accepted by the method, since arbitrary keywords support non-symbol keys in 2.7. However, I don't think it is that difficult to restore this support if we decide to do so.

## Empty Keyword Argument Splat to Mandatory Positional Argument Conversion

In Ruby 2.7, empty keyword splats are removed and do not pass arguments.  However, if the method requires a positional argument, for compatibility Ruby 2.7 passes a new empty positional hash argument.  Restoring this behavior would allow the following code to continue to work:

```ruby
h = {}
def foo(a) a end
foo(**h)
```

I think very little code relies on this feature, and it would very painful and invasive to restore it, so I recommend we do not restore this behavior.

## Proposed behavior for Ruby 3.0

My preference would be to not change behavior at all. However, assuming we want to restore some backwards compatibility in regards to keyword argument separation, my proposal for Ruby 3.0 is to only restore positional argument to keyword argument conversion.  We convert positional hash argument to keyword arguments if:

* Method accepts keyword arguments
* Method is not passed keyword arguments
* Method is passed a hash as the final positional argument
* Method is passed more arguments than the minimum required

If the method does not accept arbitrary keywords, we only convert positional argument to keyword argument if the above are true and the final positional hash is empty or contains a symbol key.  It doesn't make sense to convert a positional hash to keywords if it has no symbol keys and the method does not accept arbitrary keywords.

When converting positional arguments to keyword arguments, we issue a warning in verbose mode in 3.0.  In 3.1 or 3.2, we can switch the warning to non-verbose mode, before removing it and enforcing full separation in the following version.

I propose to keep the current master branch behavior other than this change.

## Proposed behavior for Ruby 2.7.2

I propose we change the positional argument to keyword argument conversion warning to only issue in verbose mode and keep all other behavior the same.

## Patch

Attached is a patch that implements my proposal.  It does not include test or spec changes, as those will be extensive even for just the change I am proposing. I think we should agree on a proposal before working on changes to tests or specs.

---Files--------------------------------
keyword-hash-integration.diff (5.2 KB)


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

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