Project

General

Profile

Feature #14183

"Real" keyword argument

Added by mame (Yusuke Endoh) over 1 year ago. Updated 8 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
[ruby-core:84255]

Description

In RubyWorld Conference 2017 and RubyConf 2017, Matz officially said that Ruby 3.0 will have "real" keyword arguments. AFAIK there is no ticket about it, so I'm creating this (based on my understanding).

In Ruby 2, the keyword argument is a normal argument that is a Hash object (whose keys are all symbols) and is passed as the last argument. This design is chosen because of compatibility, but it is fairly complex, and has been a source of many corner cases where the behavior is not intuitive. (Some related tickets: #8040, #8316, #9898, #10856, #11236, #11967, #12104, #12717, #12821, #13336, #13647, #14130)

In Ruby 3, a keyword argument will be completely separated from normal arguments. (Like a block parameter that is also completely separated from normal arguments.)
This change will break compatibility; if you want to pass or accept keyword argument, you always need to use bare sym: val or double-splat ** syntax:

# The following calls pass keyword arguments
foo(..., key: val)
foo(..., **hsh)
foo(..., key: val, **hsh)

# The following calls pass **normal** arguments
foo(..., {key: val})
foo(..., hsh)
foo(..., {key: val, **hsh})

# The following method definitions accept keyword argument
def foo(..., key: val)
end
def foo(..., **hsh)
end

# The following method definitions accept **normal** argument
def foo(..., hsh)
end

In other words, the following programs WILL NOT work:

# This will cause an ArgumentError because the method foo does not accept keyword argument
def foo(a, b, c, hsh)
  p hsh[:key]
end
foo(1, 2, 3, key: 42)

# The following will work; you need to use keyword rest operator explicitly
def foo(a, b, c, **hsh)
  p hsh[:key]
end
foo(1, 2, 3, key: 42)

# This will cause an ArgumentError because the method call does not pass keyword argument
def foo(a, b, c, key: 1)
end
h = {key: 42}
foo(1, 2, 3, h)

# The following will work; you need to use keyword rest operator explicitly
def foo(a, b, c, key: 1)
end
h = {key: 42}
foo(1, 2, 3, **h)

I think here is a transition path:

  • Ruby 2.6 (or 2.7?) will output a warning when a normal argument is interpreted as keyword argument, or vice versa.
  • Ruby 3.0 will use the new semantics.

Files

vm_args.diff (4.19 KB) vm_args.diff jeremyevans0 (Jeremy Evans), 03/25/2019 10:48 PM
vm_args_v2.diff (4.18 KB) vm_args_v2.diff mame (Yusuke Endoh), 03/29/2019 10:29 AM

Related issues

Related to Backport200 - Backport #8040: Unexpect behavior when using keyword argumentsClosed03/08/2013Actions
Related to Ruby master - Bug #8316: Can't pass hash to first positional argument; hash interpreted as keyword argumentsClosedActions
Related to Ruby master - Bug #9898: Keyword argument odditiesClosed06/03/2014Actions
Related to Ruby master - Bug #10856: Splat with empty keyword args gives unexpected resultsClosedActions
Related to Ruby master - Bug #11236: inconsistent behavior using ** vs hash as method parameterOpenActions
Related to Ruby master - Bug #11967: Mixing kwargs with optional parameters changes way method parameters are parsedRejectedActions
Related to Ruby master - Bug #12717: Optional argument treated as kwargOpenActions
Related to Ruby master - Bug #12821: Object converted to Hash unexpectedly under certain method callClosedActions
Related to Ruby master - Bug #13336: Default Parameters don't workClosedActions
Related to Ruby master - Bug #13647: Some weird behaviour with keyword argumentsOpenActions
Related to Ruby master - Bug #14130: Keyword arguments are ripped from the middle of hash if argument have default valueOpenActions
Related to Ruby master - Bug #15078: Hash splat of empty hash should not create a positional argument.OpenActions
Related to Ruby master - Bug #14415: Empty keyword hashes get assigned to ordinal args.OpenActions
Related to Ruby master - Bug #12022: Inconsistent behavior with splatted named argumentsOpenActions
Related to Ruby master - Bug #11860: Double splat does not work on empty hash assigned via variableOpenActions
Related to Ruby master - Bug #10708: In a function call, double splat of an empty hash still calls the function with an argumentAssignedActions
Related to Ruby master - Bug #11068: unable to ommit an optional keyarg if the previous arg is an optional hashOpen04/14/2015Actions
Related to Ruby master - Bug #11039: method_missing の *args 引数に symbol をキーにした hash だけを渡すと エラーとなるOpen04/07/2015Actions
Related to Ruby master - Bug #10994: Inconsistent behavior when mixing optional argument and keyword splatOpen03/21/2015Actions

History

#1

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Backport #8040: Unexpect behavior when using keyword arguments added
#2

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #8316: Can't pass hash to first positional argument; hash interpreted as keyword arguments added
#3

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #9898: Keyword argument oddities added
#4

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #10856: Splat with empty keyword args gives unexpected results added
#5

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #11236: inconsistent behavior using ** vs hash as method parameter added
#6

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #11967: Mixing kwargs with optional parameters changes way method parameters are parsed added
#7

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #12104: Procs keyword arguments affect value of previous argument added
#8

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #12717: Optional argument treated as kwarg added
#9

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #12821: Object converted to Hash unexpectedly under certain method call added
#10

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #13336: Default Parameters don't work added
#11

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #13647: Some weird behaviour with keyword arguments added
#12

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Related to Bug #14130: Keyword arguments are ripped from the middle of hash if argument have default value added

Updated by jeremyevans0 (Jeremy Evans) over 1 year ago

For a method definition like:

def foo(hsh={})
end

Will either of the following continue to work?:

foo(key: val)
foo(:key => val)

One performance issue with keyword arguments is that keyword splats allocate a hash per splat, even if no keywords are used.

In performance sensitive code, allocations can be avoided using a shared frozen hash as the default argument:

OPTS = {}.freeze
def foo(hsh=OPTS)
  bar(1, hsh)
end
def bar(val, hsh=OPTS)
end

By doing this, calling foo without keyword arguments does not allocate any hashes even if the hash is passed to other methods. If you use keyword arguments, you have to do:

def foo(**hsh)
  bar(1, **hsh)
end
def bar(val, **hsh)
end

Which I believe allocates a multiple new hashes per method call, one in the caller and one in the callee. Example:

require 'objspace'
GC.start
GC.disable
OPTS = {}

def hashes
  start = ObjectSpace.count_objects[:T_HASH]
  yield
  ObjectSpace.count_objects[:T_HASH] - start - 1
end

def foo(opts=OPTS)
  bar(opts)
end
def bar(opts=OPTS)
  baz(opts)
end
def baz(opts=OPTS)
end

def koo(**opts)
  kar(**opts)
end
def kar(**opts)
  kaz(**opts)
end
def kaz(**opts)
end

p hashes{foo}
p hashes{foo(OPTS)}
p hashes{koo}
p hashes{koo(**OPTS)}

# Output
0
0
5
6

I humbly request that unless keyword splats can be made to avoid allocation, then at least make:

def foo(hsh)
end
foo(:key => val)

still function as it has since ruby 1.8, since that can be considered a hash and not a keyword argument.

#14

Updated by mame (Yusuke Endoh) over 1 year ago

  • Backport deleted (2.3: UNKNOWN, 2.4: UNKNOWN)
  • Tracker changed from Bug to Feature

Updated by sos4nt (Stefan Schüßler) over 1 year ago

I've filed a bug report some time ago, maybe you could add it as a related issue: https://bugs.ruby-lang.org/issues/11993

Updated by dsferreira (Daniel Ferreira) over 1 year ago

It’s not clear for me all the implications of this change.
Would it be possible to exemplify the before and after behaviours in the description?

It feels to me that with this implementation it would be possible to consider both symbols and strings as keys for the keywords hash.

Would it be a possibility?

The dynamic generation of keywords hashes would be positively impacted with that move.

#17

Updated by mame (Yusuke Endoh) 12 months ago

  • Description updated (diff)

Updated by mame (Yusuke Endoh) 12 months ago

jeremyevans0 (Jeremy Evans) wrote:

For a method definition like:

def foo(hsh={})
end

Will either of the following continue to work?:

foo(key: val)
foo(:key => val)

No, it will not work. You need to rewrite the definition to def foo(**hsh).

One performance issue with keyword arguments is that keyword splats allocate a hash per splat, even if no keywords are used.

If the issue really matters, it can be fixed by lazy Hash allocation, like block parameters (#14045).

dsferreira (Daniel Ferreira) wrote:

It’s not clear for me all the implications of this change.
Would it be possible to exemplify the before and after behaviours in the description?

Added.

It feels to me that with this implementation it would be possible to consider both symbols and strings as keys for the keywords hash.

It is a completely different topic, and I'm strongly negative against allowing strings as a key.

Updated by mame (Yusuke Endoh) 12 months ago

Sorry, it seems my original description was unclear. I think it can be rephased very simply:

  • keyword argument MUST be always received as a keyword parameter
  • non-keyword argument MUST be always received as a non-keyword parameter

The following behavior will be abandoned:

  • keyword argument is passed to a last normal parameter
  • last normal hash argument is passed to keyword parameters

Here is an experimental patch to warn a deprecated behavior of keyword arguments, and it shows some OK/NG samples.

NG: a keyword argument is passed to a normal parameter

$ ./miniruby -w -e '
def foo(h)
end
foo(k: 1)
'
-e:4: warning: The keyword argument for `foo' is used as the last parameter

OK: receving it as a keyword rest argument

$ ./miniruby -w -e '
def foo(**h)
end
foo(k: 1)
'

NG: a normal hash argument is passed to a keyword argument

$ ./miniruby -w -e '
def foo(k: 1)
end
h = {k: 42}
foo(h)
'
-e:5: warning: The last argument for `foo' is used as the keyword parameter

OK: the hash as keyword argument by using **

$ ./miniruby -w -e '
def foo(k: 1)
end
h = {k: 42}
foo(**h)
'

It still needs more work. It does not support yet methods written in C because C methods always handles keyword arguments as normal arguments.

Updated by jeremyevans0 (Jeremy Evans) 12 months ago

mame (Yusuke Endoh) wrote:

jeremyevans0 (Jeremy Evans) wrote:

For a method definition like:

def foo(hsh={})
end

Will either of the following continue to work?:

foo(key: val)
foo(:key => val)

No, it will not work. You need to rewrite the definition to def foo(**hsh).

If that was the only change, it wouldn't be a big deal. However, in addition to foo(:key => val) calls, there are also foo(hsh) calls. So all callers that pass hashes would need to change from foo(hsh) to foo(**hsh). And that also breaks if there are any non-symbol keys in the hash.

In the libraries I maintain, this will be a bigger breaking change than 1.8 -> 1.9. If the decision has already been made and there is no turning back, there should probably be deprecation warnings added for it in 2.6, anytime keywords are passed to a method that accepts a default argument, or anytime a hash is passed when keyword arguments should be used.

One performance issue with keyword arguments is that keyword splats allocate a hash per splat, even if no keywords are used.

If the issue really matters, it can be fixed by lazy Hash allocation, like block parameters (#14045).

This does really matter, excessive hash allocation has a significant negative effect on performance. In addition to all of the code churn in libraries required to support this change, users of the libraries will also have to accept a significant performance hit until there is an allocation-less way to pass keyword arguments from one methods to another.

The following behavior will be abandoned:

  • keyword argument is passed to a last normal parameter
  • last normal hash argument is passed to keyword parameters

Is it possible to abandon one of these without the other? Abandoning "last normal hash argument is passed to keyword parameters" only breaks code that uses keyword arguments. Abandoning "keyword argument is passed to a last normal parameter" (supported at least back to Ruby 1.8) breaks tons of ruby code that never used keyword arguments, just to supposedly fix problems that were caused by keyword arguments.

If keyword arguments are not part of the method definition, then what is the issue with converting keyword arguments to a hash argument?

It still needs more work. It does not support yet methods written in C because C methods always handles keyword arguments as normal arguments.

What will happen to external C extension gems that use rb_get_kwargs and rb_extract_keywords, both of which accept a hash?

Updated by mame (Yusuke Endoh) 12 months ago

Jeremy, thank you for discussing this issue seriously.

jeremyevans0 (Jeremy Evans) wrote:

If that was the only change, it wouldn't be a big deal. However, in addition to foo(:key => val) calls, there are also foo(hsh) calls. So all callers that pass hashes would need to change from foo(hsh) to foo(**hsh). And that also breaks if there are any non-symbol keys in the hash.

Yes, in the current proposal, you need to rewrite all callers that passes a hash object. Note that you can already write foo(**hsh) in caller side since 2.0 (when callee-side keyword argument was introduced). Also, I believe it is a good style because the explicit operator clarifies the intent.

I have no strong opinion whether foo(:kw => 1) should pass a normal hash argument or be interpreted as keyword argument. I think the latter is better in terms of compatibility, but I'm not sure.

If keyword arguments are not part of the method definition, then what is the issue with converting keyword arguments to a hash argument?

I have never thought of this. I want to reject the following program,

def foo(*ary)
end
foo(kw: 1)

but it might be a good idea as a measure for compatibility.

It still needs more work. It does not support yet methods written in C because C methods always handles keyword arguments as normal arguments.

What will happen to external C extension gems that use rb_get_kwargs and rb_extract_keywords, both of which accept a hash?

Yes, we need to prepare C API. Ko1 has had a big plan about this since last year (or older).

Updated by jeremyevans0 (Jeremy Evans) 12 months ago

Here's an alternative proposal, with the basic idea that behavior for historical ruby 1.6+ code that doesn't use keyword arguments remains the same.

OK: Historical ruby 1.6+ (maybe before) usage (hash argument with omitted braces)

def foo(h)
  # h # => {:k => 1}
end
foo(:k => 1)
foo(k: 1) # ruby 1.9+ syntax

OK: Ruby 2.0 keyword usage that will keep working

def foo(k: 1) # or foo(**h)
end
foo(:k => 1)
foo(k: 1)
foo(**{k: 1})

NG: Using ** splat as hash argument

def foo(h)
end
foo(**{k: 1})

NG: Using hash argument instead of keyword arguments

def foo(k: 1) # or foo(**h)
end
foo({k: 1})

My reasoning for this is that historical behavior for methods that do not use keyword arguments should not be broken to fix problems caused by keyword arguments. I reviewed all issues mentioned in this ticket:

#8040: method keyword arguments
#8316: method keyword arguments
#9898: method regular argument, caller uses **
#10856: method regular argument, caller uses ** on empty array
#11236: method keyword arguments
#11967: method keyword arguments
#12104: proc usage, unrelated to keyword argument vs regular argument
#12717: method keyword arguments
#12821: method keyword arguments
#13336: method keyword arguments
#13467: method keyword arguments
#14130: method keyword arguments

As you can see, all of the problems are with using keyword arguments in the method definition or with ** at the call site when a method regular argument is used. There are no issues when the method takes a regular argument and ** is not used at the call site, with the historical behavior and syntax of specifying a hash argument with omitted braces. I see no reason to break the ruby 1.6+ historical behavior when keyword arguments are not involved.

Regarding the following program mentioned by mame:

def foo(*ary)
end
foo(kw: 1)

there is a lot of historical ruby code that does:

def foo(*ary)
  options = ary.pop if ary.last.is_a?(Hash)
  # ...
end

For that reason I think it would be best if foo(kw: 1) continued to work in such cases, since there are no problems in terms of the keyword arguments being used (no keyword arguments in method definition implies argument syntax is a hash with omitted braces).

Updated by shevegen (Robert A. Heiler) 12 months ago

I don't want to write too much, so just one comment - I would also prefer foo(kw: 1)
to retain being a Hash rather than to be assumed to be a keyword argument. I think
that it may surprise people when it would become a keyword suddenly.

Updated by matz (Yukihiro Matsumoto) 12 months ago

shevegen (Robert A. Heiler) Of course, we will take plenty of time to migrate before making it a keyword.
If we made the decision, we will make it warn you first for a year or two before the actual change.

Matz.

Updated by jeremyevans0 (Jeremy Evans) 11 months ago

To give an example of how much code this would break, let's use Redmine as an example, since it runs this bug tracker. For simplicity, let's limit our analysis to the use of a single method, ActiveRecord's where method. ActiveRecord's where method uses the following API (note, no keyword arguments):

def where(opts = :chain, *rest)
  # ...
end

where is used at least 597 times in 180 files in the application, and most of these cases appear to be calls to the ActiveRecord where method. In at least 399 cases, it appears to use an inline hash argument without braces (there are additional cases where ruby 1.9 hash syntax is used), and in 11 cases it uses an inline hash argument with braces:

$ fgrep -r .where\( !(public|doc|extra) |wc -l
     597
$ fgrep -lr .where\( !(public|doc|extra) |wc -l
     180
$ fgrep -r .where\( !(public|doc|extra) | fgrep '=>' | fgrep 'where(:' |wc -l
     399
$ fgrep -r .where\( !(public|doc|extra) | fgrep 'where({' |wc -l
      11

Examples of where usage:

# Inline hash without braces
@time_entries = TimeEntry.where(:id => params[:ids]).

# Inline hash with braces
Enumeration.where({:type => type}).update_all({:is_default => false})

# Noninline hash
condition_hash = self.class.positioned_options[:scope].inject({}) do |h, column|
  h[column] = yield(column)
  h
end
self.class.where(condition_hash)

Hopefully this serves an example of how much code this would break. Remember, this is only looking at a single method. Note that omitting the braces for hashes is almost 40x more common than including the braces.

Dropping support for braceless hashes would probably break the majority of ruby applications and libraries. Consider this another plea to limit behavior changes to methods that accept keyword arguments.

Updated by mame (Yusuke Endoh) 11 months ago

Jeremy, thank you for investigating the examples. I'd like to discuss this issue at the next developers' meeting.

This is my personal current opinion: this change indeed requires users' action, however, I believe that the problem is not so significant, and that its advantage is significant.

This change seems to remind you the breaking change of character encoding in 1.9/2.0, but it was much worse than this change because the previous one was not trivial "where to fix". The site where an error occurred was often different to the site where a wrong encoding string was created.
On the other hand, this change requires very trivial fixes. By running a test suite on Ruby 2.6 or 2.7, the interpreter will "pinpoint" all usages like you showed, and warn "this method call in line XX will not work in Ruby 3.x!". Users can easily fix the issue by checking the warnings and changing either the method calls or method definition.

I agree that compatibility is important, but the current wrong design has continuously caused troubles. This fact also looks important to me. This change will fix the issue, will make the language simpler, and will make users' code more explicit and less error-prone, which will pay users' action.

Updated by jeremyevans0 (Jeremy Evans) 11 months ago

mame (Yusuke Endoh) wrote:

Jeremy, thank you for investigating the examples. I'd like to discuss this issue at the next developers' meeting.

This is my personal current opinion: this change indeed requires users' action, however, I believe that the problem is not so significant, and that its advantage is significant.

This change seems to remind you the breaking change of character encoding in 1.9/2.0, but it was much worse than this change because the previous one was not trivial "where to fix". The site where an error occurred was often different to the site where a wrong encoding string was created.

I disagree. I migrated many applications and libraries from Ruby 1.8 to Ruby 1.9 (and later to Ruby 2.6). The changes for Ruby 1.8 -> 1.9 were minimal in comparison with the impact of this change, in terms of the amount of code that needed to be modified.

On the other hand, this change requires very trivial fixes. By running a test suite on Ruby 2.6 or 2.7, the interpreter will "pinpoint" all usages like you showed, and warn "this method call in line XX will not work in Ruby 3.x!". Users can easily fix the issue by checking the warnings and changing either the method calls or method definition.

I agree that compatibility is important, but the current wrong design has continuously caused troubles. This fact also looks important to me. This change will fix the issue, will make the language simpler, and will make users' code more explicit and less error-prone, which will pay users' action.

As I've already shown earlier in this issue, all problems in issues referenced in your initial post boil down to two basic cases:

1) Where the method being called accepts keyword arguments
2) Where double splat (**) is used by the caller and the method does not accept keyword arguments

No problems have been posted where the method does not accept keyword arguments and braces are just omitted when calling the method with an inline hash. That code has not continuously caused problems, it has worked fine since at least Ruby 1.6 with basically no changes.

The keyword argument problems started occurring in Ruby 2.0 when keyword arguments were introduced, and only affected people who chose to use define methods that accepted keyword arguments or use the double splat. If you never used double splats and never defined methods that accepted keyword arguments, either to avoid the usability and performance problems with keyword arguments or to retain compatibility with ruby <2.0, then you never ran into any of these problems.

This change makes sense for methods that accept keyword arguments, and for double splat usage on hashes when the method does not accept keyword arguments. I agree that those cases are problematic and we should fix those cases in Ruby 3. I'm just requesting that the changes be limited to those cases, and not break cases where keyword arguments and double splats were never used, since those cases have never been problematic.

Ruby is a beautiful language designed for programmer happiness. Having to change all calls from where(:id=>1) to where({:id=>1}) makes the code uglier and is going to make most Ruby programmers less happy. Does this argument for explicitness lead to requiring parentheses for all method calls?

Updated by mame (Yusuke Endoh) 11 months ago

jeremyevans0 (Jeremy Evans) wrote:

Having to change all calls from where(:id=>1) to where({:id=>1}) makes the code uglier and is going to make most Ruby programmers less happy. Does this argument for explicitness lead to requiring parentheses for all method calls?

In this specific case, it looks better to change the callee side instead of the caller side: the method definition of where should receive a keyword rest argument. Of course, it still requires us change some calls of where(opt_hash) to where(**opt_hash), but I think it is better and clearer.

Updated by mame (Yusuke Endoh) 11 months ago

Here is a scenario where allowing "hash argument with omitted braces" causes a problem. Assume that we write a method "debug" which is equal to "Kernel#p".

def debug(*args)
  args.each {|arg| puts arg.inspect }
end

Passing a hash argument with omitted braces, unfortunately, works.

debug(key: 42) #=> {:key=>42}

Then, consider we improve the method to accept the output IO as a keyword parameter "output":

def debug(*args, output: $stdout)
  args.each {|arg| output.puts arg.inspect }
end

However, this change breaks the existing call.

debug(key: 42) #=> ArgumentError (unknown keyword: key)

This is too easy to break. So, what is bad? I believe that passing a hash argument as a normal last parameter is bad.

I'd like to make it safe to extend an existing method definition with a keyword parameter.

Updated by jeremyevans0 (Jeremy Evans) 11 months ago

mame (Yusuke Endoh) wrote:

jeremyevans0 (Jeremy Evans) wrote:

Having to change all calls from where(:id=>1) to where({:id=>1}) makes the code uglier and is going to make most Ruby programmers less happy. Does this argument for explicitness lead to requiring parentheses for all method calls?

In this specific case, it looks better to change the callee side instead of the caller side: the method definition of where should receive a keyword rest argument. Of course, it still requires us change some calls of where(opt_hash) to where(**opt_hash), but I think it is better and clearer.

Changing the callee side will not fix all cases. The where method supports more than just symbols keys in hashes. where('table.id'=>1) is supported, for example. Accepting a keyword args splat and then appending it to the array of arguments just decreases performance for no benefit.

It is important to realize that keyword arguments are not a substitute for hash arguments, as keyword arguments only handle a subset of what a hash argument can handle.

mame (Yusuke Endoh) wrote:

Here is a scenario where allowing "hash argument with omitted braces" causes a problem. Assume that we write a method "debug" which is equal to "Kernel#p".

def debug(*args)
  args.each {|arg| puts arg.inspect }
end

Passing a hash argument with omitted braces, unfortunately, works.

debug(key: 42) #=> {:key=>42}

Then, consider we improve the method to accept the output IO as a keyword parameter "output":

def debug(*args, output: $stdout)
  args.each {|arg| output.puts arg.inspect }
end

However, this change breaks the existing call.

Note how this problem does not occur until you add keyword arguments to the method. If you never add keyword arguments, you never run into this problem, and there are ways to add the support you want without using keyword arguments.

Are you assuming that all methods that use hash arguments will end up wanting to use keyword arguments at some point? I think that is unlikely. If keyword arguments are never added to the method in the future, then you have broken backwards compatibility now for no benefit.

You are implying it is better to certainly break tons of existing code now, to allow for a decreased possibility of breaking code later if and only if you decide to add keyword arguments.

This is too easy to break. So, what is bad? I believe that passing a hash argument as a normal last parameter is bad.

That is an opinion I do not share. I believe passing a hash argument as a normal last parameter is fine and one of the nice features that makes Ruby a beautiful language to write in. I think omitting braces for hash arguments has a natural similarity to the ability to omit parentheses for method calls, which is another Ruby feature that makes it enjoyable to write in.

I'd like to make it safe to extend an existing method definition with a keyword parameter.

Attempting to avoid backwards compatibility problems is a noble goal that I think we share. Part of that is avoiding future backwards compatibility problems. Another part of that is avoiding current backwards compatibility problems. A change that causes more current backwards compatibility problems than the future backwards compatibility problems it is designed to avoid is a step in the wrong direction, in my opinion.

Updated by mame (Yusuke Endoh) 11 months ago

jeremyevans0 (Jeremy Evans) wrote:

Changing the callee side will not fix all cases. The where method supports more than just symbols keys in hashes. where('table.id'=>1) is supported, for example. Accepting a keyword args splat and then appending it to the array of arguments just decreases performance for no benefit.

As an experiment, I'm now trying to check Ruby's existing APIs, and noticed that some methods had the issue: Kernel#spawn, JSON::GenericObject.from_hash, etc. It might be good to provide a variant of define_method for this case as a migration path:

define_last_hash_method(:foo) do |opt|
  p opt
end

foo(k: 1)     #=> {:k=>1}
foo("k"=>1) #=> {"k"=>1}

Are you assuming that all methods that use hash arguments will end up wanting to use keyword arguments at some point? I think that is unlikely. If keyword arguments are never added to the method in the future, then you have broken backwards compatibility now for no benefit.

I don't think that all methods will have keyword arguemnts eventually. However, I assume that we can never predict which methods will have.

I'd like to make it safe to extend an existing method definition with a keyword parameter.

Attempting to avoid backwards compatibility problems is a noble goal that I think we share. Part of that is avoiding future backwards compatibility problems. Another part of that is avoiding current backwards compatibility problems. A change that causes more current backwards compatibility problems than the future backwards compatibility problems it is designed to avoid is a step in the wrong direction, in my opinion.

In general, I agree. For this specific topic, however, the current spec and implementation are really a mess; the current backward compatibility problem is relatively easy to fix; the future backwards compatibility problem is hard to avoid and will become painful more and more. We should now pay the debt for the future.

But this is just my opinion. I really appreciate and respect your opinion. I'd like to tell matz your opinion as fairly as I can.

Updated by duerst (Martin Dürst) 11 months ago

jeremyevans0 (Jeremy Evans) wrote:

mame (Yusuke Endoh) wrote:

This change seems to remind you the breaking change of character encoding in 1.9/2.0, but it was much worse than this change because the previous one was not trivial "where to fix". The site where an error occurred was often different to the site where a wrong encoding string was created.

I disagree. I migrated many applications and libraries from Ruby 1.8 to Ruby 1.9 (and later to Ruby 2.6). The changes for Ruby 1.8 -> 1.9 were minimal in comparison with the impact of this change, in terms of the amount of code that needed to be modified.

I think the amount of changes from Ruby 1.8 to Ruby 1.9 depended a lot on what kind of processing your application did, and what kind of data was involved. If you mostly just worked with US-ASCII data, the changes needed were minimal. For other data, in particular also for Japanese data, some kinds of processing may have been heavily affected.

Updated by Eregon (Benoit Daloze) 10 months ago

I agree with Jeremy here, the current idea seems too incompatible and will require too many changes (no matter the gain).
And those changes cannot easily be automated either, they need careful considerations.

I think we need to compromise here, to avoid too many incompatible changes, especially on methods which have no keyword arguments and where the intention is clear.
I would think the number of methods like debug() is a tiny fraction of the number of places we'd need to change if hash-without-braces is no longer supported.

IMHO such a method with rest + kwargs seems a bad design in the first place as the arguments are too complex. That debug method could only accept one argument for instance.

Also, how should foo(1, "foo" => "bar") behave?
Should it be like foo(1, {"foo" => "bar"})? In this case the syntax is inconsistent with foo(1, foo: "bar") where having or leaving out the braces matter.
Or does the => imply the braces?
I believe all Rubyists are used to foo(1, :foo => "bar") and foo(1, foo: "bar") being identical.

BTW, p foo: 1 will no longer work then, and p({foo: 1}) would be required, which feels very unlike Ruby, and is just impractical when debugging.

Updated by mame (Yusuke Endoh) 10 months ago

Eregon (Benoit Daloze) wrote:

I would think the number of methods like debug() is a tiny fraction of the number of places we'd need to change if hash-without-braces is no longer supported.

IMHO such a method with rest + kwargs seems a bad design in the first place as the arguments are too complex. That debug method could only accept one argument for instance.

I think you are too familiar with the current weird keyword arguments. The original and primary purpose of keyword arguments is an extension of existing methods. It looks rather "too complex" for a mere addition of keyword parameters to disturb other parameters and to break existing calls.

That being said, I agree that the "cancer" of this issue is a combination of rest/optional agruments and keyword ones. Another, more modest idea that I have is, to prohibit (or just warn) a method definition that has both rest/optional + keyword parameters. I don't like this because this spoils the purpose of keyword arguments, though.

Also, how should foo(1, "foo" => "bar") behave?
Should it be like foo(1, {"foo" => "bar"})?

I think so.

In this case the syntax is inconsistent with foo(1, foo: "bar") where having or leaving out the braces matter.

Braced hash and bare one are inconsistent, even in the current spec.

def foo(v=:default)
  p v
end

h={}
foo( **h )    #=> {}
foo({**h})    #=> {}
foo(1,  **h ) #=> 1
foo(1, {**h}) #=> wrong number of arguments (given 2, expected 0..1)

Note that **{} does not simply mean "no argument". If it was "no argument", the above foo(**h) would print :default instead of {}.

Or does the => imply the braces?
I believe all Rubyists are used to foo(1, :foo => "bar") and foo(1, foo: "bar") being identical.

They will be still identical because it is determined not only syntactically but also dynamically: a key-value pair whose key is a Symbol, is handled as keyword argument. This behavior is not new. Ruby 2.5 even does it:

def foo(h1=nil, **h2)
  p [h1, h2]
end
foo("foo" => 1, :bar => 2, baz: 3) #=> [{"foo"=>1}, {:bar=>2, :baz=>3}]

(This behavior has been changed in trunk, but I'm unsure if it is determined or not.)

BTW, p foo: 1 will no longer work then, and p({foo: 1}) would be required, which feels very unlike Ruby, and is just impractical when debugging.

I completely agree with this. I showed the method debug as an example, but I don't think that Kernel#p itself should change. Some existing APIs that people expect to accept both foo(k:1) and foo({k:1}), e.g, ERB#result_with_hash, Sequel's where, should be kept as well.

Updated by mame (Yusuke Endoh) 10 months ago

mame (Yusuke Endoh) wrote:

BTW, p foo: 1 will no longer work then, and p({foo: 1}) would be required, which feels very unlike Ruby, and is just impractical when debugging.

I completely agree with this. I showed the method debug as an example, but I don't think that Kernel#p itself should change. Some existing APIs that people expect to accept both foo(k:1) and foo({k:1}), e.g, ERB#result_with_hash, Sequel's where, should be kept as well.

Half-joking: I'm not fully satisfied with p foo: 1 #=> {:foo=>1}. If a keyword argument is separated from other ones, it can emit a much better output:

def p(*args, **kw)
  args.each {|arg| puts arg.inspect }
  kw.each {|label, arg| puts "#{ label }: #{ arg.inspect }" }
end

p foo: 1, bar: {"A"=>"B"}, baz: {qux: 1}
#=> foo: 1
#   bar: {"A"=>"B"}
#   baz: {:qux=>1}
#36

Updated by marcandre (Marc-Andre Lafortune) 10 months ago

  • Related to deleted (Bug #12104: Procs keyword arguments affect value of previous argument)

Updated by marcandre (Marc-Andre Lafortune) 10 months ago

mame (Yusuke Endoh) wrote:

Braced hash and bare one are inconsistent, even in the current spec.

def foo(v=:default)
  p v
end

h={}
foo( **h )    #=> {}
foo({**h})    #=> {}
foo(1,  **h ) #=> 1
foo(1, {**h}) #=> wrong number of arguments (given 2, expected 0..1)

The fact that foo(**h) #=> {} is a bug. Note that foo(**{}) # => :default, as I believe it should. Both should have same result. See #15078.

def foo(h1=nil, **h2)
  p [h1, h2]
end
foo("foo" => 1, :bar => 2, baz: 3) #=> [{"foo"=>1}, {:bar=>2, :baz=>3}]

I believe this is currently a bug (#14130) and I hope this is not accepted in the future either. Is there a good use case for this anyways? I fear it only creates hard to find errors.

Updated by marcandre (Marc-Andre Lafortune) 10 months ago

Let me add my voice to that of Benoit and Jeremy: the incompatibility is absolutely not worth it.

I believe that if we fix the few remaining corner cases, improve the error messages and explicitly document how Ruby handles keyword parameters vs optional positional parameters, we'll have a really solid solution.

#39

Updated by mame (Yusuke Endoh) 10 months ago

  • Related to Bug #15078: Hash splat of empty hash should not create a positional argument. added

Updated by marcandre (Marc-Andre Lafortune) 10 months ago

After working a lot on **{}, I still strongly believe that we must maintain conversion of keyword arguments to positional argument, e.g.:

def foo(*ary)
end
foo(kw: 1) # => must remain ok

OTOH, it may be possible to disallow promotion of last positional argument to keyword arguments without causing as huge incompatiblities. Using **hash could be required, if given enough time (say warnings in Ruby 2.6 & 2.7)

def foo(**options); end
foo(hash) # => Could be disallowed, only foo(**hash) would work

A major consequence of disallowing promotion to keyword arguments is that the naive forwarding calls (only with *args) will no longer be always valid. This means that all forwarding calls, including those of delegate library, will have to become capture arguments with *args, **options. This means that the meaning of **{} will become more important than it currently is.

As I argue in #15078, it will be important that **{} doesn't create a positional argument so that full forwarding works even for normal methods.

My recommendation:

Ruby 2.6: Fix **{} to not create positional argument (#15078). Improve wording of ArgumentErrors

If we want to have stricter keyword arguments (I'm not sure it's worth it), then:

Ruby 2.6: In verbose mode, warn about promotion of positional argument to keyword arguments, recommending using hash splat.
Ruby 2.7: Same, even if not-verbose.
Ruby 3.0: Stop promoting normal argument to keyword argument.

.... after I'm long dead

Ruby 42.0: Stop demoting keyword argument to normal argument

Updated by akr (Akira Tanaka) 10 months ago

I have an idea to separate positional arguments and keyword arguments without incompatibility.

Basic idea is introducing an flag, keyword_given,
which means the last argument is a Hash object which represent keyword argument.
(The name, keyword_given, is inspired from block_given? method.)

The flag will be true if method call uses k => v, k: v, "k": v or **h and
all keys of the Hash object constructed from them are symbol.
(I think hash separation is not good idea.)

The flag is referenced by a new Ruby method (keyword_given?) and
C level function (rb_keyword_given_p).

This doesn't break anything because it just add new method (and new C function).

This makes the confusion of positional/keyword arguments solvable.
But I don't say the confusion is solvable easily (or by-default).
Programmers must use the flag carefully.

If we want to solve the confusion by default, we need to change
method invocation behavior incompatible way.
However positional/keyword separation by the flag makes possible to
change behavior incrementally.

If a method is changed to use keyword_given?,
only the method is changed.
We can discuss the situation about the actual method.

If Ruby-level method definition/invocation behavior is changed
(def m(h) end cannot receive m(:k=>0) for example),
it affects many applications.
However method definition/invocation behavior contains
several points which can refer the flag.
We can discuss how big/small incompatibility and
how big/small benefits for each one.

Updated by jeremyevans0 (Jeremy Evans) 10 months ago

akr (Akira Tanaka) wrote:

I have an idea to separate positional arguments and keyword arguments without incompatibility.

I like this idea of allowing per-method handling of arguments. Just to confirm my understanding of the proposal:

def m(*a)
  [keyword_given?, a]
end

m # => [false, []]
m(1) # => [false, [1]]
m({:a=>1}) # => [false, [{:a=>1}]]
m(:a=>1) # => [true, [{:a=>1}]]
m(a: 1) # => [true, [{:a=>1}]]
m("a": 1) # => [true, [{:a=>1}]]
m(**{a: 1}) # => [true, [{:a=>1}]] or ArgumentError ?
m(**{}) # => [true, [{}]], [true, []], or ArgumentError ?
m('a'=>1, :a=>1) # => [false, [{'a'=>1, :a=>1}]]
a = :a
m(a=>1) # => [true, [{:a=>1}]]

def m2(*a, **kw)
  [keyword_given?, a, kw]
end

m2 # => [false, [], {}]
m2(1) # => [false, [1], {}]
m2({:a=>1}) # => [false, [{:a=>1}], {}]
m2(:a=>1) # => [true, [], {:a=>1}]
m2(a: 1) # => [true, [], {:a=>1}]
m2("a": 1) # => [true, [], {:a=>1}]
m2(**{a: 1}) # => [true, [], {:a=>1}]
m2(**{}) # => [true, [], {}]
m2('a'=>1, :a=>1) # => [false, [{'a'=>1, :a=>1}], {}]
a = :a
m2(a=>1) # => [true, [], {:a=>1}]

def m3(a)
  [keyword_given?, a]
end

m3 # => ArgumentError
m3(1) # => [false, 1]
m3({:a=>1}) # => [false, {:a=>1}]
m3(:a=>1) # => [true, {:a=>1}]
m3(a: 1) # => [true, {:a=>1}]
m3("a": 1) # => [true, {:a=>1}]
m3(**{a: 1}) # => [true, {:a=>1}] or ArgumentError ?
m3(**{}) # => [true, {}] or ArgumentError ?
m3('a'=>1, :a=>1) # => [false, {'a'=>1, :a=>1}]
a = :a
m3(a=>1) # => [true, {:a=>1}]

def m4(**kw)
  [keyword_given?, kw]
end

m4 # => [true, {}] or [false, {}] ?
m4(1) # => ArgumentError
m4({:a=>1}) # => ArgumentError
m4(:a=>1) # => [true, {:a=>1}]
m4(a: 1) # => [true, {:a=>1}]
m4("a": 1) # => [true, {:a=>1}]
m4(**{a: 1}) # => [true, {:a=>1}]
m4(**{}) # => [true, {}] or [false, {}] ?
m4('a'=>1, :a=>1) # => ArgumentError
a = :a
m4(a=>1) # => [true, {:a=>1}]

If the method does not explicitly declare any keyword arguments, and the caller uses **hash is used to explicitly pass a keyword argument, does that raise an ArgumentError or does it pass the hash as a positional argument and have keyword_given? return true?

If the method does not explicitly declare any keyword arguments, does it pass the splatted empty hash as a positional argument, does it ignore it, or does it raise an ArgumentError?

If the method explicitly declares keyword arguments (either required keyword, optional keyword, or keyword splat), and is called without keyword arguments, does keyword_given? return true or false?

If the method explicitly declares keyword arguments and an empty hash is splatted, does keyword_given? return true or false?

Updated by akr (Akira Tanaka) 10 months ago

keyword_given? provides information about the caller side.
The information is not related to callee side.
There is no chance to call keyword_given? if ArgumentError is raised, though.

For simplicity, I assume keys for keyword argument is Symbol, here.
(If non-Symbol key is provided, keyword_given? returns false.)

jeremyevans0 (Jeremy Evans) wrote:

If the method does not explicitly declare any keyword arguments, and the caller uses **hash is used to explicitly pass a keyword argument, does that raise an ArgumentError or does it pass the hash as a positional argument and have keyword_given? return true?

keyword_given? return true.
We can discuss ArgumentError or not.

If the method does not explicitly declare any keyword arguments, does it pass the splatted empty hash as a positional argument, does it ignore it, or does it raise an ArgumentError?

If splatted empty hash means **{} in caller side, keyword_given? return true.
We can discuss ArgumentError or not.

If the method explicitly declares keyword arguments (either required keyword, optional keyword, or keyword
splat), and is called without keyword arguments, does keyword_given? return true or false?

keyword_given? return false.
I think there is no chance for keyword argument related ArgumentError if no required keyword.

If the method explicitly declares keyword arguments and an empty hash is splatted, does keyword_given? return true or false?

keyword_given? return true.

I assume **{} in caller add {} in arguments and keyword_given? return true.
However, another behavior is possible: **{} doesn't add {} in arguments and keyword_given? return false.
Their difference is visible until we have a way to obtain positional arguments and keyword argument in single array.
I choose former because I'm considering to distinguish them using **nil and `**{}'.
See details with https://bugs.ruby-lang.org/issues/15078#note-13

Updated by marcandre (Marc-Andre Lafortune) 10 months ago

Very interesting.

akr (Akira Tanaka) wrote:

The flag will be true if method call uses k => v, k: v, "k": v or **h and
all keys of the Hash object constructed from them are symbol.
(I think hash separation is not good idea.)

I agree that hash separation is not a good idea. I'm wondering if (k = :a) => v should be accepted. It is the only case that is not syntactical.

This makes the confusion of positional/keyword arguments solvable.
But I don't say the confusion is solvable easily (or by-default).
Programmers must use the flag carefully.

IIUC, the only 100% correct way to forward a method call (including the keyword_given? flag) would be:

def forward(*args, &block)
  if keyword_given?
    options = args.pop
    target(*args, **options, &block)
  else
    target(*args, &block)
  end
end

That is assuming the current **{} creating a positional argument.

Assuming Ruby 2.x compatiblity, there's no way of a general forward with **capture though. We'd need a method lash_argument_converted_to_keyword?...

def forward(*args, **options, &block)
  if keyword_given?
    target(*args, **options, &block)
  else
    args << options if last_argument_converted_to_keyword?
    target(*args, &block)
  end
end

Maybe the API could be combined in a single method keyword_style, returning one of [nil, :keyword, :hash_to_keyword, :keyword_to_hash]:

def foo(*); keyword_style; end
def bar(**); keyword_style; end
h = {}

foo      # => nil
foo(**h) # => :keyword_to_hash
bar(**h) # => :keyword
bar(h)   # => :hash_to_keyword

Or keyword_given? could return nil in case a hash was converted to a keyword argument like in bar(h)

If we distinguish **nil and **{} as in #15078, then there's no need to even call keyword_given? and normal forwarding works I imagine... But that might be quite incompatible.

Updated by mame (Yusuke Endoh) 8 months ago

I talked with matz about this proposal at Keep Ruby Weird conference (more precisely, Franklin BBQ at Austin). As far as I understand, Matz currently likes syntactical separation of keyword and normal arguments. (Note that it is not decided yet.)

Short summary:

def foo(**kw); p kw; end
def bar(kw = {}); p kw; end
h = {:k => 1}

# base (non-braced) hash arguments passed as keywords
foo(k: 1)    #=> {:k=>1} in 2.X and 3.0
foo(:k => 1) #=> {:k=>1} in 2.X and 3.0
foo(**h)     #=> {:k=>1} in 2.X and 3.0
bar(k: 1)    #=> {:k=>1} in 2.X, ArgumentError in 3.0
bar(:k => 1) #=> {:k=>1} in 2.X, ArgumentError in 3.0
bar(**h)     #=> {:k=>1} in 2.X, ArgumentError in 3.0

# braced hash arguments are passed as a last argument
foo({ k: 1 })    #=> {:k=>1} in 2.X, ArgumentError in 3.0
foo({ :k => 1 }) #=> {:k=>1} in 2.X, ArgumentError in 3.0
foo(h)           #=> {:k=>1} in 2.X, ArgumentError in 3.0
bar({ k: 1 })    #=> {:k=>1} in 2.X and 3.0
bar({ :k => 1 }) #=> {:k=>1} in 2.X and 3.0
bar(h)           #=> {:k=>1} in 2.X and 3.0

Unfortunately, this change will break many existing programs. But, it would be still easy to fix. We can pick up keywords or normal hash explicitly for each callsite that an error occurred. In many cases, keywords would be preferable: just change from def foo(h = {}) to def foo(**h), and from foo(h) to foo(**h).

And, this is a new topic. There is an non-Symbol-key call, like where("table.id" => 1) (which was shown by Jeremy Evans). This is difficult to change to keyword argument. To allow this, matz came up with an idea: non-Symbol key is also allowed as a keyword.

def foo(**kw)
  p kw
end

foo("str" => 42) #=> {"str"=>42}

Note that, if you need to write a library that works on both 2.X and 3.X, you must write a shim:

def foo(kw1 = {}, **kw2)
  kw = kw1.merge(kw2)
  kw
end

However, after EOL of all Ruby 2.X series, you can remove the shim and just write a simple code. This is better than my original proposal.

What do you think?

Updated by jeremyevans0 (Jeremy Evans) 8 months ago

mame (Yusuke Endoh) wrote:

And, this is a new topic. There is an non-Symbol-key call, like where("table.id" => 1) (which was shown by Jeremy Evans). This is difficult to change to keyword argument. To allow this, matz came up with an idea: non-Symbol key is also allowed as a keyword.

def foo(**kw)
  p kw
end

foo("str" => 42) #=> {"str"=>42}

Note that, if you need to write a library that works on both 2.X and 3.X, you must write a shim:

def foo(kw1 = {}, **kw2)
  kw = kw1.merge(kw2)
  kw
end

However, after EOL of all Ruby 2.X series, you can remove the shim and just write a simple code. This is better than my original proposal.

What do you think?

I agree with the proposed changes to foo-like (keyword arguments) methods, as those changes actually solve real problems with keyword arguments.

I disagree with the proposed changes to bar-like (positional hash arguments) methods. Those changes do not solve existing problems, they just break existing code for the potential future ability to introduce keyword arguments without behavior changes.

Let's consider if this proposed changes to bar-like (positional hash arguments) methods is accepted. The main argument for acceptance is the ability to introduce keyword arguments without behavior changes. However, in many if not most cases where bar-like methods are used, keyword arguments would be used to replace the option hashes (something that works OK in 2.X except for corner cases with optional position arguments and argument splats), not as an addition to option hashes. With the changes discussed to foo-like methods, you would no longer be able to replace an option hash argument with keyword arguments in a backwards compatible manner. So the fact that the proposed changes to bar-like methods allow keyword arguments to be introduced in a backwards compatible manner will not help, since replacing the option hashes with keywords will still be a backwards incompatible change.

There are many cases where you have a method that accepts a hash where you have cases where you want to pass an existing hash and other cases where you want pass a new hash, and having to add braces to all call-sites where you currently can omit them would be annoying, add no value, and make the code slightly harder to read.

The performance disadvantages to keyword splats that I discussed earlier still have not be addressed, and it is still impossible to create a method that accepts arbitrary keyword arguments and delegates the call to another method that accepts arbitrary keyword arguments without at least 3 hash allocations per-call (and you can have 0 hash allocations per call with a option-hash based approach).

In conclusion, the ability to add keyword arguments in a backwards compatible manner to methods that accept option hashes adds very little benefit. I think there are huge costs in breaking existing compatibility (potentially leading to a Python 2/3-like situation in libraries), and other costs in making code using bar-like methods harder to read (by requiring braces), as well as hurting performance by encouraging unoptimized keyword splats as a replacement for option hashes.

The ability for **kw to accept non-Symbol keys would make it a slightly easier to convert option hash methods to keyword arguments methods, but the keyword argument approach would still perform worse due to the additional hash allocations, and converting option hashes to keyword splats would still not be backwards compatible, and I think in most cases using an options hash would still be the preferable approach. So even if **kw handled non-Symbol keys, I would still be strongly against changing the behavior for bar-like methods.

Updated by decuplet (Nikita Shilnikov) 7 months ago

I'm not sure if this was discussed but one more thing to consider is hash destructuring using keywords. As far as I understand there's a use case I rely on which is going to be broken by the proposed changes. Specifically, things like

xs = [a: 1, b: 2, c: 3]
xs.map { |a:, b:, c:| ... }

Perhaps it makes sense to make hash destructuring a separate feature with the following syntax

xs.map { |{a, b, c}| ... }
xs.map { |{a, **rest}| ... }

But it seems to be a separate feature and I don't see how this can help with other compatibility issues mentioned here.

Updated by ioquatix (Samuel Williams) 7 months ago

I agree we should fix this issue. It is very much unexpected behaviour and even context sensitive behaviour. Double splat operator should be required in all cases to turn hash into keyword arguments.

Updated by mame (Yusuke Endoh) 4 months ago

Sorry for leaving this ticket. Matz, akr and I talked about this issue several times since the last year, and we have never reached a perfect solution. But I try to re-summarize the problem, current proposal, and migration path.

Problem

The current spec of keyword arguments is broken in several senses.

1. Keyword extension is not always safe

We call "keyword extension" to add a keyword parameter to an existing method.
Unfortunately, keyword extension is not safe when the existing method accepts rest arguments.

def foo(*args)
  p args
end
foo(key: 42) #=> [{:key=>42}]

If we add a new mode to the method, the existing call will break.

def foo(*args, output: $stdout)
  output.puts args.inspect
end
foo(key: 42) #=> unknown keyword: key

Safe keyword extension is a fundamental expectation for keyword arguments, so that is a pity.

2. Explicit Delegation of keywords backfires

You are writing a delegation, and you think of keywords, so you wrote:

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

However, this does not work correctly.

def bar(*args)
  p args
end

foo() #=> excepted:[], actual:[{}]

3. There are many unintuitive corner cases

There are many bug reports about keyword arguments. One of the most weird cases:

def foo(opt=42, **kw)
  p [opt, kw]
end

foo({}, **{})  #=> expected:[{}, {}], actual:[42, {}]

All of these issues are caused by the fundamental design flaw of the current keyword arguments which handles a keyword as a last positional argument that is a Hash object. Matz, akr and I have considered these issues seriously. Actually, matz came up with multiple ideas that would be compatible (or mildly incompatible) and solve the issues. However, all of them were proved to be incompatible, complex, and/or not to solve some of the above issues.

Proposal for 3.X semantics

The current proposal consists of two parts:

A) Separate keyword arguments from positional arguments completely
B) Allow non-Symbol keys as a keyword

(A) is the original proposal of this ticket.

  • A keyword argument is passed only by foo(k: 1) or foo(**opt), and accepted only by def foo(k: 1) or def foo(**opt).
  • A positional Hash argument is passed only by foo({ k: 1 }) or foo(opt), and accepted only by def foo(opt) or def foo(opt={}) or def foo(*args)

See the next section in detail.

(B) allows some DSL usages of brace omission:

def where(**kw)
  p kw
end

where("table.id" => 42) #=> {"table.id"=>42}

Actually, this behavior is not new. Ruby 2.0.0-p0 allowed non-Symbol keys.

Typical rewrite cases

This change brings incompatibility, so you need to rewrite existing code. Typical rewrite cases are three (plus one):

1. Accept keywords by **opt, not by opt={}

# NG in 3.X
def foo(opt={})
end

# OK in 3.X
def foo(**opt)
end

2. Pass keywords without braces, or with explicit **

def foo(**opt)
end

# NG in 3.X
foo({ k: 1 })
h = { k: 1 }
foo(h)

# OK in 3.X
foo(k: 1)
foo(**h)

3. Delegate keyword argument explicitly

# NG in 3.X
def foo(*args, &blk)
  bar(*args, &blk)
end

# OK in 3.X
def foo(*args, **kw, &blk)
  bar(*args, **kw, &blk)
end

Plus one. Manually merge the last argument with a keyword argument

If you want to allow both calling styles, you can do it manually.

# NG in 3.X
def foo(opt={})
  p opt
end
foo({ k: 1 }) #=> {:k=>1}
foo(k: 1)     #=> expected:{:k=>1}, actual:error

# OK in 3.X
def foo(opt={}, **kw)
  opt = opt.merge(kw)
  p opt
end
foo({ k: 1 }) #=> {:k=>1}
foo(k: 1)     #=> {:k=>1}

Migration path: 2.7 semantics

Basic approach:

  • If a code is valid (no exception raised) in 3.X, Ruby 2.7 should run it in the same way as 3.X
  • If a code is invalid (an exception raised) in 3.X, Ruby 2.7 should run it in the same way as 2.6, but a warning is printed

Typical examples:

def foo(opt)
end
foo(k: 1) #=> test.rb:3: warning: The keyword argument for `foo' (defined at test.rb:1) is used as the last parameter
def foo(**opt)
end
foo({ k: 1 }) #=> test.rb:3: warning: The last argument for `foo' (defined at test.rb:1) is used as the keyword parameter

These warnings tell users how to fix the source code.

(A naive implementation of this approach is not enough. Very subtle hack is required for delegation. This is explained in the last appendix section.)

Experiment

I have implemented 2.7's candidate semantics:

https://github.com/ruby/ruby/compare/trunk...mame:keyword-argument-separation

And I actually modified the standard libraries and tests to support the keyword argument separation. Many of the changes are one of the three (plus one) typical rewrite cases. There are a few tricky modifications, but in my opinion, almost all of them were trivial.

In addition, I tested an internal Rails app in my company (about 10k lines) with my prototype. Honestly speaking, when running rake spec, it produces about 120k (!) warnings, but there are many duplicated warnings. By removing the duplications, we got about 1k warnings. And, I found that almost all warnings were produced in gems. If we focus on only the application itself, we found only five method definitions to be modified. All fixes were the first typical rewrite case: def foo(opt={}) -> def foo(**opt)). We will need to rewrite some more calls to add an explicit ** if some libraries decided that their APIs only accept keywords.

Appendix: Special frozen Hash object for delegation

Unfortunately, the naive implementation of the migration path is incomplete with regard to delegation.
Consider the following code.

# in 2.7
def f1(k: 1)
  p k
end

def f2(*args)
  p args
end

def dispatch(target, *args, &blk)
  if target == :f1
    f1(*args, &blk)
  else
    f2(*args, &blk)
  end
end

dispatch(:f1, k: 1) #=> 1
#=> t.rb:17: warning: The keyword argument for `dispatch' (defined at t.rb:9) is used as the last parameter
#   t.rb:11: warning: The last argument for `f1' (defined at t.rb:1) is used as the keyword parameter
#   1

dispatch(:f2, 1, 2, 3) #=> [1, 2, 3]

You see a warning, so you rewrite it by explicit keyword delegation:

# in 2.7
def f1(k: 1)
  p k
end

def f2(*args)
  p args
end

def dispatch(target, *args, **kw, &blk)
  if target == :f1
    f1(*args, **kw, &blk)
  else
    f2(*args, **kw, &blk)
  end
end

dispatch(:f1, k: 1)    #=> 1
dispatch(:f2, 1, 2, 3) #=> [1, 2, 3, {}]
#=> t.rb:18: warning: The keyword argument for `f2` (defined at t.rb:4) is used as the last parameter

dispatch(:f1, k: 1) works perfectly with no warnings. However, the result of dispatch(:f2, 1, 2, 3) changed and a new warning is emitted. This is because **kw was automatically converted to a positional argument (due to 2.6 compatibility layer).

To fix this issue, we introduce a Hash flag to distinguish between "no keyword given" and "empty keyword given".

def foo(**kw)
  p kw
end

foo(**{}) #=> {}
foo()     #=> {(NO KEYWORD)}

{} is a normal empty hash object, and {(NO KEYWORD)} is the special empty hash object that represents "no keyword given".

If we pass the flagged empty hash to another method with ** operator, it is omitted.

def bar(*args)
  p args
end

def foo(**kw)
  # kw is {(NO KEYWORD)}
  bar(**kw) # **{(NO KEYWORD)} is equal to nothing: bar()
end

foo({}) #=> [{}]
foo()   #=> []

This is akr's idea that was explained at https://bugs.ruby-lang.org/issues/14183#note-41.

This hack of special empty hash flag is temporal just during the migration. After 3.X completes the separation of keyword arguments, this dirty hack can be removed.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

mame,

Thanks for your continued work on this.

I still agree that for methods that accept keyword arguments, we should make changes to avoid the problems that currently exist for keyword arguments.

I still believe that breaking all backwards compatibility for methods that do not currently accept keyword arguments, just to allow keyword arguments to be added safely in the future, is not a worthy tradeoff, for the following reasons:

  • Many if not most of the methods may never be converted to keyword arguments, in which case backwards compatibility is broken for no benefit.

  • This encourages the use of keyword arguments, while the use of keyword arguments hurts performance in all cases where keyword splats are used (either at the caller side or the callee side). The option hash approach can be made faster and allocation-less, while all keyword splats are currently slower as they require allocations. I'm not sure that the keyword argument splat performance issues could be fixed without breaking backwards compatibility for all keyword argument splats.

  • For methods that currently use option hashes, requiring braces around the option hash can make it more difficult to convert to keyword arguments, not less. A method such as def foo(opts={}) end that is usually called using foo(bar: 1), will still work if you switch to keyword arguments: def foo(**opts) end. It is true that the foo(hash) calling style would require modifications with the switch to keyword arguments, though.

I think the biggest problem with keeping backwards compatibility for methods that do not accept keyword arguments is handling delegation.

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

I believe with your proposal, this is expected to work regardless of whether bar accepts keyword arguments. If bar doesn't accept keyword arguments, then calling foo with a keyword argument will raise an exception when foo calls bar. I think one possible way to get that simple delegation to work would be to allow double-splat when calling methods that do not accept keyword arguments (keep backwards compatibility). For example, allow this:

def bar(hash={})
  hash[:a]
end

bar(**{a: 1})
# => 1

foo(**{a: 1})
# => 1

This keeps backwards compatibility back to Ruby 2.0. It will also make it easier to transition such code to keyword arguments later without breaking backwards compatibility, since changing the definition of bar to def bar(**hash) hash[:a] end would still work in that case.

The main problematic case would be if bar accepted a positional splat but did not accept keyword arguments, where an empty hash would be provided if no keyword arguments were used:

def bar(*a)
  a
end

bar
# => []

foo
# => [{}]

One possible way around that would be that if a method accepts a positional splat and does not accept keyword arguments, then calling the method with an empty keyword argument splat would not pass a positional argument. Proposed behavior:

def bar(*a)
  a
end

bar
# => []

foo
# => []

bar(**{})
# => []

foo(**{})
# => []

bar(1, a: 1)
# => [1, {a: 1}]

foo(1, a: 1)
# => [1, {a: 1}]

My Proposed Alternative

To sum up, here is my proposed alternative approach:

  • For methods that accept keyword arguments, the same as your proposal
  • For methods that do not accept keyword arguments:
    • Allow braceless hashes as positional arguments (keep backwards compatibility)
    • Allow **keyword splats
      • If keyword is empty hash, do not add the empty hash positional argument (new behavior)
      • Otherwise, add keyword as positional hash argument (keep backwards compatibility)

I think this alternative proposal handles "2. Explicit Delegation of keywords backfires" and "3. There are many unintuitive corner cases". It does not handle "1. Keyword extension is not always safe". However, I believe you could keep safe keyword extension if using keyword splat, using an approach that works and is backwards compatible to Ruby 2.0. From your example:

# Before
def foo(*args)
  p args
end
foo(key: 42)
# => [{:key=>42}]

# Add keyword arguments
def foo(*args, output: $stdout, **kw)
  args << kw
  output.puts args.inspect
end
foo(key: 42)
# => [{:key=>42}]

Issues with keyword-argument-separation branch

In terms of the specific implementation in your keyword-argument-separation branch:

The rb_no_keyword_hash approach breaks modification of the hash, which I believe is unexpected:

def foo(**opts)
  opts
end

foo
#  => {(NO KEYWORD)}

def foo(**opts)
  opts[:a] = 1
  opts
end

foo
# FrozenError (can't modify frozen Hash)

It may be possible to work around that by setting a flag on the hash instead of using a shared frozen hash, assuming there is a spare flag we can use for that purpose. If a flag isn't available, we probably could use an instance variable that doesn't start with @ (making it only visible to C).

The warning seems inconsistent. For positional splats, you get warned if the braceless hash is the first argument, but not if it is a subsequent argument:

def bar(*a)
  a
end

bar
=> []

bar(a: 1)
# warning: The keyword argument for `bar' (defined at XXX) is used as the last parameter
# => [{:a=>1}]

bar(1, a: 1)
# => [1, {:a=>1}]

This situation also occurs for methods without splats where both arguments are optional (and maybe other cases):

def baz(a=1, b={})
  [a, b]
end

baz
# => [1, {}]

baz(a: 2)
# warning: The keyword argument for `baz' (defined at XXX) is used as the last parameter
[{:a=>2}, {}]

baz(1, a: 2)
# => [1, {:a=>2}]

Is that behavior in regards to warnings expected?

Behavior is different for methods defined in C, as C methods are always passed a hash, so the brace, braceless, and splat forms all work:

String.new(capacity: 1000)
# => ""
String.new({capacity: 1000})
# => ""
String.new(**{capacity: 1000})
# => ""

This results in inconsistent behavior depending how how the method is defined. This will lead to backwards compatibility problems if you move a method definition from C to ruby, or if you have a method defined in both C and ruby, with the pure ruby version used as a fallback if the C version cannot be used.

I look forward to discussing this issue in person at the developer meeting next month.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

Since I think it is best to back proposed behavior changes with a proposed implementation, attached is a patch based on mame's keyword-argument-separation branch that implements my proposal:

  • Same behavior as mame's for methods that accept keyword arguments
  • For methods that do not accept keyword arguments
    • Allow use of braceless hash without warning (keep backwards compatibility)
    • Allow **keyword splats
      • If keyword splat is empty, do not add positional argument (new behavior)
      • Otherwise, add hash as positional argument (keep backwards compatibility)

mame, if you have the time, could you try this patch with your internal Rails app, using the same checkout that resulted in about 120k warnings, and see how many warnings it causes and whether not adding a positional argument for an empty keyword splat breaks any code?

I'm not sure the patch is the best approach possible. I have limited knowledge of and experience with the VM internals. This patch is the minimum change necessary, it doesn't remove the rb_no_keyword_hash variable, even though I don't think the variable is needed if we do not pass positional arguments for empty keyword splats.

Updated by mame (Yusuke Endoh) 4 months ago

Jeremy,
I really appreciate you to use time for this issue. And sorry for my late response.

I have misunderstood some points of your proposal, and now I feel that it is fairly good. But please let me consider for a while... This topic is really hard to exhaust corner cases.

My Proposed Alternative

Just confirm. I think your following snippet lacks unless kw.empty?, right?

# Add keyword arguments
def foo(*args, output: $stdout, **kw)
  args << kw unless kw.empty? # This "unless" modifiler is needed, I think.
  output.puts args.inspect
end
foo(key: 42)
# => [{:key=>42}]

And, foo({}) will assign args = [{}], right? If so, your proposal looks good enough to me.
Of course, if there was a call foo(output: 42) before adding keywords, the call will break.
That is unfortnate, but this may be a good compromise.

Issues with keyword-argument-separation branch

Thank you for checking my prototype deeply!

The rb_no_keyword_hash approach breaks modification of the hash, which I believe is unexpected:

Yes. Akr and I knew that this would bring some incompatibility. We expected that the incompatibility should be small, but I noticed that it doesn't, unfortunately. It should be fixed by something like special instance variable, as you said.

The warning seems inconsistent. For positional splats, you get warned if the braceless hash is the first argument, but not if it is a subsequent argument:

Good catch, I didn't intend it. I fixed my branch. And this brings more warnings ;-) so I re-examined our internal Rails app (explained later). And the modification of my branch made your patch inapplicable, so I'm attaching a modified version of your patch.

Behavior is different for methods defined in C, as C methods are always passed a hash, so the brace, braceless, and splat forms all work:

We will keep the compatibility of C API because it would be more difficult to fix. Regardless of whether the brace is used, C method consisitently receives a hash.
By the way, ko1 is now working on replacement of built-in methods from C to Ruby. (He will talk about this plan in RubyKaigi: Write a Ruby interpreter in Ruby for Ruby 3.)
His main motivation is performance, but this will also reduce the problem of C methods that receives keyword arguments.

And, thank you for your alternative patch. I tried it with our internal Rails app again. It emitted about 8k warnings (much less than 120k!). Unfortunately I have no enough time to analyze the result, but it looks that no modification is required in our code base. Great.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

mame (Yusuke Endoh) wrote:

My Proposed Alternative

Just confirm. I think your following snippet lacks unless kw.empty?, right?

Correct. Sorry about that.

# Add keyword arguments
def foo(*args, output: $stdout, **kw)
  args << kw unless kw.empty? # This "unless" modifiler is needed, I think.
  output.puts args.inspect
end
foo(key: 42)
# => [{:key=>42}]

And, foo({}) will assign args = [{}], right?

Correct, in Ruby 3 assuming the behavior changes for keyword arguments are in effect. With your branch+my patch:

foo({})
# warning: The last argument for `foo' (defined at (irb):1) is used as the keyword parameter
# output: []

With your branch+my patch, you can work around the warning by passing an empty keyword splat:

foo({}, **{})
# output: [{}]

If so, your proposal looks good enough to me.

Great!

Issues with keyword-argument-separation branch

Thank you for checking my prototype deeply!

The rb_no_keyword_hash approach breaks modification of the hash, which I believe is unexpected:

Yes. Akr and I knew that this would bring some incompatibility. We expected that the incompatibility should be small, but I noticed that it doesn't, unfortunately. It should be fixed by something like special instance variable, as you said.

One issue with the special instance variable approach is that if you add an entry to the keyword hash, you probably do want to pass the keyword arguments even if the instance variable is present. So you would want to also check that the hash is still empty. Example:

def foo(*a, **kw)
  kw[:b] = 1 if a.length == 1
  bar(*a, **kw)
end

def bar(*a)
  a
end

foo
# => []

foo(1)
# => [1, {:b=>1}]

In my patch, we skip passing all empty keyword argument splats as hashes, so it should already handle this case (once the keyword splat hash is no longer frozen).

The warning seems inconsistent. For positional splats, you get warned if the braceless hash is the first argument, but not if it is a subsequent argument:

Good catch, I didn't intend it. I fixed my branch. And this brings more warnings ;-) so I re-examined our internal Rails app (explained later). And the modification of my branch made your patch inapplicable, so I'm attaching a modified version of your patch.

Thank you, I will try to do some more testing with your revised branch and the modified patch next week.

Behavior is different for methods defined in C, as C methods are always passed a hash, so the brace, braceless, and splat forms all work:

We will keep the compatibility of C API because it would be more difficult to fix. Regardless of whether the brace is used, C method consisitently receives a hash.

I figured that would be difficult to change. I think my patch would make C-methods perform the same as Ruby methods without keywords, which is probably best for compatibility other Ruby implementations that do not implement the C-API and use alternatives written in Ruby.

By the way, ko1 is now working on replacement of built-in methods from C to Ruby. (He will talk about this plan in RubyKaigi: Write a Ruby interpreter in Ruby for Ruby 3.)
His main motivation is performance, but this will also reduce the problem of C methods that receives keyword arguments.

That is very interesting. I will make sure to attend ko1's presentation.

And, thank you for your alternative patch. I tried it with our internal Rails app again. It emitted about 8k warnings (much less than 120k!). Unfortunately I have no enough time to analyze the result, but it looks that no modification is required in our code base. Great.

It is great to hear that it required no changes in your app's code, only requiring changes in gems that are passing hashes to methods that expect keywords.

Updated by matz (Yukihiro Matsumoto) 4 months ago

jeremyevans0 (Jeremy Evans) I will investigate your proposal. I was not fully satisfied with the complete separation model proposed for 3.0, but I didn't think of any other model which is intuitive and clean, especially considering static type analysis. Your proposal could be a better alternative.

Matz.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

jeremyevans0 (Jeremy Evans) wrote:

mame (Yusuke Endoh) wrote:

Good catch, I didn't intend it. I fixed my branch. And this brings more warnings ;-) so I re-examined our internal Rails app (explained later). And the modification of my branch made your patch inapplicable, so I'm attaching a modified version of your patch.

Thank you, I will try to do some more testing with your revised branch and the modified patch next week.

mame,

With your revised branch, it looks like the the keyword argument separation for positional splats has already happened, and there is no warning. Both your revised branch and your previous branch also already implement keyword argument separation for optional positional arguments without a warning. There is still a warning for the case where all positional arguments are required, though.

Example code:

def foo(a, *b, **c)
  [a, b, c]
end

def bar(a, b=1, **c)
  [a, b, c]
end

def baz(a, **c)
  [a, c]
end

Your revised branch (commit 73a9633114ef00bf793d7ca39e49f24448499487)

foo(1, {a: 1})
# => [1, [{:a=>1}], {(NO KEYWORD)}]

bar(1, {a: 1})
# => [1, {:a=>1}, {(NO KEYWORD)}]

baz(1, {a: 1})
# warning: The last argument for `baz' (defined at (irb):9) is used as the keyword parameter
# => [1, {:a=>1}]

Your previous branch (commit 3903e75678eca4874e3122a42bd073b018f9458e):

foo(1, {a: 1})
# warning: The last argument for `foo' (defined at (irb):15) is used as the keyword parameter    
# => [1, [], {:a=>1}]

bar(1, {a: 1})
# => [1, {:a=>1}, {(NO KEYWORD)}]

baz(1, {a: 1})
# warning: The last argument for `baz' (defined at (irb):9) is used as the keyword parameter
# => [1, {:a=>1}]

Ruby 2.6:

foo(1, {a: 1})
# => [1, [], {:a=>1}]

bar(1, {a: 1})
# => [1, 1, {:a=>1}]

baz(1, {a: 1})
# => [1, {:a=>1}]

I believe the expected behavior in Ruby 2.7 is to warn but return the same results as Ruby 2.6 in all three cases, is that correct?

I applied your vm_args_v2.diff on top of your revised branch, and also removed the rb_no_keyword_hash variable and related handling. No compilation issues, and some basic tests work, but many stdlib tests fail due to the keyword argument separation already being applied for methods that use positional splats (mostly tmpdir and csv).

To make testing easier, I uploaded my branch GitHub: https://github.com/jeremyevans/ruby/commits/keyword-argument-separation

After the issues with positional splats and optional positional arguments are fixed, I'll rebase my patch on top of that. Note that my branch does not include your changes to the standard library and tests to avoid warnings. I believe the changes required to the standard library and tests should be much less extensive with my proposal, and I would like to only make the minimum changes necessary. I want to make sure the branch does not cause any failures before attempting to remove warnings.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

I have updated my GitHub branch (https://github.com/jeremyevans/ruby/commits/keyword-argument-separation) to fix the issues in mame's branch that I identified in my previous comment.

Now, my branch keeps compatibility with Ruby 2.6 in regards to treating a hash argument as keywords, issuing warnings as expected for all three cases where behavior will change in Ruby 3:

def foo(a, *b, **c)
  [a, b, c]
end

def bar(a, b=1, **c)
  [a, b, c]
end

def baz(a, **c)
  [a, c]
end

foo(1, {a: 1})
# warning: The last argument for `foo' (defined at (irb):1) is used as the keyword parameter
# => [1, [], {:a=>1}]

bar(1, {a: 1})
# warning: The last argument for `bar' (defined at (irb):5) is used as the keyword parameter
# => [1, 1, {:a=>1}]

baz(1, {a: 1})
# warning: The last argument for `baz' (defined at (irb):9) is used as the keyword parameter
# => [1, {:a=>1}]

I have also found another behavior change with mame's branch that I think is undesirable, and that is how positional hash arguments with non-Symbol keys are converted to keywords. That breaks backwards compatibility with Ruby 2.6, and there is no reason to change behavior and emit a warning in Ruby 2.7 when the Ruby 3 behavior will be same as Ruby 2.6 in this case.

Ruby 2.6 and my branch:

def a(x=1, **h)
  [x, h]
end

a({:a=>1})
# => [{}, {:a=>1}]

a({"a"=>1})
# [{"a"=>1}, {}]

There is still backwards compatibility breakage in cases where the last positional hash has both Symbol keys and non-Symbol keys. In Ruby 2.6, those hashes would be split, but I'm not sure if we want to keep backwards compatibility and warn about that case in Ruby 2.7. Doing so would probably increase complexity significantly. My branch changes the behavior so that the positional hash is always treated as a positional argument if it contains a non-Symbol key:

Ruby 2.6:

a({"a"=>1, :a=>1})
# => [{"a"=>1}, {:a=>1}]

My branch:

a({"a"=>1, :a=>1})
# => [{"a"=>1, :a=>1}, {}]

I have fixed all issues in lib that caused warnings, and no changes are required in ext. Most changes were in the csv library, with minor changes in net, rdoc, rubygems, tempfile, and tmpdir. Here is a stat for issues in lib:

 lib/csv.rb                             | 36 ++++++++++++++++++------------------
 lib/csv/core_ext/array.rb              |  2 +-
 lib/csv/core_ext/string.rb             |  2 +-
 lib/csv/row.rb                         |  2 +-
 lib/csv/table.rb                       |  4 ++--
 lib/net/ftp.rb                         |  2 +-
 lib/net/protocol.rb                    |  2 +-
 lib/rdoc/generator/darkfish.rb         | 12 ++++++------
 lib/rubygems.rb                        |  2 +-
 lib/rubygems/commands/setup_command.rb |  2 +-
 lib/rubygems/package.rb                |  2 +-
 lib/tempfile.rb                        |  4 ++--
 lib/tmpdir.rb                          |  4 ++--

Here's a comparison with the changes mame's branch requires in lib and ext:

 ext/etc/extconf.rb                                     |   2 +-
 ext/json/lib/json/common.rb                            |  28 +++++++++++++++-------------
 ext/json/lib/json/generic_object.rb                    |   4 ++--
 ext/openssl/lib/openssl/ssl.rb                         |   4 ++--
 ext/psych/lib/psych.rb                                 |   2 +-
 ext/psych/lib/psych/core_ext.rb                        |   4 ++--
 lib/bundler/dsl.rb                                     |   6 +++---
 lib/bundler/runtime.rb                                 |   2 +-
 lib/cgi/core.rb                                        |   7 ++++---
 lib/cgi/html.rb                                        |  11 ++++++-----
 lib/csv.rb                                             |  24 ++++++++++++------------
 lib/csv/core_ext/array.rb                              |   2 +-
 lib/csv/core_ext/string.rb                             |   2 +-
 lib/csv/row.rb                                         |   2 +-
 lib/csv/table.rb                                       |   4 ++--
 lib/erb.rb                                             |   3 ++-
 lib/mkmf.rb                                            |   4 ++--
 lib/net/ftp.rb                                         |  10 +++++-----
 lib/net/http.rb                                        |   5 ++---
 lib/net/http/generic_request.rb                        |   2 +-
 lib/net/imap.rb                                        |   2 +-
 lib/net/protocol.rb                                    |   2 +-
 lib/open-uri.rb                                        |  23 ++++++++++++-----------
 lib/open3.rb                                           |  96 +++++++++++++++++++++++++-----------------------------------------------------------------------
 lib/rdoc/generator/darkfish.rb                         |  18 +++++++++---------
 lib/resolv.rb                                          |  18 +++++++-----------
 lib/rexml/document.rb                                  |   6 ++----
 lib/rexml/xpath.rb                                     |   2 +-
 lib/rss/parser.rb                                      |   7 +------
 lib/rss/rss.rb                                         |   9 ++-------
 lib/rubygems.rb                                        |   6 +++---
 lib/rubygems/command.rb                                |   4 ++--
 lib/rubygems/commands/install_command.rb               |   4 ++--
 lib/rubygems/commands/pristine_command.rb              |   4 ++--
 lib/rubygems/commands/setup_command.rb                 |   2 +-
 lib/rubygems/dependency_installer.rb                   |   4 ++--
 lib/rubygems/installer.rb                              |   8 ++++----
 lib/rubygems/package.rb                                |   2 +-
 lib/rubygems/request_set.rb                            |  10 +++++-----
 lib/rubygems/request_set/gem_dependency_api.rb         |  11 +++++------
 lib/rubygems/resolver/git_specification.rb             |   4 ++--
 lib/rubygems/resolver/lock_specification.rb            |   2 +-
 lib/rubygems/resolver/specification.rb                 |   8 ++++----
 lib/rubygems/test_case.rb                              |  18 +++++++++---------
 lib/rubygems/test_utilities.rb                         |  14 +++++++-------
 lib/tempfile.rb                                        |   8 ++++----
 lib/uri/file.rb                                        |   4 ++--
 lib/uri/ftp.rb                                         |   2 +-
 lib/uri/generic.rb                                     |  22 +++++++++++-----------
 lib/uri/http.rb                                        |   6 +++---
 lib/uri/mailto.rb                                      |   2 +-
 lib/yaml/store.rb                                      |   4 ++--

I have fixed all issues in test that caused warnings. These were also much less extensive than in mame's branch.

I have fixed a few broken tests that expected a specific format for warning messages for invalid keywords, as well as a test that assumed TypeError for a non-Symbol keyword (an ArgumentError is now used).

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

I have updated my branch (https://github.com/jeremyevans/ruby/commits/keyword-argument-separation) to restore backwards compatibility for methods using keyword arguments when calling with a final positional hash with mixed Symbol and non-Symbol keys. These calls are now handled like Ruby 2.6, splitting the hash into a positional hash for the non-Symbol keys and using the Symbol keys as keywords. A warning is added so that developers know they need to update their code, as in Ruby 3 this will always be treated as a positional argument without being split.

Example (same behavior as Ruby 2.6 except for warnings):

def a(x=1, **h)
  [x, h]
end

a({:a=>1})
# (irb):5: warning: The last argument for `a' (defined at (irb):1) is used as the keyword parameter
# => [1, {:a=>1}]

a({"a"=>1})
# => [{"a"=>1}, {}]

a({"a"=>1, :a=>1})
# (irb):7: warning: The last argument for `a' (defined at (irb):1) is split into positional and keyword parameters
# => [{"a"=>1}, {:a=>1}]

I did not change the behavior for bare-keywords with Symbol and non-Symbol keys, only for positional hashes. Now that keywords can support non-Symbol keys (in this branch), I do not think it makes sense to restore backwards compatibility with Ruby 2.6 for those. I think that would only make sense if we are going to delay support for non-Symbol keys until Ruby 3.

My Branch:

a(:a=>1)
# =>[1, {:a=>1}]

a("a"=>1)
# => [1, {"a"=>1}]

a("a"=>1, :a=>1)
# => [1, {"a"=>1, :a=>1}]

Ruby 2.6:

a(:a=>1)
# => [1, {:a=>1}]

a("a"=>1)
# => [{"a"=>1}, {}]

a("a"=>1, :a=>1)
# => [{"a"=>1}, {:a=>1}]

Updated by mame (Yusuke Endoh) 3 months ago

Jeremy, thank you for working on this issue.

I believe the expected behavior in Ruby 2.7 is to warn but return the same results as Ruby 2.6 in all three cases, is that correct?

I had intended the incompatibility, as I said in "Migration path: 2.7 semantics" section of note-45:

Basic approach:

  • If a code is valid (no exception raised) in 3.X, Ruby 2.7 should run it in the same way as 3.X
  • If a code is invalid (an exception raised) in 3.X, Ruby 2.7 should run it in the same way as 2.6, but a warning is printed

Akr also objects this approach, so I withdraw this proposal.

However, akr and I think that "2.7 is completely compatible with 2.6 except warnings" approach is not good enough. We need to provide a migration path that allows users to rewrite their code for 3.0 gradually. So, Ruby 2.7 must run (reasonably almost) all programs that are valid as either 2.6 or 3.0. (I expected the above proposal to be good enough, but turned out somewhat too breaking.)

In other words, if a warning is printed, there must be a reasonable change that is not warned (i.e., will work in 3.0) and that causes no behavior change (in, at least, 2.7 semantics).

Consider delegation. Currently we write:

# we call this "old-style delegation"
def foo(*args, &blk)
  bar(*args, &blk)
end

but this is warned when keywords are passed. So we'd like to rewrite it as:

# we call this "new-style delegation"
def foo(*args, **kw, &blk)
  bar(*args, **kw, &blk)
end

However, this rewrite changes the behavior unfortunately in 2.6 semantics because of the problem "2. Explicit Delegation of keywords backfires"

def bar(*args)
  p args
end

def foo0(*args, &blk)
  bar(*args, &blk)
end

def foo1(*args, **kw, &blk)
  bar(*args, **kw, &blk)
end

bar()  #=> []
foo0() #=> []
foo1() #=> [{}] # broken

rb_no_keyword_hash trick works gracefully in this case. The trick allows the new-style delegation in (almost) 2.6 semantics. So the hack is needed, we think.

Updated by mame (Yusuke Endoh) 3 months ago

jeremyevans0 (Jeremy Evans),

You registered this ticket for pre-RubyKaigi [Misc#15459]. Do you have an idea how to discuss the issue?

ko1 (Koichi Sasada) is now creating an agenda, and maybe 30 minutes will be allotted to this issue. The agenda is not decided yet, though.

To make it easy to discuss the issue, I'm creating a slide deck.

https://docs.google.com/presentation/d/16rReiCVzUog3s5vV702LzcIFM2LNcX53AX8m5K8uCZw

I hope that this would be helpful and fair, but could you check the content? If you want to edit it yourself, emaiil me (mame@ruby-lang.org) your google acount.

If you have already prepared something, you can ignore my slide. Anyway, I'm happy if you let me know. Thanks.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

mame (Yusuke Endoh) wrote:

However, akr and I think that "2.7 is completely compatible with 2.6 except warnings" approach is not good enough. We need to provide a migration path that allows users to rewrite their code for 3.0 gradually. So, Ruby 2.7 must run (reasonably almost) all programs that are valid as either 2.6 or 3.0. (I expected the above proposal to be good enough, but turned out somewhat too breaking.)

In other words, if a warning is printed, there must be a reasonable change that is not warned (i.e., will work in 3.0) and that causes no behavior change (in, at least, 2.7 semantics).

I agree we should aim for this. I think it is true with my proposal, but there may be cases I have not considered.

Consider delegation. Currently we write:

# we call this "old-style delegation"
def foo(*args, &blk)
  bar(*args, &blk)
end

but this is warned when keywords are passed. So we'd like to rewrite it as:

# we call this "new-style delegation"
def foo(*args, **kw, &blk)
  bar(*args, **kw, &blk)
end

However, this rewrite changes the behavior unfortunately in 2.6 semantics because of the problem "2. Explicit Delegation of keywords backfires"

def bar(*args)
  p args
end

def foo0(*args, &blk)
  bar(*args, &blk)
end

def foo1(*args, **kw, &blk)
  bar(*args, **kw, &blk)
end

bar()  #=> []
foo0() #=> []
foo1() #=> [{}] # broken

rb_no_keyword_hash trick works gracefully in this case. The trick allows the new-style delegation in (almost) 2.6 semantics. So the hack is needed, we think.

The rb_no_keyword_hash hack is not needed in my branch, as my branch returns [] for bar, foo0, and foo1, since double-splatting empty hashes to a method that does not accept keyword arguments does not pass a positional hash in my branch.

I think using the rb_no_keyword_hash hack will cause problems, because it treats some empty hashes different from other empty hashes. Consider the following case, where you want to limit which keyword arguments are passed when delegating:

ALLOWED_KEYWORDS = [:baz, :quux]
def foo2(*args, **kw, &blk)
  kw = kw.select{|k| ALLOWED_KEYWORDS.include?(k)}
  bar(*args, **kw, &blk)
end

This could be fixed by switching to a mutating method (select!), though.

You registered this ticket for pre-RubyKaigi [Misc#15459]. Do you have an idea how to discuss the issue?

ko1 (Koichi Sasada) is now creating an agenda, and maybe 30 minutes will be allotted to this issue. The agenda is not decided yet, though.

To make it easy to discuss the issue, I'm creating a slide deck.

https://docs.google.com/presentation/d/16rReiCVzUog3s5vV702LzcIFM2LNcX53AX8m5K8uCZw

I hope that this would be helpful and fair, but could you check the content? If you want to edit it yourself, emaiil me (mame@ruby-lang.org) your google acount.

I briefly reviewed your slide deck and I think it does a good job explaining the various aspects of this issue, and I think we should present it during the developer meeting. I will do a more thorough review later today and email you if I have any suggested changes to the slides.

Updated by Eregon (Benoit Daloze) 3 months ago

With Jeremy's proposal, I think there is no need to support non-Symbol keywords.
I think in the case of ActiveRecord or Sequel's #where, the method doesn't need to accept keywords, it can just accept a Hash.
This Hash might contain numbers or strings as keys, and as such doesn't feel like "keywords arguments" to me.
For instance, method definitions do not have a way to specify non-Symbol keywords.

Re current behavior of splitting a Hash passed as keyword argument, mixing Symbols and non-Symbols keys, I would just raise an ArgumentError or TypeError if the Hash contains a non-Symbol key. It's somewhat similar to the "missing keyword" ArgumentError.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

During the developer meeting on Wednesday, Matz mentioned that with my approach, it would be useful to have a way to indicate that a method should be treated as a keyword argument method even if does not accept any keyword arguments. Doing so would make it so you could add keyword arguments to the method later in a backwards compatible manner without a workaround (e.g. safe keyword extension, the advantage of mame's approach). Matz recommended the currently invalid **nil syntax for this feature. I have implemented this feature in my branch at https://github.com/jeremyevans/ruby/tree/keyword-argument-separation. Example:

def a(a=1, **nil, &b)
  [a, b, local_variables]
end

a(a:1)
# ArgumentError (no keywords accepted)

a({a:1})
# => [{:a=>1}, nil, [:a, :b]]

a(**{a:1})
# ArgumentError (no keywords accepted)

a(**{})
# => [1, nil, [:a, :b]]

You can contrast this behavior with the behavior for a method that does not use the **nil syntax:

def b(a=1, &b)
  [a, b, local_variables]
end

b(a:1)
# => [{:a=>1}, nil, [:a, :b]]

b({a:1})
# => [{:a=>1}, nil, [:a, :b]]

b(**{a:1})
# => [{:a=>1}, nil, [:a, :b]]

b(**{})
# => [1, nil, [:a, :b]]

More work should be done if this is accepted. Specifically, we need to decide:

  • What should Method#parameters return for a method that uses **nil?
  • How should ripper handle for the **nil syntax?
  • How should RubyVM::AbstractSyntaxTree handle the **nil syntax?

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

jeremyevans0 (Jeremy Evans) wrote:

More work should be done if this is accepted. Specifically, we need to decide:

  • What should Method#parameters return for a method that uses **nil?
  • How should ripper handle for the **nil syntax?
  • How should RubyVM::AbstractSyntaxTree handle the **nil syntax?

I've updated my branch (https://github.com/jeremyevans/ruby/tree/keyword-argument-separation) to add support for the **nil syntax in Method/Proc#parameters, ripper, and RubyVM::AbstractSyntaxTree

Method/Proc#parameters uses a :nokey entry if the **nil syntax is used:

proc{||}.parameters
# => []

proc{|**a|}.parameters
# => [[:keyrest, :a]]

proc{|**nil|}.parameters
# => [[:nokey]]

Ripper uses :nil for the keyword rest argument if the **nil syntax is used:

Ripper.sexp('def a() end')
# => [:program, [[:def, [:@ident, "a", [1, 4]], [:paren, [:params, nil, nil, nil, nil, nil, nil, nil]], [:bodystmt, [[:void_stmt]], nil, nil, nil]]]]

 Ripper.sexp('def a(**b) end')
# => [:program, [[:def, [:@ident, "a", [1, 4]], [:paren, [:params, nil, nil, nil, nil, nil, [:kwrest_param, [:@ident, "b", [1, 8]]], nil]], [:bodystmt, [[:void_stmt]], nil, nil, nil]]]]

Ripper.sexp('def a(**nil) end')
# => [:program, [[:def, [:@ident, "a", [1, 4]], [:paren, [:params, nil, nil, nil, nil, nil, :nil, nil]], [:bodystmt, [[:void_stmt]], nil, nil, nil]]]]

RubyVM::AbstractSyntaxTree uses false instead of nil for the keyword and keyword rest arguments if the **nil syntax is used:

node = RubyVM::AbstractSyntaxTree.parse("def a() end")
node.children.last.children.last.children[1].children
# => [0, nil, nil, nil, 0, nil, nil, nil, nil, nil]

node = RubyVM::AbstractSyntaxTree.parse("def a(**a) end")
node.children.last.children.last.children[1].children
# => [0, nil, nil, nil, 0, nil, nil, nil, #<RubyVM::AbstractSyntaxTree::Node:DVAR@1:6-1:9>, nil]

node = RubyVM::AbstractSyntaxTree.parse("def a(**nil) end")
node.children.last.children.last.children[1].children
# => [0, nil, nil, nil, 0, nil, nil, false, false, nil]

Hopefully these are all of the introspection methods that we need to modify to support **nil. I'm not sure that these are the best ways of handling each case, but I'm open to better ideas.

#64

Updated by jeremyevans0 (Jeremy Evans) 26 days ago

  • Related to Bug #14415: Empty keyword hashes get assigned to ordinal args. added
#65

Updated by jeremyevans0 (Jeremy Evans) 19 days ago

  • Related to Bug #12022: Inconsistent behavior with splatted named arguments added
#66

Updated by jeremyevans0 (Jeremy Evans) 19 days ago

  • Related to Bug #11860: Double splat does not work on empty hash assigned via variable added
#67

Updated by jeremyevans0 (Jeremy Evans) 19 days ago

  • Related to Bug #10708: In a function call, double splat of an empty hash still calls the function with an argument added

Updated by sawa (Tsuyoshi Sawada) 12 days ago

I would like to ask for clarification.

I understand that this feature removes the rule that complements argument-final brace-less key-value pairs with braces. That is, the rule that interprets:

foo("bar", a: 1, b:2)

as

foo("bar", {a: 1, b: 2})

will be removed.

There is another case where a similar complementation rule exists. That is, complementing the final key-values pairs in an array literal:

[e1, e2, k1: v1, k2: v2]

with braces to make them a hash as:

[e1, e2, {k1: v1, k2: v2}]

What would happen to this rule? Will it be removed as well (in which case the first example above would become ungrammatical)?

#69

Updated by jeremyevans0 (Jeremy Evans) 11 days ago

  • Related to Bug #11068: unable to ommit an optional keyarg if the previous arg is an optional hash added
#70

Updated by jeremyevans0 (Jeremy Evans) 9 days ago

  • Related to Bug #11039: method_missing の *args 引数に symbol をキーにした hash だけを渡すと エラーとなる added
#71

Updated by jeremyevans0 (Jeremy Evans) 9 days ago

  • Related to Bug #10994: Inconsistent behavior when mixing optional argument and keyword splat added

Updated by sawa (Tsuyoshi Sawada) 8 days ago

Can someone answer my question in https://bugs.ruby-lang.org/issues/14183#note-68? Perhaps mame (Yusuke Endoh) knows? If it is not decisive yet, that is fine. I just want to know what the developers have in mind at this point.

Updated by jeremyevans0 (Jeremy Evans) 8 days ago

sawa (Tsuyoshi Sawada) wrote:

Can someone answer my question in https://bugs.ruby-lang.org/issues/14183#note-68? Perhaps mame (Yusuke Endoh) knows? If it is not decisive yet, that is fine. I just want to know what the developers have in mind at this point.

It is a literal and not a method call, so it should continue to work. It does in my branch and I believe mame's branch as well.

Also available in: Atom PDF