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