Feature #16289: Reduce duplicated warnings for the change of Ruby 3 keyword arguments

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: -

[mame (Yusuke Endoh)]

Problem

Currently, the interpreter emits 200 lines of warnings against the following program.

def foo(**opt); end
100.times { foo({kw:1}) }

$ ./miniruby -e 'def foo(**opt); end; 100.times { foo({kw:1}) }'
-e:1: warning: The last argument is used as the keyword parameter
-e:1: warning: for `foo' defined here
-e:1: warning: The last argument is used as the keyword parameter
-e:1: warning: for `foo' defined here
-e:1: warning: The last argument is used as the keyword parameter
-e:1: warning: for `foo' defined here
...

In theory, the warnings are not harmful because they don't stop or interfere the execution. But in practice, I'm afraid if they are annoying because they flush all console logs away.
I think that the warning is not needed if the call is already warned.

Proposal

How about limiting the count of warnings to at most once for each pair of caller and callee?

I've created a pull request. It records all pairs of caller position and callee iseq when emitting a warning, and suppress the warning if the same pair of caller and callee is already warned.

What do you think?

[marcandre (Marc-Andre Lafortune)]

Definite +1 for me. I thought there was already an issue about this but looks I was mistaken.

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

It leads a memory leak.
And different warnings won't be suppressed too?

[mame (Yusuke Endoh)]

It leads a memory leak.

Theoretically, yes. Even if iseqs of caller or callee are free'd, the corresponding records are not free'd. But honestly I don't think it is worth elaborating the feature for some reasons:

(1) in practice, the case would be rare (to cause the leak, we need to repeatedly create iseq by using "eval" and trigger a warning in the iseq);
(2) the leak does not occur if a warning is not emitted (if any, it should be fixed); and
(3) this hack is only for Ruby 2.7.

Before implementing the patch, I discussed with ko1. At first I was going to avoid the leak by adding some fields to iseq, but he objected the approach and recommended the above design. Now I agree with him.

And different warnings won't be suppressed too?

It was not intended, but it would be also very rare to trigger different keyword warnings in one call, I guess.

[mame (Yusuke Endoh)]

What I worry about is Jeremy's comment in the PR:

In terms of idea, whether we want this feature depends on how annoying we want the warnings to be. If they warn every call, they are very annoying and it will push people to fixing the issue. If they only warn once, they are not that annoying, and people may be less inclined to fix the issue.

It is really convincing. That being said, I'm afraid if it is too annoying.

I'd like to hear opinions and discuss it at the next meeting, so I've created this ticket.

[Eregon (Benoit Daloze)]

What's the performance with this patch for a call site that would warn?

$ chruby ruby-2.7.0-preview2
$ ruby -rbenchmark/ips -e 'def m(**kw); end; h = {a: 1}; Benchmark.ips { |x| x.report { m(h) } }' 
... many warnings ...
                         88.603k (±15.8%) i/s -    434.163k in   5.025597s


$ ruby -rbenchmark/ips -e 'def m(**kw); end; h = {a: 1}; Benchmark.ips { |x| x.report { m(h) } }' 2>/dev/null 
Warming up --------------------------------------
                        46.040k i/100ms
Calculating -------------------------------------
                        504.460k (± 7.0%) i/s -      2.532M in   5.050515s

$ chruby this-branch
$ ruby -rbenchmark/ips -e 'def m(**kw); end; h = {a: 1}; Benchmark.ips { |x| x.report { m(h) } }' 
Warming up --------------------------------------
                    -e:1: warning: The last argument is used as the keyword parameter
-e:1: warning: for `m' defined here
   372.010k i/100ms
Calculating -------------------------------------
                          8.166M (± 2.6%) i/s -     40.921M in   5.015298s

$ chruby 2.6.2
$ ruby -rbenchmark/ips -e 'def m(**kw); end; h = {a: 1}; Benchmark.ips { |x| x.report { m(h) } }' 
Warming up --------------------------------------
                       322.637k i/100ms
Calculating -------------------------------------
                          5.925M (± 8.8%) i/s -     29.683M in   5.055851s

[marcandre (Marc-Andre Lafortune)]

If they only warn once, they are not that annoying, and people may be less inclined to fix the issue.

It's a warning about an upcoming incompatibility, once should be serious enough!

Maybe it should be reworded to emphasize seriousness, using the word "Deprecated" and "This will not be supported in Ruby 3.x":

# Change:
warning: The last argument is used as the keyword parameter
warning: for `foo' defined here

# to something like this:
Deprecated: The last argument is used as the keyword parameter
Deprecated: for `foo' defined here. This will not be supported in Ruby 3.x

[Dan0042 (Daniel DeLorme)]

If they only warn once, they are not that annoying, and people may be less inclined to fix the issue.

It's a warning about an upcoming incompatibility, once should be serious enough!

I think the issue is more that in a long-lived process you'll get only a few warnings in a large log, and it may be easy to miss unless you specifically grep for the warnings.

[shevegen (Robert A. Heiler)]

Makes sense to me what mame wrote, so +1; marcandre's suggestion for a deprecation-specific
notice makes sense as well and may be useful past ruby 3.0 (if ruby 4.x may have more
incompatibilites than ruby 3.0; then there could be more distinct "Deprecated" or
"Deprecation" messages).

Daniel wrote:

I think the issue is more that in a long-lived process you'll get only a few warnings
in a large log, and it may be easy to miss unless you specifically grep for the warnings.

While your comment is perfectly reasonable and makes sense as well, I think this taps into
the older discussion about customizing warnings issued by ruby, so that people can adjust
ruby to different use cases (a bit like you can customize rubocop). But this may be a
discussion past ruby 3.0 perhaps. IMO what mame wrote makes sense too since it may not be
too overly useful to have your full screen overflow with the same warning-message spam
that you already know is a problem. (Of course if there was some way to customize it in
a simple manner, ruby users could then adjust this to their general use case - but I
don't want to dilute mame's suggestion here. Picking the "right" default behaviour
covering the most likely or "best" use case of course makes a lot of sense to me and
I think most ruby users who run into this situation are more likely to want to read cozy
and helpful messages on the commandline, and using logs only secondary. :) )

marcandr's suggestion has the advantage that the leading word "deprecated" can be
colourized quite easily, e. g. people could "style" it to red on their computer;
KDE Konsole supports R,G,B style too, I use that a lot myself for different colours
(HTML colours such as "tomato" or "orangered", but I am going off-topic here a
little bit; it is also not available to everyone, of course). But in general I think
mame's description makes a lot of sense.

Note that while I understand jeremy's comment about annoying messages "helping"
people change their code, not everyone thinks that spam is that helpful. See also
past discussions about reversing the output of the errors so that some people
have it easier to find the problem instantly, on the last part of the message,
whereas other ruby users prefer the old style. In the long run I think it would
be best to have some larger proposal for people being able to customize ruby
warnings (and perhaps error output to, to some extent), but it should be simple
to use too. But again, sorry for diluting here, +1 to what mame wrote. I don't
think "nagging" should be of higher priority than the goal of making it simpler
to read and understand what is going on - that is also in the spirit of the
did-you-mean gem, if you ask me.

[Eregon (Benoit Daloze)]

I updated the benchmark results above and measured on this branch.
It's ~100x faster in that micro benchmark.

If we don't want pretty bad performance when migrating to 2.7 without fixing kwargs warnings (which probably takes a long time for application with many dependencies), then I think we need this.

[sam.saffron (Sam Saffron)]

Just to provide some context on the extent of the issue.

Running the spec suite for Discourse results in 2,698,774 rows being printed to STDERR.

sort log | uniq -c | sorg -bgr

https://gist.github.com/SamSaffron/7c2932e4a6af2a046d2c1c60efca6aa2

