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

Comments

Projects
None yet
3 participants
@boie0025

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 10, 2016

Contributor

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

Contributor

dblock commented Mar 10, 2016

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

@boie0025

This comment has been minimized.

Show comment
Hide comment
@boie0025

boie0025 Mar 14, 2016

@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).

boie0025 commented Mar 14, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 14, 2016

Contributor

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

Contributor

dblock commented Mar 14, 2016

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

@michaelherold

This comment has been minimized.

Show comment
Hide comment
@michaelherold

michaelherold Feb 6, 2018

Contributor

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!

Contributor

michaelherold commented Feb 6, 2018

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

Document Dash double-splat operator gotchas
Let this be a lesson, folks: don't subclass the Hash class!

For more information, see the following:
intridea#353 (comment)

@dblock dblock closed this in #440 Feb 7, 2018

@boie0025

This comment has been minimized.

Show comment
Hide comment
@boie0025

boie0025 Feb 9, 2018

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

boie0025 commented Feb 9, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment