Bug #20218: aset/masgn/op_asgn with keyword arguments

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[jeremyevans0 (Jeremy Evans)]

I found that use of keyword arguments in multiple assignment is broken in 3.3 and master:

h = {a: 1}
o = []
def o.[]=(*args, **kw)
  replace([args, kw])
end

# This segfaults as RHS argument is not a hash
o[1, a: 1], _ = [1, 2]


# This passes the RHS argument as keywords to the method, treating keyword splat as positional argument
o[1, **h], _ = [{b: 3}, 2]
o
# => [[1, {:a=>1}], {:b=>3}]

Before 3.3, keyword arguments were treated as positional arguments.

This is similar to #19918, but for keyword arguments instead of block arguments.

@matz (Yukihiro Matsumoto) indicated he wanted to prohibit block arguments in aset/masgn and presumably also op_asgn (making them SyntaxErrors). Can we also prohibit keyword arguments in aset/masgn/op_asgn?

Note that aset treats keyword arguments as regular arguments:

o[1, a: 1] = 2
o
# => [[1, {:a=>1}, 2], {}]

o[1, **h] = {b: 3}
o
# => [[1, {:a=>2}, {:b=>3}], {}]

While op_asgn treats keyword arguments as keywords:

h = {a: 2}
o = []
def o.[](*args, **kw)
  concat([:[], args, kw])
  x = Object.new
  def x.+(v)
    [:x, v]
  end
  x
end
def o.[]=(*args, **kw)
  concat([:[]=, args, kw])
end
o[1, a: 1] += 2
o
# => [:[], [1], {:a=>1}, :[]=, [1, [:x, 2]], {:a=>1}]

o.clear
o[1, **h] += {b: 3}
o
# => [:[], [1], {:a=>2}, :[]=, [1, [:x, {:b=>3}]], {:a=>2}]

[matz (Yukihiro Matsumoto)]

OK, prohibit keyword arguments in aset.

Matz.

[kddnewton (Kevin Newton)]

Does this also include blocks? Sorry I can't remember if that was officially decided or not. Also I'm presuming this would be for Ruby 3.4?

[mame (Yusuke Endoh)]

Does this also include blocks?

Yes. @nobu (Nobuyoshi Nakada) wrote a patch in #19918, so a block will also be prohibited.

Note that keyword arguments and a block are allowed in the normal method call style: obj.[]=(1, 2, kw: 1, &blk).

[nobu (Nobuyoshi Nakada)]

https://github.com/nobu/ruby/tree/ary_set-prohibited-args

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|0d5b16599a4ad606619228623299b931c48b597b.

---

[Bug #20218] Reject keyword arguments in index

[bughit (bug hit)]

In this issue there's no consideration of compatibility or utility. This is a breaking change. The ability to pass kwargs to index methods has been in ruby for a long time, probably from the inception of kwargs and I have code that uses that. Ruby doesn't have a spec so MRI behavior is effectively the spec so you can't say using kwargs in index methods was somehow "wrong". It wasn't "wrong" empirically and it's not "wrong" conceptually. kwargs just allow for more variability in store/lookup operations via index methods, it allows you to control where/how something is stored/looked up.

this is from 2.6

module IndexTest
  @store = {}

  def self.store
    @store
  end

  def self.key(name, namespace: nil)
    name = "#{namespace}:#{name}" if namespace
    name
  end

  def self.[](name, namespace: nil)
    p [name, namespace]
    @store[key(name, namespace: namespace)]
  end

  def self.[]=(name, opts = {}, val)
    p [name, opts, val]
    @store[key(name, namespace: opts[:namespace])] = val
  end


end

IndexTest['foo'] = 1
p IndexTest['foo']
IndexTest['foo', namespace: 'bar'] = 2
p IndexTest['foo', namespace: 'bar']
p IndexTest.store

So kwargs in index methods should be preserved for the sake of compatibility and utility. The only reasonable breaking change here is for []= to have real kwargs, rather than the middle positional kwarg collector hash in the above example.

[bughit (bug hit)]

@matz (Yukihiro Matsumoto) Why is it necessary to introduce a breaking change here by removing useful, long-standing syntax? See example in the previous post.

[kddnewton (Kevin Newton)]

There were long-standing bugs with aset within masgn. Things like:

foo, bar[1, 2, qux: 1, &qaz], baz = *qoz

and in other expressions like:

begin
rescue => bar[1, 2, qux: 1, &qaz]
end

and:

for foo, bar[1, 2, qux: 1, &qaz], baz in qoz do end

I kind of thought these were the only expressions that were going to lose the ability to have keywords/blocks. I think maybe we need some clarification here, because:

foo[1, 2, bar: 1, &baz] = qux

always worked as far as I know.

[bughit (bug hit)]

The release notes, which is what caught my attention, are categorical:

Keyword arguments are no longer allowed in index. [Bug #20218]

[jeremyevans0 (Jeremy Evans)]

In Ruby 3.3, behavior is inconsistent:

a = Class.new do
  def [](*a, **kw, &b)
    p([a, kw, b])
    0
  end
  alias []= []
end.new
b = proc{}

# Regular assignment treats keywords as positional
a[1, 2, bar: 3, &b] = 4
# [[1, 2, {:bar=>3}, 4], {}, #<Proc:0x00000b17febec6e0 (irb):8>]

# Operator assignment respects keywords
a[1, 2, bar: 3, &b] += 4
# [[1, 2], {:bar=>3}, #<Proc:0x00000b17febec6e0 (irb):8>]
# [[1, 2, 4], {:bar=>3}, #<Proc:0x00000b17febec6e0 (irb):8>]

# Mass assignment crashes process
a[1, 2, bar: 3, &b], _ = 4, 5

Due to keyword argument separation, the regular assignment behavior for keywords should be considered incorrect. It could be changed to make it similar to operator assignment, but that would be worse in terms of backwards incompatibility, because it would silently change behavior. With the change to make it invalid syntax, at least the few Ruby users using the syntax know to update their code.

You can still call the [] and []= methods with keywords and a block, using send/public_send, in which case you can specify which arguments are positional and which are keywords.

[kddnewton (Kevin Newton)]

Thanks for the clarification @jeremyevans0 (Jeremy Evans). I think it's also worth noting you should be able to call them normally with a call operator, as in:

a.[]=(1, 2, 4, bar: 3, &b)

[bughit (bug hit)]

These are not arguments for removing a useful, long-standing syntactic feature, but for fixing the various edge cases with it.

Multiple assignment segfaults in 2.6 too. I didn't know about it but it did not prevent me from using the feature, but this "fix" sure will.

It could be changed to make it similar to operator assignment, but that would be worse in terms of backwards incompatibility

As someone who's is actually using this functionality, that's a change that would be acceptable to accommodate. kwargs separation is already there so you have to deal with the fallout in multiple contexts. So no it would not be generally "worse". The feature should remain, I already illustrated its utility.