Bug #20513: the feature of kwargs in index assignment has been removed without due consideration of utility, compatibility, consistency and logic

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

[bughit (bug hit)]

See #20218

The ability to pass kwargs to index methods has been in ruby for a long time, probably from the inception of kwargs, so there's code that makes use of it. Other than the multiple assignment edge-case it's been working fine and is not conceptually unsound. kwargs allow for more variability in store/lookup operations via index methods, letting you 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

A reasonable breaking change would be for []= to have real kwargs, rather than the middle positional kwarg collector hash in the above example.

I am not arguing that breaking changes can't be introduced, but that a removal of a long-standing feature deserves more consideration and deliberation than the following:

I found that use of keyword arguments in multiple assignment is broken
Can we also prohibit keyword arguments ... ?
OK, prohibit keyword arguments

[jeremyevans0 (Jeremy Evans)]

In my opinion, opening a new issue just because you didn't get the response you wanted to your comments in the original issue (#20218) is not an appropriate way to handle things. If you would like @matz (Yukihiro Matsumoto) to reconsider his decision, add issue #20218 as a discussion item for the next developer meeting (#20435).

The support for keyword arguments in aset/op_asgn/masgn was not removed just because multiple assignment was broken. That could have been fixed, as I fixed operator assignment in Ruby 3.3. I fixed operator assignment in Ruby 3.3 because it was segfaulting, without realizing the resulting keyword argument behavior differed from aset and without realizing mass assignment had the same issue.

I agree that a decision to remove syntax should not be made lightly, and I don't think it was. However, fixing the a[1, kw: 2] = 3 syntax to correctly support keyword arguments would result in silent behavior changes, which is much worse than raising a SyntaxError. Maybe you believe that silent behavior changes are better than raising a SyntaxError, but I doubt that is a commonly held belief.

Even if you fixed aset to treat keywords correctly, the behavior is strange, because the rhs would be inserted between the positional arguments and keyword arguments. While technically correct in regards to keyword argument separation, I think inserting the rhs between the positional and keyword arguments of the lhs is a bad idea from a language design perspective.

I see three possibilities:

1. Treat keyword arguments as positional in aset/op_asgn/masgn. This is inconsistent with keyword argument separation, and silently breaks op_asgn backwards compatibility with Ruby 3.3.
2. Treat keyword arguments as keywords in aset/op_asgn/masgn. This results in poor language design, and silently breaks aset backwards compatibility with most previous Ruby versions.
3. Remove the syntax, which avoids the problems with options 1 and 2. Still backwards incompatible, but not silently backwards incompatible.

I can easily see why @matz (Yukihiro Matsumoto) chose option 3 in this case.

[zverok (Victor Shepelev)]

but that a removal of a long-standing feature deserves more consideration and deliberation than the following

I wholeheartedly agree with this. I feel like a lot of “keyword arguments separation leftovers” (and not only) are removed in this manner, hurting the language’s image.

A lot of things that are intuitively following from the common rules of syntax/semantics, and always worked, and even if used rarer than others, brought joy when tried or used for small private DSLs, suddenly just stop working or not follow the entire language dynamics/evolution.

I understand that currently, the main driver for said evolution is maintainers of huge Rails codebases where squeezing out a few percent of performance is more important than the joy of the language, but I never felt comfortable with this direction.

The logic of “#[] is a method, so you can use in its definition/call whatever you can use in other methods” is beautifully simple and consistent. Even if it brings some implementation complexities, this follows the always-present logic of Ruby: complex internally language for writing simple and beautiful code.

But in recent years, arguments of “let’s simplify the implementation, so what if it breaks some of the consistency, no big amount of devs should want this anyway, and you can’t prove otherwise” seem to have grown deeper into the development process, and it is sad to see.

I always felt that the main argument for Ruby’s structure was “if it is semantically consistent and logically clear, it should work.” Nowadays, the frequent argument is “until there is strong proof it is needed in production code (and this proof would meet the harshest scrutiny), we can drop that.”

My two go-to examples:

1. Once, something.map { |foo:, bar: "default"| ... was a beautiful and convenient idiom, making keyword arguments more accepted and demonstrating Ruby’s flexibility. On “keyword args separation,” it was just dropped, and no as clear and convenient replacement exists;
2. When anonymous argument passing was introduced, it was prohibited for blocks (instead of keeping them on par with methods), even though small blocks might benefit enormously from this.

[zverok (Victor Shepelev)]

To add: just a “point-of-view” thing: I imagine teaching Ruby to somebody, and helping them to become proficient in it, and to find how everything is consistent and convenient (I did that a lot). And I imagine the following dialog:

— So, for your custom object to have `[]`, you just `def []`, like it is a common method!
— Oh, nice! I like how there are ground rules and everything is conforming to them!
... 
— Oh, I expermented with it a bit, and for some reason, `colored_hash[:key, color: :red]` doesn’t work?.. 
  What am I doing wrong?.. Is it a bug?..
— You see, young padawan... Many versions ago, there was that thing about Keyword Argument Separation.
  And when it was being done, we forgot to handle the `[]` situation... And then, in a few years, when
  it was handled, in order to not break some code slightly, we just removed the feature.
— Ugh.

I mean, looking at the language with a fresh eye, “something semantically clear and conforming to the existing intuitions doesn’t work because it was prohibited arbitrarily during one historical transition” might be really harmful for the language’s image.

[byroot (Jean Boussier)]

I understand that currently, the main driver for said evolution is maintainers of huge Rails codebases where squeezing out a few percent of performance is more important than the joy of the language

Could you please stop with this sort of veiled references? If you got a problem say it.

It's even more infuriating because the people you are discreetly shitting on have nothing to do with the change in question...

[jeremyevans0 (Jeremy Evans)]

zverok (Victor Shepelev) wrote in #note-3:

— So, for your custom object to have `[]`, you just `def []`, like it is a common method!
— Oh, nice! I like how there are ground rules and everything is conforming to them!
... 
— Oh, I expermented with it a bit, and for some reason, `colored_hash[:key, color: :red]` doesn’t work?.. 
  What am I doing wrong?.. Is it a bug?..
— You see, young padawan... Many versions ago, there was that thing about Keyword Argument Separation.
  And when it was being done, we forgot to handle the `[]` situation... And then, in a few years, when
  it was handled, in order to not break some code slightly, we just removed the feature.
— Ugh.

If you are going to complain about a change using a hypothetical, at least get the details correct, otherwise you appear not to know what you are talking about. The example you gave still works, because it isn't an assignment.

I mean, looking at the language with a fresh eye, “something semantically clear and conforming to the existing intuitions doesn’t work because it was prohibited arbitrarily during one historical transition” might be really harmful for the language’s image.

It isn't intuitive behavior to have keyword argument separation in all other methods and not aset/op_asgn/masgn. It isn't intuitive behavior to insert the rhs of an assignment between the positional and keyword arguments of the lhs.

[zverok (Victor Shepelev)]

I understand that currently, the main driver for said evolution is maintainers of huge Rails codebases where squeezing out a few percent of performance is more important than the joy of the language

Could you please stop with this sort of veiled references? If you got a problem say it.

I am really sorry if my statement was perceived as offensive, and even more so if it was perceived as implying somebody in particular. I see how it can be read this way, and it is 100% the fault of my wording, but it was not what I intended to say, even less so to “shit on” somebody.

I believe that all of the core team members (active currently or in the past) are acting in good faith and are stellar professionals. I believe that deep internal changes in the language achieved in the last versions were extremely good and tough engineering.

The only thing I was trying to say (phrasing it in an absolutely unfortunate way) is that many discussions are currently driven by the fact that Ruby is perceived by many of its users and maintainers as a tool to create and maintain large production (mostly Rails) codebases, and corresponding industry practices.

My main paid Ruby job is also related to the large production Rails codebase and I fully appreciate all changes that make its development and maintenance easier.

At the same time, I care deeply about the language as a tool of thought, and changing it (or refusing to make so) towards less pliability or semantical flexibility makes me sad. The fact that makes me more sad is that such discussions tend to be quite conservative (and not only in the sense of scrutinizing new APIs/syntaxes, but even doubting the utility of existent, if less used, ones).

I don’t think that it is anybody’s “fault” or that I am somehow “smarter” than somebody else; I just wanted to highlight this fact. I didn’t mean any personal or generic offense to anybody.

[zverok (Victor Shepelev)]

@jeremyevans0 (Jeremy Evans) I see your point (as well as your irritation).

I deeply apologize to everybody I’ve offended.

I will make myself scarce.

[bughit (bug hit)]

The example you gave still works, because it isn't an assignment.

Are you saying that [] (p IndexTest['foo', namespace: 'bar']) allows kwargs and only []= (IndexTest['foo', namespace: 'bar'] = 2`) does not?

Because the release notes don't differentiate:

Keyword arguments are no longer allowed in index.

[byroot (Jean Boussier)]

Are you saying that [] (p IndexTest['foo', namespace: 'bar']) allows kwargs and only []= (IndexTest['foo', namespace: 'bar'] = 2`) does not?

Yes. The change is only for []=. Some methods like Dir[] do take keyword arguments, and it's unchanged.

[Eregon (Benoit Daloze)]

@bughit Yes, see my comments on #20218.

Also in such cases, please try ruby-head it's the best way to know.

And I close this as duplicate of #20218, I don't see any value to split the discussion on two issues.

If you care about this, please add #20218 to the dev meeting as Jeremy said in the first reply here.

[Eregon (Benoit Daloze)]

• Is duplicate of Bug #20218: aset/masgn/op_asgn with keyword arguments added

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed

[bughit (bug hit)]

Yes. The change is only for []=. Some methods like Dir[] do take keyword arguments, and it's unchanged.

Someone should correct the release notes.

This asymmetry/inconsistency between [] and []= syntax is arguably even worse. Its arbitrary and incoherent. kwargs being legal in the index reader, strengthens the argument for them being legal in the index writer.

@zverok (Victor Shepelev) I don't know why you're apologizing and retreating. First of all the release notes are wrong so its @jeremyevans0 (Jeremy Evans) who is out of line:

If you are going to complain about a change using a hypothetical, at least get the details correct, otherwise you appear not to know what you are talking about.

How about those involved with this change "get the details correct" in the release notes first. Till then his "irritation" at any sown confusion is unjustified.

Second. Your teaching scenario is still valid with a small adjustment. Try explaining to your hypothetical padawan why IndexTest['foo', namespace: 'bar'] = 2 is illegal while p IndexTest['foo', namespace: 'bar'] is legal. Its absurd.

[jeremyevans0 (Jeremy Evans)]

bughit (bug hit) wrote in #note-13:

Yes. The change is only for []=. Some methods like Dir[] do take keyword arguments, and it's unchanged.

Someone should correct the release notes.

Agreed. I'll take care of that.

This asymmetry/inconsistency between [] and []= syntax is arguably even worse. Its arbitrary and incoherent. kwargs being legal in the index reader, strengthens the argument for them being legal in the index writer.

The change is neither arbitrary nor incoherent. Keyword arguments in aref syntax do not have the problems that keyword arguments have in aset/op_asgn/masgn syntax. Allowing them in aref but prohibiting them in aset/op_asgn/masgn was done for good reasons already explained.

[bughit (bug hit)]

And I close this as duplicate of #20218

This closure reason is invalid, its not a duplicate. #20218 introduced the change in question and is closed. This issue is about reconsidering it and reverting it (instead, fixing multiple assignment issues and giving []= real kwargs) and should not be closed until its actually been reviewed and accepted or rejected.

[ufuk (Ufuk Kayserilioglu)]

For what it's worth: @bughit, as a side-observer of this thread and the previous discussion on #20218, it feels like your engagement in these discussions aren't very healthy. It is quite obvious that you feel strongly about the topic at hand, but despite multiple people telling what the proper process for arbitrating these kinds of decisions is (hint: keep the conversation on the original ticket, and add it to the dev meeting agenda), you seem to want to continue telling everyone what a huge mistake it was in the first place and not follow the guidance. It is pretty obvious what everyone's positions are on the matter, so all that can happen is for @matz (Yukihiro Matsumoto) to make a decision after a discussion at the dev meeting.

I (personally) don't think extending this thread will change anyone's minds nor result in any healthy conversation. As I said before, this is just my personal observation for what it is worth.

[jeremyevans0 (Jeremy Evans)]

jeremyevans0 (Jeremy Evans) wrote in #note-14:

bughit (bug hit) wrote in #note-13:

Yes. The change is only for []=. Some methods like Dir[] do take keyword arguments, and it's unchanged.

Someone should correct the release notes.

Agreed. I'll take care of that.

I submitted a pull request that updates NEWS and also fixes the error messages to be clear this is related to index assignment and not plain index: https://github.com/ruby/ruby/pull/10877

[bughit (bug hit)]

• Subject changed from the feature of kwargs in index methods has been removed without due consideration of utility and compatibility to the feature of kwargs in index assignment has been removed without due consideration of utility, compatibility, consistency and logic