Improve our SafeDup module #2960
Conversation
I noticed that calling `SafeDup.frozen_or_dup({a: :b})` on ruby 3.2 would break;
because it tries to call `+(-){a: :b}` which doesn't work.
I had to ensure the `SafeDup` module worked for all objects.
I decided to extract that work into a separate PR to ease the work of reviewing.
The method `frozen_or_dup` and `frozen_dup` for ruby 2.3 and forward only expect to receive String values.
This could lead to future errors.
By checking if the value is a String we can warranty the same behaviour for all kinds of objects
Also, I added some extract refimients to be able to call `#dup` on `true, false, 1, 1.0` values.
GustavoCaso
force-pushed
the
imporve-safe-dup-module
branch
from
63cbbc8
to
8b5983e
Compare
Codecov Report
@@ Coverage Diff @@
## master #2960 +/- ##
==========================================
+ Coverage 97.96% 98.07% +0.10%
==========================================
Files 1287 1301 +14
Lines 71025 71955 +930
Branches 3285 3308 +23
==========================================
+ Hits 69581 70571 +990
+ Misses 1444 1384 -60
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
lib/datadog/core/utils/safe_dup.rb
Outdated
| # - then it will dup it more efficiently with +v | ||
| v.frozen? ? v : +(-v) | ||
| case v | ||
| when String |
There was a problem hiding this comment.
Can you capture here why String is handled separately from other types?
There was a problem hiding this comment.
Minor: Isn't case v .... when String ... else ... the same as if String === v ... else? (Same applies below as well)
lib/datadog/core/utils/safe_dup.rb
Outdated
| end | ||
|
|
||
| def self.frozen_dup(v) | ||
| -v if v | ||
| case v | ||
| when String |
lib/datadog/core/utils/safe_dup.rb
Outdated
| # - then it will dup it more efficiently with +v | ||
| v.frozen? ? v : +(-v) | ||
| case v | ||
| when String |
There was a problem hiding this comment.
Minor: Isn't case v .... when String ... else ... the same as if String === v ... else? (Same applies below as well)
GustavoCaso
force-pushed
the
imporve-safe-dup-module
branch
from
4777667
to
be7e099
Compare
lib/datadog/core/utils/safe_dup.rb
Outdated
| # they are faster and chepaer on the memory side. | ||
| # Check the benchmark on | ||
| # https://github.com/DataDog/dd-trace-rb/pull/2704 | ||
| if v === String |
There was a problem hiding this comment.
Ah wait, it's not the same thing between v === String and String === v as Class#=== checks for instances but not the way around:
[12] pry(main)> "foo" === String
=> false
[13] pry(main)> String === "foo"
=> true
[14] pry(main)> class MyString < String
[14] pry(main)* end
=> nil
[15] pry(main)> String === MyString.new
=> true
There was a problem hiding this comment.
Interestingly enough, since this code path is a fast path, the tests don't catch this error ;)
There was a problem hiding this comment.
I will use v.is_a?(String) is quite readable
What does this PR do?
When working on #2936 I noticed that calling
SafeDup.frozen_or_dup({a: :b})on ruby 3.2 would break; because it tries to call+(-){a: :b}which doesn't work.I had to ensure the
SafeDupmodule worked for all objects.I decided to extract that work into a separate PR to ease the work of reviewing.
The method
frozen_or_dupandfrozen_dupfor ruby 2.3 and forward only expect to receive String values. This could lead to future errors.By checking if the value is a String, we can warranty the same behaviour for all kinds of objects
I extracted the refinements into the
backportfile under theBackportFrom24namespaceMotivation
Additional Notes
Interesting read on the whole
dupandclonedebacle for ruby versions https://blog.arkency.com/2017/03/prototypes-in-ruby/How to test the change?