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

Issue with double splat merge in Dash #353

Closed
boie0025 opened this issue Mar 9, 2016 · 5 comments
Closed

Issue with double splat merge in Dash #353

boie0025 opened this issue Mar 9, 2016 · 5 comments

Comments

@boie0025
Copy link

boie0025 commented Mar 9, 2016

On Ruby (MRI) 2.3.0

Given a Dash class like:

class PersonHash < Hashie::Dash
  property :name
  property :nickname
  property :identifier
end

When I create an instance of PersonHash

person = PersonHash.new(name: 'Nic')

And create a new hash with double splatted person as an argument:

{ **person, height: '5 feet' }
# => {**person, height: '5 feet'}

I get a Hashie::Dash instance which includes the non-defined Dash attribute

{ **person, height: '5 feet' }.class
# => PersonHash

And can not access the non-defined attribute

{ **person, height: '5 feet' }.height
# => NoMethodError: undefined method `height' for #<PersonHash:0x00...>
{ **person, height: '5 feet' }[:height]
# => NoMethodError: The property 'height' is not defined for #<PersonHash:0x00...>

And I CAN access the defined attribute

{ **person, height: '5 feet' }.name
# => "Nic"

As a workaround, If I call to_h on the instance of PersonHash before creating the new hash, it resolves the issue, and I can access the attribute through the hash syntax

{ **person.to_h, height: '5 feet' }[:height]
# => "5 feet"

I am not quite sure what my opinion on the correct way for this to work is. If I try to merge a hash with a Hashie::Dash object, it will fail if I merge it with attributes that the Dash doesn't have defined -- This behavior makes sense to me. If I double splat when creating a new hash, I suppose I just expected the resulting hash to be a new Hash object not a Hashie::Dash object.

Nic

@dblock
Copy link
Member

dblock commented Mar 10, 2016

Interesting, I am not sure what this should do either, labelling this as discuss ;)

@boie0025
Copy link
Author

@dblock I was mucking around with it a bit more, this seems to be an artifact of the ** behavior. If I reverse the order: { height: '5 feet', **person} it is a Hash, and works as I had expected. This behavior is the same with other objects that inherit from Hash as well. Maybe just a short note in docs mentioning the double splat behavior would be enough? I'm still not a fan of what I found mentioned above (where it adds the hash entries, but won't allow access).

@dblock
Copy link
Member

dblock commented Mar 14, 2016

I don't have strong options about this, a doc entry doesn't hurt.

@michaelherold
Copy link
Member

I ran into a similar issue when I was working on one of the issues I was working on yesterday (sadly, I cannot remember which). This behavior comes from the behavior of the double splat operator.

In Ruby, as opposed to C, the algorithm for double splat is this:

def double_splat(arg)
  if arg.is_a?(::Hash)
    arg
  else
    arg.to_hash
  end
end

Because all of the Hashie special Hashes have ::Hash in their inheritance chain, that is_a? check returns true and the Dash itself is yielded as the result.

So, what exactly happens when we use the double-splat operators within a Hash? We can look at the instruction sequence to figure that out:

puts RubyVM::InstructionSequence.compile("h = { hi: 'there' }; { **h }").disasm
#=> == disasm: #<ISeq:<compiled>@<compiled>>================================
#=> local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
#=> [ 1] h
#=> 0000 trace            1                                               (   1)
#=> 0002 putobject        :hi
#=> 0004 putstring        "there"
#=> 0006 newhash          2
#=> 0008 setlocal_OP__WC__0 3
#=> 0010 putspecialobject 1
#=> 0012 getlocal_OP__WC__0 3
#=> 0014 opt_send_without_block <callinfo!mid:core#hash_merge_kwd, argc:1, ARGS_SIMPLE>, <callcache>
#=> 0017 opt_send_without_block <callinfo!mid:dup, argc:0, ARGS_SIMPLE>, <callcache>
#=> 0020 leave

If we look at instruction 14, we see that when we use the double-splat within a hash (much like the spread operator in ES6), it is compiled with the core#hash_merge_kwd function. If we look at the definition of that function in vm.c, we see that it delegates to what is, essentially, a C version of Hash#merge.

The gist is that the Dash is duplicated and then enumerated through in C and merged with the other "keywords" in the hash. Again, since this happens in C, our Ruby code is not activated at all when we set the extra value(s) on the Dash.

Le sigh. There isn't much we can do about this, since it's all in the C code that makes up the Ruby VM. The only way to work around the problem is to make Dash not inherit from Hash. That would require, at minimum, a major version bump plus a large rewrite of the functionality.

I think for now, the best we can do is document the behavior, like the rest of the gotchas in Hashie.

I will write this up in a PR so we can close this issue. Sorry for the trouble, @boie0025!

michaelherold added a commit to michaelherold/hashie that referenced this issue Feb 7, 2018
Let this be a lesson, folks: don't subclass the Hash class!

For more information, see the following:
hashie#353 (comment)
@boie0025
Copy link
Author

boie0025 commented Feb 9, 2018

@michaelherold Thanks for explaining your findings, I appreciate the explanation and your work to write it up.

michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 23, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 23, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 23, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 24, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 30, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelherold added a commit to michaelherold/hashie that referenced this issue Oct 30, 2020
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie#353 (comment)
michaelzedwards added a commit to michaelzedwards/hashie that referenced this issue Feb 11, 2022
Let this be a lesson, folks: don't subclass the Hash class!

For more information, see the following:
hashie/hashie#353 (comment)
michaelzedwards added a commit to michaelzedwards/hashie that referenced this issue Feb 11, 2022
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: hashie/hashie#353 (comment)
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

3 participants