Ruby 3: Deprecations are not caught in irb and not by DeprecationToolkit if the method is stubbed
Copied from slack:
There are 2 loopholes due to which kwargs deprecations might go unnoticed:
- The author has stubbed this method in tests. We run automated detection for this warning in tests via
deprecation_toolkit
, but it relies on the fact thatKernel#warn
emits a warning. Stubbing out the implementation removes that warning, and we never pick it up, so the build is green. - Testing in
irb
/rails c
silences the deprecation warning, sinceirb
in Ruby 2.7.x has a bug that prevents deprecation warnings from showing: https://bugs.ruby-lang.org/issues/17377 For now we just need to be extra vigilant. All occurrences off({k: v})
should raise your eyebrows. This is only allowed in Ruby 3 iff
actually takes a Hash, not keyword arguments.
More context: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/2461#note_967340801 [internal only]
https://docs.gitlab.com/ee/development/ruby3_gotchas.html
The (interesting parts of the) discussion from the internal MR:
Stubbed method in tests:
Yes I had to go back and understand how DeprecationToolkit even works; it patches Warning
(which is the impl of Kernel#warn
and used to issue deprecation warnings by the VM): https://github.com/Shopify/deprecation_toolkit/blob/main/lib/deprecation_toolkit/warning.rb#L46-L60
These deprecations warnings are issued by the VM when it executes the iseqs representing the method call:
else if (args_pop_keyword_hash(args, &keyword_hash, 1)) {
/* Warn the following:
* def foo(k:1) p [k]; end
* foo({k:42}) #=> 42
*/
rb_warn_last_hash_to_keyword(ec, calling, ci, iseq);
given_argc--;
}
https://github.com/ruby/ruby/blob/v2_7_5/vm_args.c#L934-L941
So stubbing out this call will effectively remove the call to warn
, so DT never sees it.
Testing in irb
/rails c
:
This might be a bug in irb
: https://bugs.ruby-lang.org/issues/17377; even with -W2
it does not print deprecation warnings.
On the CLI, I see it:
$ ruby -W2 -e 'def f(a:); pp a; end; f({a: 42})'
-e:1: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
-e:1: warning: The called method `f' is defined here
42
As indicated in the PR for that bug report, you can see it like so:
[12:49:37] work/gl-gck::master ✔ irb -W2
irb(main):001:0> Warning[:deprecated] = true
=> true
irb(main):002:0> def f(a:); pp a; end
=> :f
irb(main):003:0> f({a: 42})
(irb):3: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
(irb):2: warning: The called method `f' is defined here
42
=> 42
The problem:
It might be difficult to detect, since even a Cop will likely not suffice (it can only reason about syntax, but this issue can take many forms that are harder to catch than passing a hash literal)
The problem is that QA runs do not provide sufficient code coverage, so we cannot even fully rely on the 2-hour Ruby 3 pipeline to catch these issues.