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

Fix storing of typed Hashes from a list #5172

Merged
merged 1 commit into from Jan 28, 2023
Merged

Conversation

2colours
Copy link
Contributor

@2colours 2colours commented Jan 27, 2023

Prior to this commit, a Hash with typed keys ("hash object") couldn't be
properly stored in another Hash, typed or untyped, if it appeared in a
list. This is a regression from July 2016,
commit ecfb956 .
In the affected timespan, the behavior was the following:

my %h{Any} = foo => 'bar', 12 => 'baz';
my %copy = (%h, );

The copy got recreated as the internal, $!storage representation.
Because of the current default implementation of .hash, the problem
appeared also when calling the .hash method on a list that contained a
"hash object", or enforcing hash context upon them with %().

STORE_AT_KEY is the method that takes care of creating an entry for an
implementation of a Hash, transforming the key and the value in a way the
underlying $!storage BOOTHash can hold them. When different
implementations of Hash are stored into another Hash (previously done with
the STORE_MAP method), this needs to be taken into account.

This commit fixes the problem by replacing non-dispatching STORE_MAP calls
with a PUSH_FROM_MAP method that needs to be provided for those
implementations of a Hash that have their own mapping to $!storage,
that is, typically the core implementations that have their own
STORE_AT_KEY. This PUSH_FROM_MAP knows how STORE_AT_KEY needs to be
called with regards to all of the elements retrieved from the Hash object
it's invoked on, meaning that all STORE_AT_KEY calls will happen with the
right arguments, at the cost of only one dispatch to the right PUSH_FROM_MAP
candidate.

Fixes #5165

@@ -33,6 +33,19 @@ my role Hash::Object[::TValue, ::TKey] does Associative[TValue] {
)
}

method PUSH_FROM_ITER(Mu \iter, \target --> Nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Guessing, this one has to be an implementation details too, same as on Hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I was just taking STORE_AT_KEY as the basis and saw no trait on it.

@lizmat
Copy link
Contributor

lizmat commented Jan 27, 2023

After mulling a bit about this while cycling, I wonder why not change the one call to !STORE_MAP at https://github.com/rakudo/rakudo/blob/main/src/core.c/Hash.pm6#L97 to $x.PUSH_FROM_ITER($temp) and do the iterator creation inside those methods.

And then probably rename it to PUSH_FROM_MAP ?

@2colours
Copy link
Contributor Author

Sounds good. I can make a new PR with that, if nobody objects.

@lizmat
Copy link
Contributor

lizmat commented Jan 27, 2023

Or just adapt this PR :-) either would be fine

Prior to this commit, a Hash with typed keys ("hash object") couldn't be
properly stored in another Hash, typed or untyped, if it appeared in a
list. This is a regression from July 2016,
commit ecfb956 .
In the affected timespan, the behavior was the following:

my %h{Any} = foo => 'bar', 12 => 'baz';
my %copy = (%h, );

The copy got recreated as the internal, `$!storage` representation.
Because of the current default implementation of `.hash`, the problem
appeared also when calling the `.hash` method on a list that contained a
"hash object", or enforcing hash context upon them with %().

STORE_AT_KEY is the method that takes care of creating an entry for an
implementation of a Hash, transforming the key and the value in a way the
underlying `$!storage` BOOTHash can hold them. When different
implementations of Hash are stored into another Hash (previously done with
the STORE_MAP method), this needs to be taken into account.

This commit fixes the problem by replacing non-dispatching STORE_MAP calls
with a PUSH_FROM_MAP method that needs to be provided for those
implementations of a Hash that have their own mapping to `$!storage`,
that is, typically the core implementations that have their own
STORE_AT_KEY. This PUSH_FROM_MAP knows how STORE_AT_KEY needs to be
called with regards to all of the elements retrieved from the Hash object
it's invoked on, meaning that all STORE_AT_KEY calls will happen with the
right arguments, at the cost of only one dispatch to the right PUSH_FROM_MAP
candidate.
@2colours 2colours requested review from lizmat and vrurg and removed request for lizmat January 28, 2023 13:28
@2colours
Copy link
Contributor Author

2colours commented Jan 28, 2023

Please check out the updated PR. :) I noticed a failing Rakudo test on my local installation while all spectests were passing - it was a test about profiling some way, I will take a look at it again. Otherwise it would seem that this is working now.

EDIT: I reran the Rakudo test and it passed now... could be that the test case has some non-determinism. 🤷‍♂️

@lizmat lizmat merged commit c9e391f into rakudo:main Jan 28, 2023
@lizmat
Copy link
Contributor

lizmat commented Jan 28, 2023

Thanks!

@2colours 2colours deleted the issue-5165-fix branch January 28, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameterised hashes implementation detail sometimes leaks out
3 participants