465944 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/attribute_mutation_tracker.rb:45: warning: for `changed?' defined here
 272627 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/relation.rb:27: warning: for `initialize' defined here
 272550 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/relation/delegation.rb:115: warning: The last argument is used as the keyword parameter
 258120 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:274: warning: for `transaction' defined here
 258089 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/transactions.rb:212: warning: The last argument is used as the keyword parameter
 211662 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/attribute_methods/dirty.rb:102: warning: The last argument is used as the keyword parameter
 195764 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/timestamp.rb:127: warning: for `create_or_update' defined here
 141394 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/dirty.rb:170: warning: The last argument is used as the keyword parameter
 139375 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/persistence.rb:503: warning: The last argument is used as the keyword parameter
 112855 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/attribute_methods/dirty.rb:52: warning: The last argument is used as the keyword parameter
  82950 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/abstract/transaction.rb:145: warning: The last argument is used as the keyword parameter
  82949 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/abstract/transaction.rb:78: warning: for `initialize' defined here
  59237 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/postgresql_adapter.rb:620: warning: given argument is nil; this will raise a TypeError in the next release
  56283 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/persistence.rb:470: warning: The last argument is used as the keyword parameter
  12471 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/routing/mapper.rb:344: warning: given argument is nil; this will raise a TypeError in the next release
   9908 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/cache.rb:738: warning: for `initialize' defined here
   9903 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/cache.rb:463: warning: The last argument is used as the keyword parameter
   5969 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/test-prof-0.9.0/lib/test_prof/recipes/rspec/let_it_be.rb:53: warning: for `let_it_be' defined here
   5959 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/test-prof-0.9.0/lib/test_prof/recipes/rspec/let_it_be.rb:49: warning: The last argument is used as the keyword parameter
   3062 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/messages/metadata.rb:17: warning: for `wrap' defined here
   3062 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/message_encryptor.rb:150: warning: for `encrypt_and_sign' defined here
   3061 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/message_encryptor.rb:175: warning: The last argument is used as the keyword parameter
   3059 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/middleware/cookies.rb:635: warning: The last argument is used as the keyword parameter
   2608 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_controller/metal/request_forgery_protection.rb:285: warning: given argument is nil; this will raise a TypeError in the next release
   2499 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/testing/integration.rb:357: warning: The last argument is used as the keyword parameter
   2455 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/type/value.rb:8: warning: for `initialize' defined here
   1895 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/postgresql/oid/specialized_string.rb:12: warning: The last argument is used as the keyword parameter
   1678 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/railties/collection_cache_association_loading.rb:7: warning: The last argument is used as the keyword parameter
   1678 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/railties/collection_cache_association_loading.rb:12: warning: for `relation_from_options' defined here
   1617 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/aws-sdk-core-3.48.6/lib/seahorse/client/http/response.rb:133: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
   1615 /home/sam/Source/discourse/app/models/concerns/has_url.rb:25: warning: URI.unescape is obsolete
    960 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/testing/integration.rb:23: warning: for `post' defined here
    951 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/aws-sdk-core-3.48.6/lib/seahorse/client/configuration.rb:107: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
    725 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/testing/integration.rb:35: warning: for `put' defined here
    708 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/testing/integration.rb:17: warning: for `get' defined here
    703 /home/sam/Source/discourse/lib/url_helper.rb:46: warning: URI.escape is obsolete
    569 /home/sam/Source/discourse/lib/freedom_patches/inflector_backport.rb:30: warning: The last argument is used as the keyword parameter
    554 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/type/integer.rb:13: warning: The last argument is used as the keyword parameter
    538 /home/sam/Source/discourse/lib/mini_sql_multisite_connection.rb:30: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
    524 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/inflector/methods.rb:128: warning: for `humanize_without_cache' defined here
    520 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/aws-sdk-core-3.48.6/lib/seahorse/client/plugin.rb:61: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
    415 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/unbound_template.rb:24: warning: The last argument is used as the keyword parameter
    415 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/template.rb:130: warning: for `initialize' defined here
    377 /home/sam/Source/discourse/app/mailers/user_notifications.rb:639: warning: URI.escape is obsolete
    260 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/helpers/translation_helper.rb:120: warning: The last argument is used as the keyword parameter
    259 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/i18n-1.7.0/lib/i18n.rb:279: warning: for `localize' defined here
    250 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/associations.rb:1860: warning: The last argument is used as the keyword parameter
    250 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/associations.rb:1370: warning: for `has_many' defined here
    206 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/helpers/tag_helper.rb:44: warning: for `tag_string' defined here
    206 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/helpers/tag_helper.rb:111: warning: The last argument is used as the keyword parameter
    204 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/lookup_context.rb:140: warning: for `template_exists?' defined here
    202 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on Integer; it always returns nil
    190 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:90: warning: given argument is nil; this will raise a TypeError in the next release
    180 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/faraday-0.15.4/lib/faraday/options.rb:166: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
    160 /home/sam/Source/discourse/app/models/post.rb:943: warning: URI.unescape is obsolete
    144 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/routing/mapper.rb:353: warning: given argument is nil; this will raise a TypeError in the next release
    130 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/view_paths.rb:11: warning: The last argument is used as the keyword parameter
    108 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/test-prof-0.9.0/lib/test_prof/event_prof/custom_events.rb:10: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
    108 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/testing/integration.rb:41: warning: for `delete' defined here
    102 /home/sam/Source/discourse/app/mailers/user_notifications.rb:614: warning: URI.escape is obsolete
     80 /home/sam/Source/discourse/app/models/remote_theme.rb:149: warning: The last argument is used as the keyword parameter
     79 /home/sam/Source/discourse/app/models/theme.rb:341: warning: for `set_field' defined here
     74 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionview-6.0.1/lib/action_view/renderer/abstract_renderer.rb:20: warning: The last argument is used as the keyword parameter
     72 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/tzinfo-1.2.5/lib/tzinfo/ruby_core_support.rb:142: warning: The last argument is used as the keyword parameter
     72 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/type.rb:27: warning: The last argument is used as the keyword parameter
     72 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/type/adapter_specific_registry.rb:9: warning: for `add_modifier' defined here
     70 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/attribute_mutation_tracker.rb:167: warning: for `changed?' defined here
     68 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/test-prof-0.9.0/lib/test_prof/before_all.rb:77: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
     52 /home/sam/Source/discourse/lib/url_helper.rb:13: warning: URI.escape is obsolete
     52 /home/sam/Source/discourse/app/models/reviewable.rb:399: warning: for `list_for' defined here
     52 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
     48 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/multi_json-1.13.1/lib/multi_json/options_cache.rb:12: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
     46 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activesupport-6.0.1/lib/active_support/inflector/methods.rb:174: warning: for `titleize_without_cache' defined here
     44 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on FalseClass; it always returns nil
     36 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on Float; it always returns nil
     36 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/middleware/static.rb:110: warning: for `initialize' defined here
     36 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/actionpack-6.0.1/lib/action_dispatch/middleware/stack.rb:37: warning: The last argument is used as the keyword parameter
     34 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/excon-0.64.0/lib/excon.rb:178: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
     32 /home/sam/Source/discourse/lib/seed_data/categories.rb:98: warning: for `create_category' defined here
     32 /home/sam/Source/discourse/lib/seed_data/categories.rb:15: warning: The last argument is used as the keyword parameter
     26 /home/sam/Source/discourse/app/controllers/reviewables_controller.rb:34: warning: The last argument is used as the keyword parameter
     26 /home/sam/Source/discourse/app/controllers/reviewables_controller.rb:33: warning: The last argument is used as the keyword parameter
     22 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/onebox-1.9.22/lib/onebox/helpers.rb:222: warning: URI.escape is obsolete
     22 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters//home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/postgresql_adapter.rb:620: warning: given argument is nil; this will raise a TypeError in the next release
     20 postgresql_adapter.rb:620: warning: given argument is nil; this will raise a TypeError in the next release
     20 /home/sam/Source/discourse/app/models/embeddable_host.rb:37: warning: URI.unescape is obsolete
     20 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/rspec-core-3.8.0/lib/rspec/core/metadata.rb:172: warning: deprecated Object#=~ is called on Class; it always returns nil
     18 /home/sam/Source/discourse/lib/seed_data/topics.rb:16: warning: The last argument is used as the keyword parameter
     18 /home/sam/Source/discourse/lib/seed_data/topics.rb:152: warning: for `update_topic' defined here
     18 /home/sam/Source/discourse/lib/seed_data/topics.rb:126: warning: for `create_topic' defined here
     17 /home/sam/Source/discourse/lib/seed_data/topics.rb:26: warning: The last argument is used as the keyword parameter
     16 /home/sam/Source/discourse/lib/seed_data/categories.rb:24: warning: The last argument is used as the keyword parameter
     16 /home/sam/Source/discourse/lib/seed_data/categories.rb:148: warning: for `update_category' defined here
     14 /home/sam/Source/discourse/lib/compression/strategy.rb:35: warning: for `strip_directory' defined here
     14 /home/sam/Source/discourse/lib/compression/engine.rb:25: warning: The last argument is used as the keyword parameter
     14 /home/sam/Source/discourse/app/models/topic.rb:1351: warning: URI.escape is obsolete
     14 /home/sam/.rb/home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/type/integer.rb:13: warning: The last argument is used as the keyword parameter
     13 env/versions/trunk/lib/ruby/gems/2.7.0/gems/activemodel-6.0.1/lib/active_model/type/value.rb:8: warning: for `initialize' defined here
     12 /home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/activ/home/sam/.rbenv/versions/trunk/lib/ruby/gems/2.7.0/gems/activerecord-6.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:274: warning: for `transaction' defined here
     10 /home/sam/Source/discourse/lib/upload_recovery.rb:85: warning: for `recover_post_upload_from_local' defined here
     10 /home/sam/Source/discourse/lib/upload_recovery.rb:64: warning: The last argument is used as the keyword parameter
     10 /home/sam/Source/discourse/app/controllers/metadata_controller.rb:28: warning: given argument is nil; this will raise a TypeError in the next release
      9 cord/persistence.rb:503: warning: The last argument is used as the keyword parameter
      8 ng: The last argument is used as the keyword parameter
      8 /home/sam/Source/discourse/app/models/topic_list.rb:65: warning: The last argument is used as the keyword parameter
      8 /home/sam/Source/discourse/app/models/tag.rb:75: warning: for `top_tags' defined here
      8 ' defined here
      7 ll raise a TypeError in the next release
      7  keyword parameter

Honestly I am not sure if the per call-site thing is needed, I would almost just have a default of count everything and just stop once you reach a number.

Pick a number 1000, 5000 ... something ... once you reach that ... you print to STDERR "Too many warnings regarding keyword args reached, you will no longer be alerted". You could then start small at like 50 for 2.7 release and crank it up to 500 at 2.7.1 if we feel people are not being bugged enough.

Technically even if the limit was 1 line globally ... that is enough to get an entire app fixed.

At the current levels I just see people using RUBYOPT=-W0 cause often they will require complex fixes to gems beyond an app owners controls (especially now that delegation is a nightmare)

[sam.saffron (Sam Saffron)]

Note... some of this fiddling with keyword arguments is showing up as a slowdown in rubybench

https://rubybench.org/ruby/ruby/commits?result_type=app_answer&display_count=2000

[duerst (Martin Dürst)]

• Related to Feature #16345: Don't emit deprecation warnings by default. added

[mame (Yusuke Endoh)]

Matz accepted this with #16345. I'll commit it soon.

[mame (Yusuke Endoh)]

• Status changed from Open to Closed

Applied in changeset git|191ce5344ec42c91571f8f47c85be9138262b1c7.

---

Reduce duplicated warnings for the change of Ruby 3 keyword arguments

By this change, the following code prints only one warning.

def foo(**opt); end
100.times { foo({kw:1}) }

A global variable st_table *caller_to_callees is a map from caller to
a set of callee methods. It remembers that a warning is already printed
for each pair of caller and callee.

[Feature #16289]

[marcandre (Marc-Andre Lafortune)]

I'm glad this was merged in, but disappointed the error message still doesn't mention that this is a deprecation warning.

Wouldn't it be cleared with https://github.com/ruby/ruby/pull/2776 ?

[mame (Yusuke Endoh)]

• Status changed from Closed to Assigned
• Assignee set to mame (Yusuke Endoh)

@marcandre (Marc-Andre Lafortune) , thank you for creating a pull request.

Your proposal:

old: The last argument is used as keyword parameters; maybe ** should be added to the call
new Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

old: The keyword argument is passed as the last hash parameter
new: Passing the keyword argument as the last hash parameter is deprecated

old: The last argument is split into positional and keyword parameters
new: Splitting the last argument into positional and keyword parameters is deprecated

I think they are better. I'm okay to merge your patch, if we can merge it before 2.7.0 release. (But I'm against changing them after the release, i.e., 2.7.1 or later.)

@matz (Yukihiro Matsumoto) @jeremyevans0 (Jeremy Evans) @Eregon (Benoit Daloze) What do you think?

@naruse (Yui NARUSE) If matz accepted, can we merge it?

[Eregon (Benoit Daloze)]

Sounds clearer to me with @marcandre's changes.

[jeremyevans0 (Jeremy Evans)]

I'm fine with @marcandre's changes.

[marcandre (Marc-Andre Lafortune)]

• Status changed from Assigned to Closed

Great, thanks.

Pushed.

[mame (Yusuke Endoh)]

@marcandre (Marc-Andre Lafortune)

You have to wait for matz and naruse's approvals! I'll ask them.

[matz (Yukihiro Matsumoto)]

Approved.

Matz.

[mame (Yusuke Endoh)]

@naruse (Yui NARUSE) also approved. Thank you for quick replies.

[mame (Yusuke Endoh)]

I've updated my pull request to update the article of Ruby 3's keyword argument separation.

https://github.com/ruby/www.ruby-lang.org/pull/2324