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

OptionMergerTest#test_method_with_options_copies_options_when_options_are_missing fails with Ruby 3.3.0dev #47627

Closed
yahonda opened this issue Mar 10, 2023 · 5 comments

Comments

@yahonda
Copy link
Member

yahonda commented Mar 10, 2023

Managed to reproduce the CI failure at https://buildkite.com/rails/rails/builds/94497#0186be64-c342-4494-b50b-e0362040cc9e/1134-1174 and found it is triggered by ruby/ruby@e87d088

Steps to reproduce

Install Ruby 3.3.0dev using `rbenv install 3.3.0-dev`
git clone https://github.com/rails/rails
cd rails
rm Gemfile.lock
bundle install
cd activesupport
bin/test test/option_merger_test.rb -n test_method_with_options_copies_options_when_options_are_missing

Expected behavior

It should pass.

Actual behavior

$ bin/test test/option_merger_test.rb -n test_method_with_options_copies_options_when_options_are_missing
Running 14 tests in a single process (parallelization threshold is 50)
Run options: -n test_method_with_options_copies_options_when_options_are_missing --seed 51119

# Running:

F

Failure:
OptionMergerTest#test_method_with_options_copies_options_when_options_are_missing [/home/yahonda/src/github.com/rails/rails/activesupport/test/option_merger_test.rb:42]:
Expected {:hello=>"world"} (oid=5300) to not be the same as {:hello=>"world"} (oid=5300).


bin/test test/option_merger_test.rb:40



Finished in 0.000405s, 2472.0410 runs/s, 2472.0410 assertions/s.
1 runs, 1 assertions, 1 failures, 0 errors, 0 skips
$

System configuration

Rails version: main branch

Ruby version: ruby 3.3.0dev (2023-03-06T06:03:06Z master e87d088291) [x86_64-linux]

@yahonda
Copy link
Member Author

yahonda commented Mar 10, 2023

cc @eugeneius Just wanted to ask you before opening a bug report to bugs.ruby-lang.org because this test code has been added via #39407

@eugeneius
Copy link
Member

👋 @yahonda! Here's the smallest test case I could find (prints false on 3.2.1, true on 3.3.0-dev):

options = { hello: "world" }

def test(opts) = opts

puts test(*[], **options).equal?(options)

I guess since ruby/ruby@e87d088, passing a hash as keyword arguments—along with positional arguments—to a method that doesn't accept keyword arguments now forwards the original hash as the final positional parameter instead of a copy.

Note that this was already the case when calling the method without positional arguments; the following prints true all the way back to Ruby 3.0:

options = { hello: "world" }

def test(opts) = opts

puts test(**options).equal?(options)

I think we should fix this in Rails by reverting 9e4ff29.

I don't know whether this should be reported upstream or not; it's an observable change in behaviour, but copying the hash previously may have been unintentional since it didn't happen unless positional arguments were also passed. I think you're in a better position to make that decision than I am, since you're closer to the Ruby project.

@yahonda
Copy link
Member Author

yahonda commented Mar 12, 2023

Thanks for the detailed information and the minimal test case. I'm inclined to report this behavior change to bugs.ruby-lang.org.
Because ruby/ruby#7128 says this is ruby/ruby#6920, which aims for better performance. I'd like to hear from Ruby developers to see if it is an expected behavior.

@yahonda
Copy link
Member Author

yahonda commented Mar 12, 2023

@yahonda
Copy link
Member Author

yahonda commented Mar 15, 2023

Confirmed ruby/ruby@6462c1a042 via ruby/ruby#7507 addressed this issue.

$ ruby -v
ruby 3.3.0dev (2023-03-15T09:05:13Z master 6462c1a042) [x86_64-linux]
$ bin/test test/option_merger_test.rb -n test_method_with_options_copies_options_when_options_are_missing
Running 14 tests in a single process (parallelization threshold is 50)
Run options: -n test_method_with_options_copies_options_when_options_are_missing --seed 1049

# Running:

.

Finished in 0.000423s, 2365.4418 runs/s, 2365.4418 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips
$

CI is also green https://buildkite.com/rails/rails/builds/94785#0186e624-fed8-4300-be93-81050571d845 .

@yahonda yahonda closed this as completed Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants