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

Add Module#ruby2_keywords for passing keyword arguments through regular splats #2449

Closed
wants to merge 4 commits into from

Conversation

jeremyevans
Copy link
Contributor

In Ruby <=2.6, delegation was often done using:

def foo(*args, &block)
  bar(*args, &block)
end

With the keyword argument separation changes recently added, this now
causes a warning if bar accepts keyword arguments, as that will break
in Ruby 3. You need to switch to delegating keyword arguments:

def foo(*args, **kw, &block)
  bar(*args, **kw, &block)
end

This works and fixes the delegation. However, it is not backwards
compatible with older versions of Ruby, as if bar does not accept
keyword arguments, it will be passed an empty positional hash if
no keywords are passed to foo. Also, there will be a hash to
keyword warning if no keywords are passed to foo, and the final
positional argument is a hash, even though behavior in terms of
arguments passed to bar will not change in Ruby 3, as the keyword
will be converted back to a hash.

For backwards compatibility with older versions of Ruby, this
allows you do to:

class Module
  private
  def pass_keywords(*) end unless respond_to?(:pass_keywords, true)
end

class Foo
  def foo(*args, &block)
    bar(*args, &block)
  end
  pass_keywords :foo
end

Then if you call foo with keyword arguments, bar will be called with
keywords arguments, but if you call foo with a positional hash
argument, bar will be called with a positional hash argument. If bar
accepts keyword arguments, the hash will be converted to keywords
in bar, and a warning will be emitted, as behavior will change in
Ruby 3 to pass the hash as a positional argument.

This implements such support using a VM frame flag. If a method has
been flagged as passing keywords (only allowed for methods that
accept an argument splat and do not accept keywords), and it is
called with keywords, it sets the flag on the next VM frame. If that
frame flag is currently set, calls with an argument splat and no
keywords where the last argument is a hash are converted into
passing the hash as keywords.

The second commit uses pass_keywords in lib/delegate to fix keyword
argument separation warnings when the target method accepts keywords
and keywords are passed to the delegate method.

@eregon
Copy link
Member

eregon commented Sep 11, 2019

Does pass_keywords then:

  • Removes the warning
  • Changes every call site in the method to try to extract keywords from the last positional argument? Or only for call sites using the rest argument?

Can there be situations where one would want to convert for one call site but not another?

@jeremyevans
Copy link
Contributor Author

Does pass_keywords then:

  • Removes the warning

Not directly. Calls to methods that do not accept keyword arguments (e.g. def foo(*args) end) never warn. If you have:

pass_keywords def foo(*args)
  bar(*args)
end
def bar(*args, **kw)
end
foo({a: 1})

You will get a warning when foo calls bar, just as you get a warning when calling bar({a: 1}).

However, this does fix the warning if you do foo(a: 1) or foo(**{a: 1}).

  • Changes every call site in the method to try to extract keywords from the last positional argument? Or only for call sites using the rest argument?

This only changes behavior to treat the final positional argument as keywords for call sites meeting all of the following:

  • In the same VM frame (so not for call sites inside a lambda/block in the method)
  • Using the rest argument (*args)
  • If the final positional argument is a hash
  • Using no literal keyword arguments or a keyword splat

Can there be situations where one would want to convert for one call site but not another?

Possibly, and this may not be able to handle every possible case. This should hopefully handle 99% of the delegating argument cases, until people no longer need to support Ruby <2.7.

Since first posted, I fixed an issue where passing empty keyword splat would not work correctly. When using pass_keywords def foo(*args) bar(*args) end, if an empty keyword splat was passed to foo, the final element of args will be an empty hash, and an empty keyword hash will be passed to bar. If bar does not accept keyword arguments, the empty splat will be eliminated (that is the default behavior in 2.7). I also fixed an issue where methods implemented in C did not handle empty splats correctly.

During testing, I found an issue where methods created with define_method were not issuing keyword argument separation warnings in all cases where they should be. I fixed that in ed96c9f and then rebased this branch after that.

