Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate Keyword Arguments from Positional Arguments #2395

Merged
merged 21 commits into from Aug 30, 2019

Conversation

@jeremyevans
Copy link
Contributor

commented Aug 20, 2019

This implements Ruby Feature 14183, see lengthy discussion in https://bugs.ruby-lang.org/issues/14183.

mame and others added 20 commits Mar 18, 2019
Separate keyword arguments from positional arguments
And, allow non-symbol keys as a keyword arugment
Allow ** syntax to be used for calling methods that do not accept key…
…words

Treat the ** syntax as passing a copy of the hash as the last
positional argument.  If the hash being double splatted is empty, do
not add a positional argument.

Remove rb_no_keyword_hash, no longer needed.
Fix hash to keyword warning to apply in all cases
Previously, it only applied if the call had more positional
arguments than the method it was calling accepted.
Fix keyword argument separation issues in lib
Mostly requires adding ** in either calls or method definitions.
Only promote last hash to keyword if all keys are symbols
If all keys are not symbols, then the non-symbol keys would not
be treated as keywords in previous versions.  It doesn't make
sense to treat these hashes as keywords to break compatibility and
warn about behavior changes in Ruby 2.7 when the Ruby 3.0 behavior
will be the same as the Ruby 2.6 for these hashes.
Update tests to fix warning message changes
Now that keyword splats accept non-Symbols, the inspect value of
the keyword is used instead of the string value.
Fix test after keyword argument separation
Now that keyword splats accept non-Symbols, the type of exception
changes.

Previously, a TypeError (hash key "k1" is not a Symbol) was raised
for this test, but now an ArgumentError (unknown keyword: "k1") is
raised.
Restore splitting of hashes into positional and keyword arguments, ad…
…d warning

This restores compatibility with Ruby 2.6, splitting the last
positional hash into positional and keyword arguments if it
contains both symbol and non-symbol keys.  However, in this case
it will warn, as the behavior in Ruby 3 will be to not split the
hash and keep it as a positional argument.

This does not affect the handling of mixed symbol and non-symbol
keys in bare keywords.  Those are still treated as keywords now,
as they were before this patch.  This results in different
behavior than Ruby 2.6, which would split the bare keywords and
use the non-Symbol keys as a positional arguments.
Set symbol export for rb_hash_stlike_foreach
This fixes MJIT after rb_hash_stlike_foreach used vm_args.c.
Support **nil syntax for specifying a method does not accept keyword …
…arguments

This syntax means the method should be treated as a method that
uses keyword arguments, but no specific keyword arguments are
supported, and therefore calling the method with keyword arguments
will raise an ArgumentError.  It is still allowed to double splat
an empty hash when calling the method, as that does not pass
any keyword arguments.
Make ripper support **nil syntax
The on_params hook will use :nil as the keyword rest argument.
There is a new on_nokw_param hook as well.

This fixes a type issue in the previous code, where an ID was
passed where a VALUE was the declared type.  The symbol :nil is
passed instead of the id.
Make RubyVM::AbstractSyntaxTree handle **nil syntax
Use false instead of nil for the keyword and keyword rest values
in that case.
Make Method/Proc#parameters handle **nil syntax
Use a [:nokey] entry in this case.
Implement keyword argument to last positional hash emulation
For methods that accept keyword arguments but do not accept a
keyword splat, if a keyword splat is passed, or keywords are
used with a non-symbol key, check the hash.  If the hash contains
all symbols, keep the same behavior as before.  If the hash
contains all non-symbols, move the hash to the last positional
hash and warn. If the hash contains symbols and non-Symbols, split
the hash and use the symbol keys for the keyword hash and non-symbol
keys for the positional hash and warn.
Update specs to handle non-Symbols for keyword splats in 2.7
Also handle some warnings for behavior that will change in 3.0.
Add back missing warning for duplicate keys in splatted hashes
This reverts the changes to parse.y in
a5b3726 as they are not actually
needed and cause the warning for duplicate hash keys to not be
emitted.
@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

This was accepted at the developer meeting yesterday. I'm going to push a rebased version that fixes the conflict to get a final CI test before committing.

@jeremyevans jeremyevans force-pushed the jeremyevans:keyword-argument-separation branch from f27bb8a to 13b9c6d Aug 30, 2019

@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Appveyvor CI failure appears unrelated: C:/projects/build/.ext/x64-mingw32/zlib.so: [BUG] Segmentation fault.

The test-bundler failures are actually caused by this, because bundler or bundler's tests have not been modified to support keyword argument separation, and the warnings generated are breaking tests checking for no warnings. I'll see if I can fix that.

Fix a couple of bundler issues with keyword argument separation
There are more issues than this, but hopefully this is enough
to get make test-bundler passing in CI.
@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

AppVeyor is still failing, but is has been continually failing since b3c8745. Everything else looks good, I'm going to merge this.

@jeremyevans jeremyevans merged commit b5b3afa into ruby:master Aug 30, 2019

11 of 12 checks passed

latest
Details
latest (check)
Details
check_branch
Details
latest (check)
Details
latest (windows-2016, 2017, test)
Details
latest (test-bundler)
Details
latest (test-bundler)
Details
latest (windows-2019, 2019, test)
Details
latest (test-bundled-gems)
Details
latest (test-bundled-gems)
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremyevans jeremyevans deleted the jeremyevans:keyword-argument-separation branch Aug 30, 2019

@amatsuda

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

🎉

@ioquatix

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

In the release window before 2.7 can we provide real world feedback?

@mame

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

🎉 Thank you for your hard work, Jeremy!

I think 2.7-preview2 will be released before the final release.

@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Sure, you can provide real world feedback. As much as possible, other than the change for keyword splats to accept non-Symbol keys, we want the 2.7 behavior to be the same as the 2.6 behavior, except for warnings. In Ruby 3, you will have to pass keyword parameters either literally or via the double splat, and Ruby will not automatically convert keyword parameters to positional hashes or vice versa if the method accepts keyword parameters. If the method does not accept keyword parameters, any keyword parameters will be treated as a positional hash argument.

@ioquatix

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

Do you think we will also consider some kind of generic forwarding syntax, e.g.

def foo(...)
    bar(...)

... means same as *args, **opts, &block but without implicit copy/dup.

@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Unrelated to this feature. I think it has been suggested before, but I can't find the feature request right now.

@ioquatix

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

It's not completely unrelated, because this feature changes the requirements in order to do perfect forwarding. No longer *args, &block is sufficient. But it's used in many places. It became slightly more complicated. So, I wonder if we can simplify it again.

def foo(**)
    bar(**)

Could be another approach to reuse existing token.

@jeremyevans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

I believe def foo(**) end that already means accept no positional arguments, but any keyword arguments.

k0kubun added a commit to benchmark-driver/benchmark-driver that referenced this pull request Sep 1, 2019
@koic koic referenced this pull request Sep 3, 2019
kamipo added a commit to kamipo/rails that referenced this pull request Sep 4, 2019
Do not enforce no braces for hash argument
Non-kwargs parameters should be to be braced for ruby/ruby#2395.
See https://bugs.ruby-lang.org/issues/14183 for details.

`Style/BracesAroundHashParameters` cop conflicts with that.

This removes `Style/BracesAroundHashParameters` cop and auto-correct to
following changes.

rails/rails@d94263f...5665fb5
koic added a commit to koic/parser that referenced this pull request Sep 4, 2019
Follow "Separate Keyword Arguments from Positional Arguments"
Follow ruby/ruby#2395.

RuboCop's CI of Ruby 2.7 matrix is failing. This is due to
a deprecation warning in Ruby 2.7.

```console
  1) RuboCop::CLI when BlockDelimiters has braces_for_chaining style
  corrects SpaceBeforeBlockBraces, SpaceInsideBlockBraces offenses
     Failure/Error: expect($stderr.string).to eq('')

       expected: ""
            got:
            "/usr/local/bundle/gems/parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:284:
            warning: The last
            argu...parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:288:
            warning: for `trigger_policy' defined here\n"
```

https://circleci.com/gh/rubocop-hq/rubocop/67195

This PR suppress the above Ruby 2.7's warning.
koic added a commit to koic/parser that referenced this pull request Sep 4, 2019
Follow "Separate Keyword Arguments from Positional Arguments"
Follow ruby/ruby#2395.

RuboCop's CI of Ruby 2.7 matrix is failing. This is due to
a deprecation warning in Ruby 2.7.

```console
  1) RuboCop::CLI when BlockDelimiters has braces_for_chaining style
  corrects SpaceBeforeBlockBraces, SpaceInsideBlockBraces offenses
     Failure/Error: expect($stderr.string).to eq('')

       expected: ""
            got:
            "/usr/local/bundle/gems/parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:284:
            warning: The last
            argu...parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:288:
            warning: for `trigger_policy' defined here\n"
```

https://circleci.com/gh/rubocop-hq/rubocop/67195

This PR suppress the following Ruby 2.7's warning.

