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


Eregon (Benoit Daloze) wrote:
> jeremyevans0 (Jeremy Evans) wrote:
> > I think @matz should strongly consider this particular problem when deciding whether to accept `ruby2_keywords` by default.  In @Eregon 's dev meeting presentation, he said of this type of code: "In some rare cases, users might want to treat keyword arguments as positional".  Perhaps this type of code is not as rare as once believed.
> 
> I believe that is an incorrect conclusion.
> This bug happens because `ruby2_keywords` exists (maybe we can fix `**empty_hash` or `**empty_flagged_hash` though, see below).
> I was originally against `ruby2_keywords` because it's not purely lexical and therefore more complicated, but this is a long lost battle now.
> Having ruby2_keywords by default changes exactly __*nothing*__ about this specific bug.
> It happens just the same with or without https://github.com/ruby/ruby/pull/2853.

With only explicit ruby2_keywords, you are only affected by the issue if you misuse ruby2_keywords.  ruby2_keywords was designed only for handling delegation cases, and usage in non-delegation cases is where it runs into trouble.  With ruby2_keywords by default, it's used for all methods accepting args splat and not keywords, making this type of issue much more common.

I consider ruby2_keywords a dangerous method, that should only be used if you know the situation requires it.  Not something applied to all methods that would accept it.  By applying it to all methods that would accept it, you are introducing dangerous behavior into potentially all ruby programs, and I believe that is a bug.

Eregon (Benoit Daloze) wrote:
> > To paraphrase @Eregon , if a ruby committer heavily involved in the process cannot figure out even why a problem occurs, let alone the solution, what chance does a standard ruby programmer have?
> 
> For the record, by "I don't know why it goes wrong." I meant "This is after midnight and I have no time to investigate it further today".
> But yes, I'm surprised a ruby2_keywords flagged Hash is just removed and "lost".
> I think we might want to fix that.

I disagree.  A ruby2_keywords flagged Hash, when converted to keywords, should be removed if the hash is empty, because that is the same behavior that happens when you double splat an empty hash.

> I think the actual issue here might be that we remove empty keyword arguments, which is also inconsistent with non-empty ones when they are given to a method not accepting keyword arguments:
> ```ruby
> def target(*args)
>   args
> end
> 
> # target does not accept keyword arguments, it shouldn't matter if we pass a last positional Hash or use keyword arguments syntax.
> # But actually it does in 2.7.0 and master!
> h = {}
> p target(**{a: 1}) # => [{:a=>1}]
> p target(**h)     # => [] in 2.7.0 and master (inconsistent with above), [{}] in 2.6 and ruby2_keywords by default
> p target(h)       # => [{}]
> ```

This is by design.  Empty keyword splats are equivalent to passing no arguments, just as empty regular splats are equivalent to passing no arguments.  I consider the Ruby <2.7 behavior a bug in this regard.  When you consider that `**{}` never passed arguments (was removed by the parser), I think the intended behavior for empty keyword splats was not to pass arguments.

> We need to remove empty keyword arguments for one reason: to make `*args, **kwargs` delegation work with a target method not taking kwargs:
> ```ruby
> def target(a, b = nil)
>   [a, b]
> end
> 
> def delegate(*args, **kwargs)
>   target(*args, **kwargs)
> end
> 
> p delegate(1) # => [1, {}] in 2.6.5, [1, nil] in 2.7.0, master and ruby2_keywords by default
> ```
> 
> Note that if `target` does accept keyword arguments, it would not change anything whether we pass or not an empty keyword Hash in Ruby 3.0+.
> 
> Here are two ideas to make `*args, **kwargs` delegation work, but not remove empty keyword hashes when passed to a method not accepting keyword arguments:
> * When the keyword Hash is created out of nothing, i.e., by `def m(**hash_created_by_double_splat)` and `m` was not passed any keyword argument, we could remember that, and when calling another method with `foo(**hash_created_by_double_splat)` remove that Hash, since the user never intended it. For other cases, keep pass the keyword Hash just like for non-empty keyword hashes, since the user passed keywords explicitly.
> * Maybe ruby2_keywords-flagged Hash should never be removed. I need to think more about that case and experiment with it.
> 
> @jeremyevans0 @mame What do you think of that?

I think the current behavior of always removing empty keyword splats (including implicit splats of empty flagged hashes) makes much more sense.

----------------------------------------
Bug #16519: pp [Hash.ruby2_keywords_hash({})] shows `[nil]`
https://bugs.ruby-lang.org/issues/16519#change-83995

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Assignee: 
* Target version: 
* ruby -v: ruby 2.8.0dev (2020-01-21T13:45:10Z master 5798d35ff6) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: REQUIRED
----------------------------------------
This happens on `master`:

```
$ ruby -ve 'ruby2_keywords def flag(*a); a.last; end; pp [flag(**{})]'
ruby 2.8.0dev (2020-01-21T13:45:10Z master 5798d35ff6) [x86_64-linux]
[nil]
```

Of course it should be `[{}]`, as it is for `pp [{}]`.

On 2.7.0 it warns (should be fixed, it's valid to `pp` a flagged Hash):
```
$ ruby -ve 'ruby2_keywords def flag(*a); a.last; end; pp [flag(**{})]'
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-linux]
[/home/eregon/.rubies/ruby-2.7.0/lib/ruby/2.7.0/pp.rb:226: warning: Passing the keyword argument as the last hash parameter is deprecated
/home/eregon/.rubies/ruby-2.7.0/lib/ruby/2.7.0/pp.rb:334: warning: The called method is defined here
{}]
```

The warning being in the middle of the output is a fun fact here.
Lines it refers to (still the same on current master):
https://github.com/ruby/ruby/blob/v2_7_0/lib/pp.rb#L226
https://github.com/ruby/ruby/blob/v2_7_0/lib/pp.rb#L334

This is very confusing as it can happen during `test-all` and then show output such as:
```
<[{:a=>1}]> expected but was
<[{:a=>1}, nil]>.
```
when the reality is (can be verified with `p` before the `assert_equal`):
```
<[{:a=>1}]> expected but was
<[{:a=>1}, {}]>.
```



-- 
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>