…r splats

In Ruby <=2.6, delegation was often done using:

```ruby
def foo(*args, &block)
  bar(*args, &block)
end
```

With the keyword argument separation changes recently added, this now
causes a warning if bar accepts keyword arguments, as that will break
in Ruby 3.  You need to switch to delegating keyword arguments:

```ruby
def foo(*args, **kw, &block)
  bar(*args, **kw, &block)
end
```

This works and fixes the delegation. However, it is not backwards
compatible with older versions of Ruby, as if bar does not accept
keyword arguments, it will be passed an empty positional hash if
no keywords are passed to foo.  Also, there will be a hash to
keyword warning if no keywords are passed to foo, and the final
positional argument is a hash, even though behavior in terms of
arguments passed to bar will not change in Ruby 3, as the keyword
will be converted back to a hash.

For backwards compatibility with older versions of Ruby, this
allows you do to:

```ruby
class Module
  private
  def pass_keywords(*) end unless respond_to?(:pass_keywords, true)
end

class Foo
  def foo(*args, &block)
    bar(*args, &block)
  end
  pass_keywords :foo
end
```

Then if you call foo with keyword arguments, bar will be called with
keywords arguments, but if you call foo with a positional hash
argument, bar will be called with a positional hash argument. If bar
accepts keyword arguments, the hash will be converted to keywords
in bar, and a warning will be emitted, as behavior will change in
Ruby 3 to pass the hash as a positional argument.

This implements such support using a VM frame flag.  If a method has
been flagged as passing keywords (only allowed for methods that
accept an argument splat and do not accept keywords), and it is
called with keywords, it sets the flag on the next VM frame.  If that
frame flag is currently set, calls with an argument splat and no
keywords where the last argument is a hash  are converted into
passing the hash as keywords.
Empty keyword hashes are normally removed, but that breaks the
passing of empty keyword hashes when pass_keywords is used.
When pass_keywords is used, empty keyword hashes will still be
present in args.  This allows the target method to correctly
receive empty keywords passsed to the delegate method.

Add a bunch more tests for different types of methods.  These
tests found a couple issues.  The main issue is that this
code didn't work correctly with cfuncs, because of the way
cfuncs checked for empty keyword splats.  To handle that
correctly, change CALLER_SETUP_ARG so that it doesn't remove
empty keyword splats, and move the removing of empty keyword
splats to CALLER_REMOVE_EMPTY_KW_SPLAT.  Some code paths will
now call both methods, others will only call CALLER_SETUP_ARG.
The cfunc code can then check for the removal of a keyword
splat by checking calling->kw_splat after it is added by
CALLER_SETUP_ARG and before it is removed by
CALLER_REMOVE_EMPTY_KW_SPLAT.
@jeremyevans jeremyevans changed the title Add Module#pass_keywords for passing keyword arguments through regular splats Add Module#ruby2_keywords for passing keyword arguments through regular splats Sep 19, 2019
@eregon
Copy link
Member

eregon commented Sep 21, 2019

I'm confused about the proposed solution:

class Foo
  def foo(*args, &block)
    bar(*args, &block)
  end
  ruby2_keywords :foo if respond_to?(:ruby2_keywords, true)
end

Will that work in Ruby 3?

I would assume not, because you mentioned earlier that ruby2_keywords should only exist for 2.7.
But then foo would not pass kwargs to bar in 3.0, isn't it?

What is the complete code for delegation, that works on 2.6, 2.7 and 3.0, without warnings?

@jeremyevans
Copy link
Contributor Author

pass_positional_hash (the approach in #2439) was designed only for 2.7. This approach is designed for longer, at least until support for Ruby 2.6 is dropped.

I will shortly be submitting a pull request that switches the approach used from a VM frame flag to a hash object flag, which is less invasive and handles some more common cases, such as storing the args in one method (that uses ruby2_keywords) and splatting them in another method.

@jeremyevans
Copy link
Contributor Author

The hash-flag approach in #2477 was merged instead of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants