From: Benoit Daloze Date: 2020-01-20T23:07:04+01:00 Subject: [ruby-core:96954] Re: [Ruby master Bug#16154] lib/delegate.rb issues keyword argument warnings --===============1979421650== Content-Type: multipart/alternative; boundary="0000000000001caf31059c99854d" --0000000000001caf31059c99854d Content-Type: text/plain; charset="UTF-8" @fieldtensor Please file a separate bug report. On Mon, Jan 20, 2020 at 9:53 AM wrote: > Issue #16154 has been updated by fieldtensor (Patrick Rutkowski). > > > jeremyevans0 (Jeremy Evans) wrote: > > The hash-flag approach for ruby2_keywords has been merged at > 3b302ea8c95d34d5ef072d7e3b326f28a611e479. That commit uses ruby2_keywords > in delegate, fixing the keyword argument separation issues. > > I ran into a warning when calling #write_nonblock on a Tempfile instance. > Some digging led me to this issue page. When I saw this merge I assumed > that the warning would go away if I switched my rbenv from 2.7.0 to > 2.7.0-dev, but switching to 2.7.0-dev did even worse, it crashed: > > ``` > $ RBENV_VERSION="2.6.5" ruby -v -r tempfile -e > 'Tempfile.new.write_nonblock("hello\n", exception: false)'; echo; echo > ruby 2.6.5p114 (2019-10-01 revision 67812) [x86_64-darwin18] > > > $ RBENV_VERSION="2.7.0" ruby -v -r tempfile -e > 'Tempfile.new.write_nonblock("hello\n", exception: false)'; echo; echo > ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin18] > /rbenv/versions/2.7.0/lib/ruby/2.7.0/delegate.rb:336: warning: Using the > last argument as keyword parameters is deprecated; maybe ** should be added > to the call > :120: warning: The called method `write_nonblock' is defined > here > > > $ RBENV_VERSION="2.7.0-dev" ruby -v -r tempfile -e > 'Tempfile.new.write_nonblock("hello\n", exception: false)'; echo; echo > ruby 2.8.0dev (2020-01-20T00:50:03Z master f31b90f2b9) [x86_64-darwin18] > Traceback (most recent call last): > 2: from -e:1:in `
' > 1: from > /rbenv/versions/2.7.0-dev/lib/ruby/2.8.0/delegate.rb:336:in `block in > delegating_block' > :120:in `write_nonblock': wrong number of arguments (given 2, > expected 1) (ArgumentError) > ``` > > Is this crash related to the issue here? Should this issue be reopened? > Should I file a separate bug report? > > ---------------------------------------- > Bug #16154: lib/delegate.rb issues keyword argument warnings > https://bugs.ruby-lang.org/issues/16154#change-83978 > > * Author: jeremyevans0 (Jeremy Evans) > * Status: Closed > * Priority: Normal > * Assignee: > * Target version: > * ruby -v: > * Backport: 2.5: DONTNEED, 2.6: DONTNEED > ---------------------------------------- > In the current master branch, `lib/delegate.rb` (e.g. > `Delegator`/`SimpleDelegator`/`DelegateClass`) does not delegate keyword > arguments, leading to keyword argument warnings when using it. It is > simple to fix this by changing it to delegate keyword arguments, and I have > a commit that does this ( > https://github.com/ruby/ruby/pull/2439/commits/30a70491b11d5235877a13d1a1d3fe0157960a64 > ). > > However, one issue with this approach, that you can notice at the bottom > of the commit, is that it warns in a case where the behavior will not > change between 2.6 and 3.0: > > ```ruby > foo = Object.new > def foo.bar(*args) > args > end > d = SimpleDelegator.new(foo) > d.bar({a: 1}) > # warns in 2.7, even though 2.6, 2.7, and 3.0 will return [{a: 1}] > ``` > > The reason it warns even though the behavior will not change is that there > is a behavior change, but the behavior change is inside the delegator. > When you call `d.bar`, the positional hash is converted to a keyword > argument, which generates a warning. When `d.bar` calls `foo.bar`, the > keyword splat is converted back to a positional hash. Because of this > hash->keyword->hash conversion, the net result is behavior does not change > outside the delegator, and therefore it would be great if we can somehow > flag this particular case as not warning, while still warning in the case > where `foo.bar` would accept keyword arguments (as in that case, behavior > will change). > > I came up with an implementation that makes delegate not emit the warning > in the case where behavior does not change, but still emit the warning in > the case where behavior will change ( > https://github.com/ruby/ruby/pull/2439/commits/6b7bd4f89143d692932dc89fadc341980816ee14). > This works by flagging a method that accepts keyword arguments. If the > flag is set for the method, and the method is passed a positional hash that > is converted to a keyword argument, no warning is emitted, and instead a VM > frame flag is set in the next frame so that keyword splats inside that > method are converted to positional arguments. The method flag is enabled > by calling a `Module#pass_positional_hash` method, and this method would > only exist in 2.7, since it is only for skipping this 2.7-specific warning. > > I'm not sure this implementation approach is best, and would be open to > any approach that allows delegate to not emit a warning for the code above, > as long as it doesn't change the default keyword behavior in 2.7. > > Attached is a patch that implements this, and there is also a pull request > that has passed CI for it (https://github.com/ruby/ruby/pull/2439). > > ---Files-------------------------------- > delegate-keyword-argument-separation.patch (19.1 KB) > > > -- > https://bugs.ruby-lang.org/ > > Unsubscribe: > > --0000000000001caf31059c99854d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
@fieldtensor Please file a separate bug report.
On Mon, = Jan 20, 2020 at 9:53 AM <patrick.rutkowski@gmail.com> wrote:
Issue #16154 has been updated = by fieldtensor (Patrick Rutkowski).


jeremyevans0 (Jeremy Evans) wrote:
> The hash-flag approach for ruby2_keywords has been merged at 3b302ea8c= 95d34d5ef072d7e3b326f28a611e479.=C2=A0 That commit uses ruby2_keywords in d= elegate, fixing the keyword argument separation issues.

I ran into a warning when calling #write_nonblock on a Tempfile instance. S= ome digging led me to this issue page. When I saw this merge I assumed that= the warning would go away if I switched my rbenv from 2.7.0 to 2.7.0-dev, = but switching to 2.7.0-dev did even worse, it crashed:

```
$ RBENV_VERSION=3D"2.6.5" ruby -v -r tempfile -e 'Tempfile.ne= w.write_nonblock("hello\n", exception: false)'; echo; echo ruby 2.6.5p114 (2019-10-01 revision 67812) [x86_64-darwin18]


$ RBENV_VERSION=3D"2.7.0" ruby -v -r tempfile -e 'Tempfile.ne= w.write_nonblock("hello\n", exception: false)'; echo; echo ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin18]
/rbenv/versions/2.7.0/lib/ruby/2.7.0/delegate.rb:336: warning: Using the la= st argument as keyword parameters is deprecated; maybe ** should be added t= o the call
<internal:io>:120: warning: The called method `write_nonblock' is= defined here


$ RBENV_VERSION=3D"2.7.0-dev" ruby -v -r tempfile -e 'Tempfil= e.new.write_nonblock("hello\n", exception: false)'; echo; ech= o
ruby 2.8.0dev (2020-01-20T00:50:03Z master f31b90f2b9) [x86_64-darwin18] Traceback (most recent call last):
=C2=A0 =C2=A0 2: from -e:1:in `<main>'
=C2=A0 =C2=A0 =C2=A0 =C2=A0 1: from /rbenv/versions/2.7.0-dev/lib/ruby/2.8.= 0/delegate.rb:336:in `block in delegating_block'
<internal:io>:120:in `write_nonblock': wrong number of arguments = (given 2, expected 1) (ArgumentError)
```

Is this crash related to the issue here? Should this issue be reopened? Sho= uld I file a separate bug report?

----------------------------------------
Bug #16154: lib/delegate.rb issues keyword argument warnings
https://bugs.ruby-lang.org/issues/16154#change-8= 3978

* Author: jeremyevans0 (Jeremy Evans)
* Status: Closed
* Priority: Normal
* Assignee:
* Target version:
* ruby -v:
* Backport: 2.5: DONTNEED, 2.6: DONTNEED
----------------------------------------
In the current master branch, `lib/delegate.rb` (e.g. `Delegator`/`SimpleDe= legator`/`DelegateClass`) does not delegate keyword arguments, leading to k= eyword argument warnings when using it.=C2=A0 It is simple to fix this by c= hanging it to delegate keyword arguments, and I have a commit that does thi= s (https://gi= thub.com/ruby/ruby/pull/2439/commits/30a70491b11d5235877a13d1a1d3fe0157960a= 64).

However, one issue with this approach, that you can notice at the bottom of= the commit, is that it warns in a case where the behavior will not change = between 2.6 and 3.0:

```ruby
foo =3D Object.new
def foo.bar(*args)
=C2=A0 args
end
d =3D SimpleDelegator.new(foo)
d.bar({a: 1})
# warns in 2.7, even though 2.6, 2.7, and 3.0 will return [{a: 1}]
```

The reason it warns even though the behavior will not change is that there = is a behavior change, but the behavior change is inside the delegator.=C2= =A0 When you call `d.bar`, the positional hash is converted to a keyword ar= gument, which generates a warning.=C2=A0 When `d.bar` calls `foo.bar`, the = keyword splat is converted back to a positional hash.=C2=A0 Because of this= hash->keyword->hash conversion, the net result is behavior does not = change outside the delegator, and therefore it would be great if we can som= ehow flag this particular case as not warning, while still warning in the c= ase where `foo.bar` would accept keyword arguments (as in that case, behavi= or will change).

I came up with an implementation that makes delegate not emit the warning i= n the case where behavior does not change, but still emit the warning in th= e case where behavior will change (https://github.com/ruby/ruby/pull/2439/commits/6b7bd4= f89143d692932dc89fadc341980816ee14).=C2=A0 This works by flagging a met= hod that accepts keyword arguments.=C2=A0 If the flag is set for the method= , and the method is passed a positional hash that is converted to a keyword= argument, no warning is emitted, and instead a VM frame flag is set in the= next frame so that keyword splats inside that method are converted to posi= tional arguments.=C2=A0 The method flag is enabled by calling a `Module#pas= s_positional_hash` method, and this method would only exist in 2.7, since i= t is only for skipping this 2.7-specific warning.

I'm not sure this implementation approach is best, and would be open to= any approach that allows delegate to not emit a warning for the code above= , as long as it doesn't change the default keyword behavior in 2.7.

Attached is a patch that implements this, and there is also a pull request = that has passed CI for it (https://github.com/ruby/ruby/pull/2= 439).

---Files--------------------------------
delegate-keyword-argument-separation.patch (19.1 KB)


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

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=3Dunsubscribe= >
<http://lists.ruby-lang.org/cgi-bin/m= ailman/options/ruby-core>
--0000000000001caf31059c99854d-- --===============1979421650== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline Unsubscribe: --===============1979421650==--