Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set maximum supported Ruby version #2497

Merged
merged 14 commits into from
Jul 20, 2023
Merged

Set maximum supported Ruby version #2497

merged 14 commits into from
Jul 20, 2023

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Dec 20, 2022

Bump maximum Ruby version to the next, unreleased version 3.3 (< 3.4 in practice).

This PR also changes the comment around the MAXIMUM_RUBY_VERSION constant in order to document why we limit it, and what version we limit it to.

lib/ddtrace/version.rb Outdated Show resolved Hide resolved
@lloeki lloeki mentioned this pull request Dec 29, 2022
@marcotc marcotc changed the title [WIP] Support Ruby 3.2 Support Ruby 3.2 Jan 6, 2023
@lloeki
Copy link
Member

lloeki commented Jan 11, 2023

From the release announcement:

Notable changes that may be impactful, sometimes silently (as in they may appear to work but displace/lose data):

Delegation of args and kwargs

ruby2_keywords is still there, but has a breaking change:

Methods taking a rest parameter (like *args) and wishing to delegate keyword arguments through foo(*args) must now be marked with ruby2_keywords (if not already the case). In other words, all methods wishing to delegate keyword arguments through *args must now be marked with ruby2_keywords, with no exception. This will make it easier to transition to other ways of delegation once a library can require Ruby 3+. Previously, the ruby2_keywords flag was kept if the receiving method took *args, but this was a bug and an inconsistency. A good technique to find potentially missing ruby2_keywords is to run the test suite, find the last method which must receive keyword arguments for each place where the test suite fails, and use puts nil, caller, nil there. Then check that each method/block on the call chain which must delegate keywords is correctly marked with ruby2_keywords.

  def target(**kw)
  end

  # Accidentally worked without ruby2_keywords in Ruby 2.7-3.1, ruby2_keywords
  # needed in 3.2+. Just like (*args, **kwargs) or (...) would be needed on
  # both #foo and #bar when migrating away from ruby2_keywords.
  ruby2_keywords def bar(*args)
    target(*args)
  end

  ruby2_keywords def foo(*args)
    bar(*args)
  end

  foo(k: 1)

We must make sure to have ruby2_keywords everywhere we expect *args to delegate both *args and **kwargs. Depending on code/specs things may appear to silently work but produce incorrect results.

Long term, in order to support both ruby <= 2.6, ruby 2.7, ruby 3.0+, and the future ruby version where ruby2_keywords is removed, we have to split such cases in two:

if RUBY_VERSION < '3.0'
  def foo(*args)
     bar(*args)
  end

  def bar(*args)
     target(*args)
  end
else
  def foo(*args, **kwargs)
     bar(*args, **kwargs)
  end

  def bar(*args, **kwargs)
     target(*args, **kwargs)
  end
end

Note: 2.7 has slightly different behaviour than 2.6 and not so slightly than 3.0, sometimes depending how foo is called. Also there's one old ruby (I think it's 2.2?) which has a very subtle behaviour change as well that was reverted on the next version. The above is what should work in all cases.

Proc arguments don't autosplat anymore

A proc that accepts a single positional argument and keywords will no longer autosplat.

proc{|a, **k| a}.call([1, 2])
# Ruby 3.1 and before
# => 1
# Ruby 3.2 and after
# => [1, 2]

We should check whether we have such cases. Depending on code/specs things may appear to silently work but produce incorrect results.

Constant assignment evaluation order

Constant assignment evaluation order for constants set on explicit objects has been made consistent with single attribute assignment evaluation order. With this code:

foo::BAR = baz

foo is now called before baz

This doesn't appear to impact us: rg '\W[^A-Z][a-z]+::' lib returns no such assignment.

Hash#shift behaviour on empty hash

Hash#shift now always returns nil if the hash is empty, instead of returning the default value or calling the default proc

We don't appear to be using this method: rg shift lib returns few results, all on arrays.

Removed methods / C API stuff

The following deprecated methods are removed.

  • Dir.exists?
  • File.exists?
  • Kernel#=~
  • Kernel#taint, Kernel#untaint, Kernel#tainted?
  • Kernel#trust, Kernel#untrust, Kernel#untrusted?
  • rb_cData
  • C “taintedness” and “trustedness” functions

The following APIs are updated.

  • rb_random_interface_t

We don't appear to be using any of these.

@unicornzero
Copy link

unicornzero commented Feb 6, 2023

I'm adding dd-trace-rb to an app already on Ruby 3.2, running into issue #2534 would love to know when Ruby 3.2 support will be available.

@ivoanjo
Copy link
Member

ivoanjo commented Feb 6, 2023

There's quite a few things here -- I plan to separate this PR out a bit, and make sure we have Ruby 3.2 support for the next release of dd-trace-rb.

@ivoanjo
Copy link
Member

ivoanjo commented Feb 7, 2023

This PR is mostly replaced by #2601 . I'm leaving it open as we may still want to import the tweak to the ruby maximum version.

@github-actions github-actions bot added the release Official release label Feb 15, 2023
@marcotc marcotc changed the title Support Ruby 3.2 Set maximum supported Ruby version Feb 15, 2023
@marcotc marcotc added dev/internal Other internal work that does not need to be included in the changelog and removed release Official release labels Feb 16, 2023
@marcotc marcotc self-assigned this Feb 16, 2023
@marcotc marcotc marked this pull request as ready for review February 16, 2023 18:40
@marcotc marcotc requested a review from a team February 16, 2023 18:40
@marcotc
Copy link
Member Author

marcotc commented Feb 16, 2023

I've refreshed this PR, and now it tackles only the maximum version change.

@marcotc marcotc requested a review from ivoanjo February 16, 2023 18:41
@marcotc marcotc requested a review from lloeki February 16, 2023 18:41
@codecov-commenter
Copy link

Codecov Report

Merging #2497 (4a1f5b6) into master (ba7fd9c) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2497      +/-   ##
==========================================
- Coverage   98.02%   98.01%   -0.01%     
==========================================
  Files        1148     1148              
  Lines       62453    62454       +1     
  Branches     2808     2808              
==========================================
- Hits        61218    61217       -1     
- Misses       1235     1237       +2     
Impacted Files Coverage Δ
spec/datadog/core/runtime/metrics_spec.rb 97.72% <0.00%> (-2.28%) ⬇️
...ng/contrib/active_support/cache/instrumentation.rb 86.30% <0.00%> (+0.09%) ⬆️
lib/datadog/core/workers/polling.rb 100.00% <0.00%> (+3.44%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ivoanjo
Copy link
Member

ivoanjo commented Feb 17, 2023

This looks good to me, although I think there was some ongoing discussion internally on < 3.3 vs < 3.4 so I guess we should probably arrive to a conclusion.

Also -- can I annoy you to remove the old commits from the PR and keep only the latest change? ;)

@marcotc
Copy link
Member Author

marcotc commented Feb 17, 2023

Also -- can I annoy you to remove the old commits from the PR and keep only the latest change? ;)

But then I can't inflate my commit stats in the repo!

@ivoanjo
Copy link
Member

ivoanjo commented Feb 20, 2023

But then I can't inflate my commit stats in the repo!

Ha! Only I am allowed to do that xD

@github-actions github-actions bot added the release Official release label Jun 28, 2023
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM and thanks for capturing our reasoning.

I agree it makes sense to double-check with Loic as you mentioned on slack, since he's also been very interested in this discussion so far :)

@lloeki
Copy link
Member

lloeki commented Jun 30, 2023

I feel like we should still keep a note about ruby2_keywords in the comments, but otherwise I'm OK with the change.

@ivoanjo
Copy link
Member

ivoanjo commented Jul 3, 2023

I feel like we should still keep a note about ruby2_keywords in the comments, but otherwise I'm OK with the change.

I don't mind keeping the comment, but I think realistically that's yet another thing that was maybe-planned by Ruby core but didn't pan out lol.

The Ruby codebase has a lot of other comments about things being experimental or removed soon and that weren't changed yet, so I'm not sure if we should worry about that one specifically :)

(I'm still waiting for my frozen-by-default strings without magic comment :P )

@marcotc marcotc merged commit 8bc9600 into master Jul 20, 2023
@marcotc marcotc deleted the 3.2 branch July 20, 2023 17:11
@github-actions github-actions bot added this to the 1.13.0 milestone Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/internal Other internal work that does not need to be included in the changelog release Official release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants