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

hash.c: Add a feature to manipulate ruby2_keywords flag #2818

Merged
merged 1 commit into from Jan 17, 2020

Conversation

@mame
Copy link
Member

mame commented Jan 6, 2020

It was found that a feature to check and add ruby2_keywords flag to an
existing Hash is needed when arguments are serialized and deserialized.
It is possible to do the same without explicit APIs, but it would be
good to provide them as a core feature.

rails/rails#38105 (comment)

Hash.ruby2_keywords?(hash) checks if hash is flagged or not.
Hash.ruby2_keywords!(hash) flags a given hash.

static VALUE
rb_hash_s_ruby2_keywords_p(VALUE dummy, VALUE hash)
{
return (RHASH(hash)->basic.flags & RHASH_PASS_AS_KEYWORDS) ? Qtrue : Qfalse;

This comment has been minimized.

Copy link
@jeremyevans

jeremyevans Jan 6, 2020

Contributor

Do you get TypeError when passing non-Hash instance as hash? I would guess no, but I didn't actually check. I think TypeError should be raised in this case. Same issue with rb_hash_s_ruby2_keywords_bang below.

This comment has been minimized.

Copy link
@mame

mame Jan 7, 2020

Author Member

Good catch! I've pushed the fix. Thanks

@eregon
eregon approved these changes Jan 6, 2020
Copy link
Member

eregon left a comment

Looks good to me.

@mame mame force-pushed the mame:ruby2_keywords-flag-manipulation branch from 6753a0d to c3a626a Jan 7, 2020
Copy link
Member

eregon left a comment

Daniel DeLorme made a good comment in https://bugs.ruby-lang.org/issues/16486#note-5.
I think there should be no way to change the ruby2_keywords flag inplace.
Instead, I think we should have hash2 = Hash.mark_ruby2_keywords(hash) (or Hash.as_ruby2_keywords) with hash2 flagged.

Having a method Hash.ruby2_keywords would be problematic as it would conflict with Module#ruby2_keywords.

@kamipo

This comment has been minimized.

Copy link
Contributor

kamipo commented Jan 8, 2020

If there is a way to selialize and deselialize a flagged hash, we as Rails team doesn't require this utility methods.

If this methods are for casual use, I agree that a casual method should not mutate an argument (at least should have an alternative non-mutation version).

If there are two version methods (mutation (non-allocation) version and non-mutation (extra allocation) version), I suppose we would use non-allocation version in the activejob case.

https://gist.github.com/mame/18cd2dfc7d965d0bad1216f2fdd008ee#file-bouncing_test-patch-L152

@mame

This comment has been minimized.

Copy link
Member Author

mame commented Jan 8, 2020

@eregon IMO, the mutating version is enough here because it is not for a casual use; in the ActiveJob case, mutating version is slightly preferable (as @kamipo said), and currently there is no known use case where non-mutating version is preferable. But I'll discuss the behavior (and method name) at the next dev-meeting.

Having a method Hash.ruby2_keywords would be problematic as it would conflict with Module#ruby2_keywords.

Good point, I didn't notice. Thanks!

@kamipo Thank you for your comment.

If there is a way to selialize and deselialize a flagged hash, we as Rails team doesn't require this utility methods.

As far as I understand, ActiveJob has a dedicated serialization mechanism which cannot be controlled by the ruby core. So, all we (ruby-core team) can do is to provide a way to check and add the ruby2_keywords flag, and I'd like to ask you to use the feature to fix ActiveJob serialization code.

If there are two version methods (mutation (non-allocation) version and non-mutation (extra allocation) version), I suppose we would use non-allocation version in the activejob case.

Yes, that is why I'm going to introduce mutating version.

rb_hash_s_ruby2_keywords_bang(VALUE dummy, VALUE hash)
{
Check_Type(hash, T_HASH);
RHASH(hash)->basic.flags |= RHASH_PASS_AS_KEYWORDS;

This comment has been minimized.

Copy link
@eregon

eregon Jan 8, 2020

Member

Should this check if the Hash is frozen?

This comment has been minimized.

Copy link
@mame

mame Jan 8, 2020

Author Member

Ah, good point. Should we raise an exception?

This comment has been minimized.

Copy link
@eregon

eregon Jan 9, 2020

Member

I think so, yes. But I would prefer if it was not mutating at all.

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Jan 9, 2020

@eregon IMO, the mutating version is enough here because it is not for a casual use; in the ActiveJob case, mutating version is slightly preferable (as @kamipo said), and currently there is no known use case where non-mutating version is preferable. But I'll discuss the behavior (and method name) at the next dev-meeting.

The problem is this tiny decision completely prevents other implementations or designs for ruby2_keywords. For instance, if we wanted to try using a KwHash subclass instead of a flag, as suggested in https://bugs.ruby-lang.org/issues/16463#note-20, the sole existence of Hash.ruby2_keywords! prevents it entirely, or would require to change the class of an existing object which is extremely ugly (for semantics at least).
It also makes debugging significantly harder IMHO.

So I think making it mutating here is needlessly restricting, and I oppose it.
Making a copy of the Hash is not a big cost when it's deserialized before.

Maybe we should not even introduce a new way to flag, as it's so easy to do it manually:

ruby2_keywords def flag_kwhash(*args)
  args.last
end

kw = flag_kwhash(**h)
@mame mame force-pushed the mame:ruby2_keywords-flag-manipulation branch 2 times, most recently from 4921c8b to 03bed4d Jan 17, 2020
It was found that a feature to check and add ruby2_keywords flag to an
existing Hash is needed when arguments are serialized and deserialized.
It is possible to do the same without explicit APIs, but it would be
good to provide them as a core feature.

rails/rails#38105 (comment)

Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not.
Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a
ruby2_keywords flag,

[Bug #16486]
@mame mame force-pushed the mame:ruby2_keywords-flag-manipulation branch from 03bed4d to 7d6ddcc Jan 17, 2020
@mame mame merged commit 7cfe93c into ruby:master Jan 17, 2020
1 of 15 checks passed
1 of 15 checks passed
make (test, windows-2019, 2019) make (test, windows-2019, 2019)
Details
make (check, --jit) make (check, --jit)
Details
make (check) make (check)
Details
make (check, ubuntu-latest) make (check, ubuntu-latest)
Details
make (test, windows-2019, 2019) make (test, windows-2019, 2019)
Details
make (check) make (check)
Details
check_branch
Details
make (check, --jit-wait) make (check, --jit-wait)
Details
make (check, ubuntu-16.04) make (check, ubuntu-16.04)
Details
make (test-bundler) make (test-bundler)
Details
make (test-bundler, ubuntu-latest) make (test-bundler, ubuntu-latest)
Details
make (test-bundled-gems) make (test-bundled-gems)
Details
make (test-bundled-gems, ubuntu-latest) make (test-bundled-gems, ubuntu-latest)
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@mame mame deleted the mame:ruby2_keywords-flag-manipulation branch Jan 17, 2020
kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2020
Related rails#38053, rails#38187, rails#38105.

This is a reattempt to fix keyword arguments warnings in Active Job.

Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be
backported to 2.7.1.

ruby/ruby#2818
https://bugs.ruby-lang.org/issues/16486

I've emulated that for 2.7.0 and older versions.
kamipo added a commit to kamipo/ruby that referenced this pull request Jan 19, 2020
In ruby#2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
mame added a commit that referenced this pull request Jan 19, 2020
In #2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
kamipo added a commit to kamipo/rails that referenced this pull request Jan 20, 2020
Related rails#38053, rails#38187, rails#38105, rails#38260.

This is a reattempt to fix keyword arguments warnings in Active Job.

Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be
backported to 2.7.1.

ruby/ruby#2818
https://bugs.ruby-lang.org/issues/16486

I've emulated that for 2.7.0 and older versions.
matzbot pushed a commit that referenced this pull request Feb 18, 2020
In #2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.