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

[Ruby 3.0 support] Handle Kernel#clone with freeze: true argument #2512

Conversation

andrykonchin
Copy link
Member

@andrykonchin andrykonchin commented Oct 20, 2021

This PR addresses issues:

[medium, java] Kernel#clone when called with the freeze: false keyword will call
#initialize_clone with the freeze: false keyword.
[Bug #14266]

and

[medium, java] Kernel#clone when called with the freeze: true keyword will call
#initialize_clone with the freeze: true keyword, and will
return a frozen copy even if the receiver is unfrozen.
[Feature #16175]

Related issue - #2453

Changes

  • return a frozen copy when freeze: true
  • handle freeze: nil
  • pass freeze: to #initialize_clone

@andrykonchin
Copy link
Member Author

I, probably, need help with passing a keyword argument to the #initialize_clone call here

initializeCloneNode.call(newObject, "initialize_clone", object);

I haven't found any similar example of calling a Ruby method from Java code with keyword arguments. Could you please point me to such an example?

@eregon
Copy link
Member

eregon commented Oct 20, 2021

I, probably, need help with passing a keyword argument to the #initialize_clone call here

Because we didn't do the keyword arguments changes yet for Ruby 3 it's just about passing a Hash with :freeze and true, but building that from Java is a little bit verbose, see org.truffleruby.core.hash.HashNodes.ConstructNode#construct (new RubyHash, PackedHashStoreLibrary.createStore(), PackedHashStoreLibrary.setHashedKeyValue).

(One easier way would be to create a (Ruby) method on Truffle::KernelOperations that calls it, and call that method, but that would then introduce an extra call which feels rather suboptimal.)

@eregon
Copy link
Member

eregon commented Oct 20, 2021

If you need any help don't hesitate to join our Slack as then it's easier to reply in real time.

@andrykonchin andrykonchin force-pushed the ruby-3-0-support-kernel-clone-with-freeze-argument branch from f3869b6 to f85852e Compare October 22, 2021 11:32
@andrykonchin
Copy link
Member Author

andrykonchin commented Nov 2, 2021

I would appreciate any advice regarding failed specs:

1)
Kernel#clone with freeze: true calls #initialize_clone with kwargs freeze: true FAILED
Expected [#<KernelSpecs::CloneFreeze:0x558>, {[:freeze, true]=>{}}] == [#<KernelSpecs::CloneFreeze:0x558>, {[:freeze, true]=>{}}]
to be truthy but was false
...spec/ruby/core/kernel/clone_spec.rb:97:in `block (4 levels) in <top (required)>'
...spec/ruby/core/kernel/clone_spec.rb:5:in `<top (required)>'

2)
Kernel#clone with freeze: false calls #initialize_clone with kwargs freeze: false FAILED
Expected [#<KernelSpecs::CloneFreeze:0x638>, {[:freeze, false]=>{}}] == [#<KernelSpecs::CloneFreeze:0x638>, {[:freeze, false]=>{}}]
to be truthy but was false
...spec/ruby/core/kernel/clone_spec.rb:129:in `block (4 levels) in <top (required)>'
...spec/ruby/core/kernel/clone_spec.rb:5:in `<top (required)>'

So actually {[:freeze, true]=>{}} doesn't equal {[:freeze, true]=>{}}.

I suppose there is an issue with a string representation of RubyHash. Actually, it's a correct Hash { freeze: true }.

And finally a key :freeze in recorded arguments doesn't equal a Symbol literal :freeze in the test case. I suppose it's related to how the key was created in the clone Java method.

if (toForceFreezing(freeze) || toForceUnfreezing(freeze)) {
final String string = "freeze";
final LeafRope rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE);
final RubySymbol key = new RubySymbol(string, rope, Encodings.US_ASCII);
Copy link
Member

Choose a reason for hiding this comment

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

The RubySymbol should be package-private so it's clear it shouldn't be used directly.
There is already coreSymbols().FREEZE which you can use :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. It fixed the failed specs as well 🎆

def clone(freeze: true)
unless freeze
def clone(freeze: nil)
if freeze == false
Copy link
Member

Choose a reason for hiding this comment

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

Primitive.object_equal or false == freeze is safer here to avoid calling == on some potentially random object.

src/main/java/org/truffleruby/core/kernel/KernelNodes.java Outdated Show resolved Hide resolved
final String string = "freeze";
final LeafRope rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE);
final RubySymbol key = new RubySymbol(string, rope, Encodings.US_ASCII);
final boolean value = toForceFreezing(freeze);
Copy link
Member

Choose a reason for hiding this comment

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

We can just use freeze later instead of value, it can only be a boolean at this point.

final int hashed = hashNode.execute(key);
PackedHashStoreLibrary.setHashedKeyValue(newStore, 0, hashed, key, value);

final RubyHash hash = new RubyHash(
Copy link
Member

Choose a reason for hiding this comment

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

This logic to create the Hash is a bit verbose, could you extract it to a method createFreezeBooleanHash() or so?

@andrykonchin
Copy link
Member Author

Done. Please review.

@andrykonchin andrykonchin marked this pull request as ready for review November 3, 2021 20:33
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you for the great work on this PR!

@eregon eregon self-assigned this Nov 4, 2021
@eregon eregon added this to the 22.0.0 milestone Nov 4, 2021
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 4, 2021
@andrykonchin
Copy link
Member Author

Thank you for your help 🙇.

I will squash commits and rebase on the master branch soon.

@eregon
Copy link
Member

eregon commented Nov 4, 2021

I pushed a couple commits on top, I'll do the rebase now.

@eregon eregon force-pushed the ruby-3-0-support-kernel-clone-with-freeze-argument branch from baaa212 to bb9b468 Compare November 4, 2021 17:18
@eregon
Copy link
Member

eregon commented Nov 4, 2021

I squashed the second commit in the first one to make it easier to rebase, should be all good now.

@eregon eregon force-pushed the ruby-3-0-support-kernel-clone-with-freeze-argument branch from 0cdfaa3 to 43d574c Compare November 4, 2021 17:23
@graalvmbot graalvmbot merged commit 36989e8 into oracle:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants