Feature #16289
Reduce duplicated warnings for the change of Ruby 3 keyword arguments
Description
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?
Related issues
Updated by marcandre (Marc-Andre Lafortune) over 1 year ago
Definite +1 for me. I thought there was already an issue about this but looks I was mistaken.
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
- Description updated (diff)
It leads a memory leak.
And different warnings won't be suppressed too?
Updated by mame (Yusuke Endoh) over 1 year ago
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.
Updated by mame (Yusuke Endoh) over 1 year ago
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.
Updated by Eregon (Benoit Daloze) over 1 year ago
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
Updated by marcandre (Marc-Andre Lafortune) over 1 year ago
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
Updated by Dan0042 (Daniel DeLorme) over 1 year ago
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.
Updated by shevegen (Robert A. Heiler) over 1 year ago
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.
Updated by Eregon (Benoit Daloze) over 1 year ago
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.
Updated by sam.saffron (Sam Saffron) over 1 year ago
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)
Updated by sam.saffron (Sam Saffron) over 1 year ago
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
Updated by duerst (Martin Dürst) over 1 year ago
- Related to Feature #16345: Don't emit deprecation warnings by default. added
Updated by mame (Yusuke Endoh) about 1 year ago
Matz accepted this with #16345. I'll commit it soon.
Updated by mame (Yusuke Endoh) about 1 year ago
- 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]
Updated by marcandre (Marc-Andre Lafortune) about 1 year ago
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 ?
Updated by mame (Yusuke Endoh) about 1 year ago
- Assignee set to mame (Yusuke Endoh)
- Status changed from Closed to Assigned
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?
Updated by Eregon (Benoit Daloze) about 1 year ago
Sounds clearer to me with marcandre (Marc-Andre Lafortune)'s changes.
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
I'm fine with marcandre (Marc-Andre Lafortune)'s changes.
Updated by marcandre (Marc-Andre Lafortune) about 1 year ago
- Status changed from Assigned to Closed
Great, thanks.
Pushed.
Updated by mame (Yusuke Endoh) about 1 year ago
marcandre (Marc-Andre Lafortune)
You have to wait for matz and naruse's approvals! I'll ask them.
Updated by mame (Yusuke Endoh) about 1 year ago
naruse (Yui NARUSE) also approved. Thank you for quick replies.
Updated by mame (Yusuke Endoh) about 1 year ago
I've updated my pull request to update the article of Ruby 3's keyword argument separation.