```console
% cd path/to/parser
% bundle exec rake

(snip)

/Users/koic/src/github.com/whitequark/parser/lib/parser/source/tree_rewriter.rb:284:
warning: The last argument for `trigger_policy' (defined at
/Users/koic/src/github.com/whitequark/parser/lib/parser/source/tree_rewriter.rb:288)
is used as the keyword parameter
```
whitequark added a commit to whitequark/parser that referenced this pull request Sep 4, 2019
Follow "Separate Keyword Arguments from Positional Arguments"
Follow ruby/ruby#2395.

RuboCop's CI of Ruby 2.7 matrix is failing. This is due to
a deprecation warning in Ruby 2.7.

```console
  1) RuboCop::CLI when BlockDelimiters has braces_for_chaining style
  corrects SpaceBeforeBlockBraces, SpaceInsideBlockBraces offenses
     Failure/Error: expect($stderr.string).to eq('')

       expected: ""
            got:
            "/usr/local/bundle/gems/parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:284:
            warning: The last
            argu...parser-2.6.4.0/lib/parser/source/tree_rewriter.rb:288:
            warning: for `trigger_policy' defined here\n"
```

https://circleci.com/gh/rubocop-hq/rubocop/67195

This PR suppress the following Ruby 2.7's warning.

```console
% cd path/to/parser
% bundle exec rake

(snip)

/Users/koic/src/github.com/whitequark/parser/lib/parser/source/tree_rewriter.rb:284:
warning: The last argument for `trigger_policy' (defined at
/Users/koic/src/github.com/whitequark/parser/lib/parser/source/tree_rewriter.rb:288)
is used as the keyword parameter
```
@koic koic referenced this pull request Sep 12, 2019
2 of 4 tasks complete
bundlerbot bot pushed a commit to rubygems/rubygems that referenced this pull request Sep 12, 2019
Suppress Ruby 2.7's real kwargs wanring
# Description:

Follow ruby/ruby#2395.

This PR suppress the following Ruby 2.7's real kwargs warning.

```console% ruby -v
% ruby -v
ruby 2.7.0dev (2019-09-11T21:58:51Z master 515b1989b1) [x86_64-darwin17]

% gem i bundler
(snip)
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/site_ruby/2.7.0/rubygems/package.rb:489:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:171:
warning: for `mkdir' defined here

% cd path/to/rubygems
% rake test
(snip)
./Users/koic/src/github.com/rubygems/rubygems/lib/rubygems.rb:472:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:197:
warning: for `mkdir_p' defined here
```
bundlerbot bot pushed a commit to rubygems/rubygems that referenced this pull request Sep 12, 2019
Merge #2912
2912: Suppress Ruby 2.7's real kwargs warning r=hsbt a=koic

# Description:

Follow ruby/ruby#2395.

This PR suppress the following Ruby 2.7's real kwargs warning.

```console% ruby -v
% ruby -v
ruby 2.7.0dev (2019-09-11T21:58:51Z master 515b1989b1) [x86_64-darwin17]

% gem i bundler
(snip)
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/site_ruby/2.7.0/rubygems/package.rb:489:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:171:
warning: for `mkdir' defined here

% cd path/to/rubygems
% rake test
(snip)
./Users/koic/src/github.com/rubygems/rubygems/lib/rubygems.rb:472:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:197:
warning: for `mkdir_p' defined here
```

______________

# Tasks:

- [x] Describe the problem / feature
- [ ] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: Koichi ITO <koic.ito@gmail.com>
bundlerbot bot pushed a commit to rubygems/rubygems that referenced this pull request Sep 12, 2019
Merge #2912
2912: Suppress Ruby 2.7's real kwargs warning r=hsbt a=koic

# Description:

Follow ruby/ruby#2395.

This PR suppress the following Ruby 2.7's real kwargs warning.

```console% ruby -v
% ruby -v
ruby 2.7.0dev (2019-09-11T21:58:51Z master 515b1989b1) [x86_64-darwin17]

% gem i bundler
(snip)
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/site_ruby/2.7.0/rubygems/package.rb:489:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:171:
warning: for `mkdir' defined here

% cd path/to/rubygems
% rake test
(snip)
./Users/koic/src/github.com/rubygems/rubygems/lib/rubygems.rb:472:
warning: The last argument is used as the keyword parameter
/Users/koic/.rbenv/versions/2.7.0-dev/lib/ruby/2.7.0/fileutils.rb:197:
warning: for `mkdir_p' defined here
```

______________

# Tasks:

- [x] Describe the problem / feature
- [ ] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: Koichi ITO <koic.ito@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.