New 2.7/3.0 keyword argument pain point

Hi, this is Matz.

A few Rails core developers (including DHH himself) contacted me that the recent keyword argument changes are too painful. I admit we underestimate the pain of migration of the keyword arguments. After serious consideration, I decided to change/postpone the migration schedule. We are awfully sorry but Ruby3.0 will not have the real keyword arguments.

But there are too many possible options. To make a proper decision, we need to hear the pain points of the keyword migration from the (Rails) community and know how much pain they are. Your input will be valuable.

  • Ruby2.7
    • Ruby2.7 currently warns you about keyword migration. We admit they are noisy.
    • Probably soon-to-be-released 2.7.2 will remove/reduce the warning regarding keyword arguments.
    • We have to decide on the ruby2_keyword method for 2.7.2.
  • Ruby3.0
    • At master we have already separated keyword arguments from normal (positional) arguments. Probably it would cause too much pain to users.
    • Postponing (or canceling) migration is OK for us.
    • But we have to decide how far we go for Ruby3.0.
    • Keeping 2.7 (kinda half-baked) solution for Ruby3 is one option.
    • Maybe we can create some other (smarter) compromise.

So, tell us about your concerns/opinions regarding keyword arguments, or your pain experience migrating to Ruby 2.7.

Matz.

45 Likes

Hi Matz,

Thank you for raising that point and for your work on Ruby!

EDIT: @mame has provided a lot of valuable input in my specific case, which could help other library implementors too. Please check out this comment. Thanks a ton for this help!!!

Just some feedback on my current pain point. In the specific case of DSL (like my gem Kiba ETL and its sister gem kiba-common), capturing args to forward them to classes later, at this point I have found a bit painful to be able to capture kwargs indeed. To that day, Ruby 2.7 is only supported in an experimental branch for this project and I must do more work with it:

Maybe I did something wrong, and also this was done in limited time & I could improve things, but to support a wide range of Rubies, I so far had to rely on RUBY_VERSION, and also when instantiating ETL components (forwarding the kwargs and regular args), I wrote such code:

if RUBY_VERSION >= '2.7'
  klass.new(*args, **(kwargs || {}))
else
  # kwargs should be nil
  klass.new(*args)
end

I will work more on this and I believe it can probably be improved without changes in Ruby, but at least this provides a bit of concrete feedback from a DSL library implementor.

I do not have yet a well-formed opinion on what can be done at the level of Ruby! I am perfectly happy with those hacks, if they are good enough for users to use my gem safely and Ruby can keep making progress.

5 Likes

(Can we make issue on bugs.ruby-lang.org?)

Matz, I was looking forward to finally having separation of keyword arguments in Ruby 3.

I have many situations where hash object is incorrectly promoted to options.

e.g. Async::Notification has trouble when signalling with a plain hash object · Issue #67 · socketry/async · GitHub is just one example.

I respect your position to minimise pain, but the reality is now we are in a situation where for several years people have been planning for the change to separate keyword arguments so either we move backwards and penalise everyone who has worked to support this change, or we move forward and penalise those who have not migrated legacy/old code.

If this really is something we cannot move forward on, how about a per-file flag like for frozen string literals?

# keyword_arguments: true

Feature flags are messy, and fracture the language, but at least we can continue moving forward and legacy code continues to work.

One more data point that might be helpful: I maintain a lot of gems and migrated them all for Ruby 2.7 to remove all warnings. Not only did I find several bugs (incorrect promotion of hash to keywords), the general code is clearer to understand (no more implicit keywords). I found the warnings incredibly helpful and useful, and the overall change made the code better.

Let me know if there is any other way I can help.

20 Likes

Hi there, speaking from the perspective of RSpec, the main pain point for our library is that we transparently pass around arguments given by users, to user code, so we are not directly aware (in a lot of cases) wether keyword arguments are being used or not, we can refactor our code to mitigate this, and we currently plan to for the next major version, but the current keyword argument splat is not parsable on older Rubies (which we must support until our next major version due to our interpretation of semantic versioning).

The current plan, for real keyword arguments in Ruby 3, realistically means we will need to have that new major version ready before the release, and only support Ruby 3 in that version, but for now we must implement arcane workarounds to detect and call keyword arguments separately to remove this warning.

A pragma flag to indicate old / new keyword argument behaviour, would work for us as a workaround, so would having a more permissive ruby2_keywords behaviour where we simply mark methods as treating arguments as a splat and passing it on. Similarly, a new api to allow us to take a arg splat and produce args / keywords would allow us to have back compatibility with much less pain.

Overall I’m a fan of having real keywords, its just a painful transition considering the long history of soft keywords.

4 Likes

Thank you all for posting your experience/feelings. We are here to listen to the community.

@ioquatix We are open but the discussion itself should be done on bugs.ruby-lang.org. We will soon open a new issue regarding this.

Matz.

3 Likes

I think we should be more gradual with keyword argument migration, and as a first important step keep *args-delegation working for 2.7, 3.0, etc. And only warn later about it with precise warnings telling what to do (e.g., use *args,**kwargs-delegation which needs Ruby 3.0+), when it’s reasonable for most libraries to drop 2.7 support (~ Ruby 3.4). That way we don’t need ruby2_keywords, and libraries don’t need to change delegation code twice.

Now, delegation code is probably not very frequent, but the current warnings are particularly unhelpful about it, and the only workaround for 2.7.0 & 2.7.1 is ruby2_keywords which I feel is a hack to write in library code.

I think other warnings related to keyword arguments are much easier to address by having precise warnings for them, and often the fix is obvious.
I’m curious to hear other opinions.

@Thibaut_Barrere shows what intuitively should work, either *args or *args, **kwargs delegation, But both don’t work in 2.7 (only ruby2_keywords works in 2.7.0 & 2.7.1), and I think that should be fixed.

Keeping *args delegation working is simple, see Feature #16463: Fixing *args-delegation in Ruby 2.7: ruby2_keywords semantics by default in 2.7.1 - Ruby master - Ruby Issue Tracking System. It passes Rails’s CI and is even more compatible with Ruby 2.6.

6 Likes

We’re currently fixing all the 2.7 warnings in Shopify’s monolith, and there’s indeed a lot of them.

Overall my opinion is that new keywords argument are definitely better than the old ones.

However I’ll be echoing out what the other said, the main pain point is blind delegation. In some cases the new ... helps (actually we can’t use it just now because Sorbet can’t parse it yet), but it has many annoying limitations:

  • Can’t use it if you delegate with public_send/send.
  • Can’t use it if you want to extract one of the arguments (typical for method_missing)
  • Can’t use it if you want to add one argument.
  • Can’t use it in block parameters (typical for define_method)

I think removing the current limitations to ... would help a lot.

However that’s only true for your own application code. As soon as you are writing code in a gem that is expected to support older Rubies, it won’t help you.

I suppose that’s what ruby2_keywords is for, but it’s tedious to use. A magic comment like frozen_string_literal would have been preferable, but now that 2.7.0 and 2.7.1 are released, it will be complicate to support them.

6 Likes

Hi,

I’m going to speak from the point of view of a developer who is in charge of moving our rails monolith to ruby 2.7.

What I missed in the first place was a real strategy. Over 50 000 warnings generated by our CI, thousands from gems. I didn’t know where to start. So I waited, to see if a strategy emerges, or if the gem maintainers can quickly fix their warnings.

Then thanks to the twitter thread of Eileen Uchitelle and the contribution of Rafael França and Kevin Deisz (here, and here), I found something that could work in our case:

  1. Fix the errors related to this change Double splat with an empty hash ( **{} ) passes no arguments – about ten tests out of 15,000. Instead of a ActiveRecord::Relation we had a ActiveRecord::QueryMethods::WhereChain.

  2. Ignore deprecation warnings: warning.rb · GitHub. Thanks to this, I avoid flooding our log in production and in development.

  3. Use DeprecationToolkit from Shopify – Thanks ! – to list existing warnings in our codebase and avoid adding new ones. Thanks to this configuration, I ask him to take into account the warnings coming only from our codebase and not from the gems: deprecation_toolkit.rb · GitHub

  4. Fix them all.

With this approach, we will be able to merge ruby 2.7 quickly, then in a second step correct the warnings from our codebase in the coming weeks. This makes the transition much less painful on our side.

In conclusion, from my opinion:

  • New keywords argument are better than the old ones
  • What I needed was not more time, but a clear strategy to make the transition right

I hope that helps. :slight_smile:

20 Likes

@matz, I think putting this in for Ruby 3 is a great idea, and I’ll be sad to see this go. Is there a reason why Ruby 3 needs to be shipped this Christmas? Maybe there could be a Ruby 2.8 this year where some more of these kinks get worked out, and then the feature can land in Ruby 3 next year.

10 Likes

@Florent_Beaurain thanks for mentioning deprecation_toolkit, I should have also talked about how we re dealing with warnings.

We didn’t keep a close tab on how much time we actually did spend on fixing warnings, but a wet finger estimate would be 3 devs on and of over the course of two weeks, so maybe something like 1 week for 1 dev full time. That’s for a repo on which hundreds of people work every day, so in the end it’s a really small fraction of our usual development effort.

What I’m trying to say is that seeing so much warnings when you upgrade is really overwhelming, but in the end it’s not that much effort to upgrade as long as you silence the ones coming from your dependencies. It’s really nothing compared to the work we had to put in most Rails upgrade over the years.

So I wholeheartedly agree with your conclusion. Ruby is only missing two things:

  • An easy way to forward parameters that works on all currently supported versions.
  • Some communication around the tools available as well as some kind of migration guide.
6 Likes

I agree with you 100%. I didn’t over-calculate. But I can come up with relatively similar numbers on a 500k LOC codebase and 80+ devs.

Very well summed up. That’s exactly what I think. I’m going to write a blog article on how we’re handling this thing at Doctolib. Hope this will help.

6 Likes

I was personally prepared to deal with the pain for the benefit, I thought it was worth it, and was looking forward to rationalized kw args. But avoiding backwards incompat pain is super important, so I respect I may have been wrong with regard to the community as a whole!

This matches my experience – this is the biggest problem I see or have had with current not entirely rational kw args.

There are other use cases for this general thing, which I’d describe as: Capturing arguments “arbitrarily” to forward them on later. Without being sure exactly what args the method in question defines. Keyword arguments? New-style, or old style as a hash? Other optional arguments? Etc.

Another use case is overriding a method, to look at/modify certain known args, while capturing and forwarding “all other” args, while being “future compatible” with it’s changing exactly what methods it has or how it defines them (switching from last arg options = {} to **options etc).

def some_method(*position_args, **kw_args)
   position_args[1] ||= something_else
   kw_args[:other] = another_thing
   other_method_with_same_args_unchanged(*position_args, **kwargs)
end

Right now, it is very hard (impossible?) to do that in a reliable way, that won’t change how the ultimate receiving method receives them. The example above, I think, won’t reliably do it while working consistently in recent ruby versions.

If there were a way to do this (even using special-purpose syntax or methods?), I think the “irrationality” of keyword arguments otherwise would seldom get in the way.

Hi. Not rails-related, but I’ve stumbled across this thread, and since I’m in the middle of quite a bit of pain related to these changes, I thought I might want to share.

I thought I was being a good Ruby citizen, and slowly migrating my code to using keyword arguments, double splat and all that. I (fortunately for me) am not keeping very long backward compatibility across ruby versions, so I can do this type of things.

I basically thought I was all set. A few days of fixing the (very welcome) warnings and that’s it. Boy was I wrong.

I ended up hitting the decision of allowing arbitrary keyword arguments in 2.7, at the same time than the other changes - my (rightfully rejected) bug report is here.

To be honest, I felt very cheated. I thought I was doing the right thing with a slow upgrade, and keyword arguments have rejected anything that’s not a Symbol since day 1. So, I (stupidly) assumed that it would stay that way at least until the “real” keyword args arrive (a.k.a. after 3.0). Now, were something like that happen again, I’d basically go the route of wait and change stuff only when strictly required.

It turns out that the “user-facing” API of my library (a robotic framework) is a DSL, and some methods are given both a mapping of strings to strings and keyword arguments. But then, I thought I’ll be fine (because there’s no way they can break that, strings can’t go in kw args). However, what was working before:

def provides(h = {}, **kw); end
provides "some" => "strings", as: "name"

now resolves everything into the keyword splat. I’m scared of even changing things now, since adding a keyword splat will break code. If I would change the following signature:

def m(mapping = {}, as: nil); end

into one that has a keyword splat - for instance to forward arbitrary arguments to another method -

def m(mapping = {}, as: nil, **kw); end

then what was going into mapping now goes into kw

This change IMO makes no sense from a backward compatibility perspective. It is a change that could definitely have waited for 3.1 after the real keyword arguments are here and here to stay, where everyone properly separated hashes from keywords. I’ve been through the 1.9 transition, and I feel like that.

Don’t get me wrong, I like the idea of using 3.0 as a way to finally fix the kw args mess, and 2.7 as a stepping stone to there. And I am biting the bullet and trying to get this to work.

But this is not that. This is a backward-compatibility-breaking change that was there not to clean up in the way to 3.0, but to add new functionality that was not there before.

As for the warnings, we would ideally need a way to disable them per-package. Users of rails or other libraries/frameworks shouldn’t have to care that the framework needs updating.

Finally, last but not least, Ubuntu 20.04 shipped with Ruby 2.7. This is going to be a lot of pain for Ruby developers on Ubuntu, and I believe will fuel the dislike of Ruby for that platform’s users.

1 Like

At Discourse we still have not upgraded to Ruby 2.7 in production. I think it is quite a shame, and the biggest driver for not upgrading was that the benefits of Ruby 2.7 were small compared to dealing with various loud deprecations. We did not notice that Discourse runs any faster on 2.7 maybe it is all the deprecation accounting, I don’t know.

Recently, we upgraded to latest Rails to reduce warnings, but we are left with warnings from gems and warnings from Discourse core.

Examples of Discourse warnings are:

/home/sam/Source/discourse/lib/freedom_patches/inflector_backport.rb:30: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activesupport-6.0.3/lib/active_support/inflector/methods.rb:128: warning: The called method `humanize_without_cache' is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:130: warning: The called method is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:144: warning: The called method is defined here
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil

Examples of gem warnings are:

/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5.rb:22: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5/document.rb:4: warning: The called method `parse' is defined here
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
..../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
........./home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil

Dealing with our warnings is feasible albeit annoying. More on this soon. Dealing with gem warnings can be a nightmare.

For example to deal with the sassc we are going to have to start with upgrading sassc we saw recent versions of this gem segfault so we have held back for a bit, getting us to latest is probably us spending weeks of debugging. Further more, gem authors take time to approve PRs and push new versions out there (understandably) so this is going to take a while.

Just silencing deprecations feels wrong to us as a path forward. If Ruby, by default, is being loud I feel we just have to deal with it.

Dealing with the Discourse side of things now comes with some fundamental problems. Firstly I am uneasy with the whole ruby2_keywords thing, it feels like I am adding technical debt to my code when I hack it in. This is all code that I am going to have to remove / re-implement.

Half of our “we did it to ourselves” warnings are coming from our Inflector memoizer. https://github.com/discourse/discourse/blob/4601833e4ed98aaaaf90c0ddab2bfebfae42db7c/lib/freedom_patches/inflector_backport.rb#L8-L40. The Rails inflector proved to be a bit of a hot spot for us (and Shopify also confirm that some methods are for them) so we introduced an LRU cache in front to memoize methods.

In Ruby 2.6, a naive implementation is very easy to reason about, it goes something like this:

def memoize_26(method_name)
  cache = {}

  uncached = "#{method_name}_without_cache"
  alias_method uncached, method_name

  define_method(method_name) do |*arguments|
    found = true
    data = cache.fetch(arguments) { found = false }
    unless found
      cache[arguments] = data = public_send(uncached, *arguments)
    end
    data
  end
end

It can be done a bit faster, but the code is easy to reason about and it has worked for years.

Now Ruby 2.7 comes along and I need to call ruby2_keywords on this dynamic method (and check if we can do this) and carry tech debt going forward, or implement it differently.

... is not going to work for me, cause I need the values passed in so I can do my cache lookup.

Simply adding **kwargs unconditionally to this memoizer means that now my memoizer just got slower for methods with no keyword args, I need to construct a new array for the cache lookup with both kwargs and args

So if I want something that is usually as fast as it used to be I am stuck with this:

def memoize_27_v2(method_name)
  uncached = "#{method_name}_without_cache"
  alias_method uncached, method_name

  cache = "MEMOIZE_#{method_name}"

  params = instance_method(method_name).parameters
  has_kwargs = params.any? {|t, name| "#{t}".start_with? "key"}
  has_args = params.any? {|t, name| !"#{t}".start_with? "key"}

  args = []

  args << "args" if has_args
  args << "kwargs" if has_kwargs

  args_text = args.map do |n|
    n == "args" ? "*args" : "**kwargs"
  end.join(",")

  class_eval <<~RUBY
    #{cache} = {}
    def #{method_name}(#{args_text})
      found = true
      all_args = #{args.length === 2 ? "[args, kwargs]" : args[0]}
      data = #{cache}.fetch(all_args) { found = false }
      unless found
        #{cache}[all_args] = data = public_send(:#{uncached} #{args.empty? ? "" : ", #{args_text}"})
      end

      data
    end
  RUBY

end

Even with all this wizardry I can not recover the performance of the old *args capture cause I am stuck constructing a new array for the cache lookup if args and kwargs are there at the same time.

My old method which was easier to reason about is 1.3 times faster cause I am not stuck generating an array for the lookup.

Full benchmark here:

What do I think should be done

Huge +1 on

  1. Allow for some new syntax that allows a global capture of both kwargs and args (...a is not going to work cause it will clash):
def foo(bar)
end

def x(***a)
   
   p a

   foo ***a
end

# maybe
def x(...)
   a = ...
   p a
   foo ...
end 
  1. Remove all kwarg deprecation accounting and warnings from Ruby 2.7

  2. The train has probably left the station but ruby2_keywords… I wish it could be removed, if we had support for ***a in 3 we could just eval ourselves out of the syntax clash in a switch for the places where we need cross Ruby support. I don’t know.

  3. Consider, why did we allow so many deprecations to go silent for years and suddenly we are very loud?

irb(main):001:0> URI.escape("a")
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.6.5"

# vs

irb(main):001:0> URI.escape("a")
(irb):1: warning: URI.escape is obsolete
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.7.1"

This for example has been deprecated for almost 15 years already, but just now got loud.

I extremely appreciate everyone in the MRI core team and I understand the enromity of this conundrum, but sadly the loud deprecation notices are holding people up from upgrading Ruby which is achieving the opposite of one of the core missions of the MRI team which is to keep everyone upgraded.

Discourse / Basecamp / Shopify / GitHub not running 2.7 yet at scale means that tons of real world performance testing is not happening and all sorts of bugs will take a lot longer to find.

@eileencodes has been working on upgrading GitHub recently, perhaps she has more insight.

I know @rafaelfranca has also been very vocal about the delegation change, perhaps he has some stuff to add here.

6 Likes

Actually we’re running it since end of March. Sorry if my message was confusing.

I might just be misunderstanding you, but I don’t think anyone is advocating about just silencing deprecations. What we’re advising people to do it to first upgrade to 2.7 with warnings silenced. And then use the warning and / or deprecation_toolkit gem to fix them in smaller chunks rather than in one big commit.

There is no shame in silencing deprecation as long as you have the process in place to grind them down over time.

In my mind there is a big problem that need to be addressed for gem authors, because it’s way too hard to write code that works without deprecations on both 2.6 and 2.7.

But for end user applications, IMHO the issue is blown out of proportion. Some Rails upgrades took us over a year of developer time (e.g. protected attributes removal), cleaning our codebase to be compatible with the real keyword args took a week, maybe two max.

4 Likes

The key change I am struggling to get my head around is the direction change the core ruby team took.

For the last 8 versions of ruby if you ran it with no special flags stuff was mostly silent. If you wanted to clean house you would turn on warnings. 2.7 introduced a series of loud deprecation notices, so to me the urgency of dealing with everything just “feels” higher.

Many JS frameworks warm loudly on almost every new release regarding upcoming deprecations, and I have grown accustomed to it.

It is just this transitional period for ruby where the ground rules have changed that I am finding tricky to deal with.

2 Likes

@samsaffron that’s fair. I think we can agree that such a verbose deprecation warning being immediately introduced at the default $VERBOSE level is unprecedented and might feel rushed. It probably all come down to wanting to go from 2.7 to 3.0 and wanting to have real keyword arguments in 3.0.

I’d also agree that there’s lessons to be learned from this.

But now that it’s a thing, might as well keep a cool head and think rationally about it. Yes it is a very noisy warning, but no it’s not that big a deal for application maintainers (definitely is for library maintainers).

3 Likes

I think deprecation warnings have almost always been shown by default in Ruby, otherwise people might miss it, and then it would just break when, e.g., the method is removed. That said, I think keyword arguments warnings are far more numerous than any deprecation warning before.

I opened this ticked on the ruby bug tracker to discuss the performance issue I mentioned in my post:

Completely agree we got to keep a cool head here. We are certainly going to get through all of this this. I love how caring Matz and team are about these niggles.

For context Discourse core which is a huge code base only has 2 places that need

if Module.respond_to?(:ruby2_keywords, true)
  ruby2_keywords(m)
end

Other deprecations we have already addressed in our custom rubocop rules here: rubocop-discourse/lib/rubocop/cop/discourse at main · discourse/rubocop-discourse · GitHub

Specifically: rubocop-discourse/no_uri_escape_encode.rb at main · discourse/rubocop-discourse · GitHub which was silent in 2.6 but became noisy in 2.7.

The bigger challenge we have is around upstreaming fixes to gems, but we will certainly also get through that.

2 Likes

For Ruby3, the real keyword argument is not the primary benefit (but JIT, concurrency, and static type analysis are). It’s kind of a side dish along with type analysis because the traditional keyword argument is harder to analyze.

Matz.

11 Likes