[Truffle] Hash merge check for splatted elements #4347
Conversation
|
Looks good. |
| @@ -1826,14 +1826,17 @@ public RubyNode visitHashNode(HashParseNode node) { | |||
| final SourceSection fullSourceSection = sourceSection.toSourceSection(source); | |||
|
|
|||
| final List<RubyNode> hashConcats = new ArrayList<>(); | |||
| final List<Boolean> hashConcatsIsMerge = new ArrayList<>(); | |||
|
|
|||
| final List<RubyNode> keyValues = new ArrayList<>(); | |||
|
|
|||
| for (KeyValuePair<ParseNode, ParseNode> pair: node.getPairs()) { | |||
| if (pair.getKey() == null) { | |||
There was a problem hiding this comment.
This case with a null key means that we have a ** splat like in {a: 1, **{b: 2}, c: 3}.
It would merit a comment for the least!
| hashConcats.add(HashCastNodeGen.create(context, fullSourceSection, pair.getValue().accept(this))); | ||
| hashConcatsIsMerge.add(true); |
There was a problem hiding this comment.
Why such a complex approach rather than introducing a node above the HashCastNode here that checks keys are all Symbols like EnsureSymbolKeysNode ?
Then that would be a single line change here, and no change in ConcatHashLiteralNode.
Don't be afraid to add new nodes.
There was a problem hiding this comment.
I'll try refactoring to use a new node. I think it would match the behavior I am looking for.
There was a problem hiding this comment.
Since true is only in this specific case (for **expr) then the wrapper node can be added just here.
BTW this check is also needed for the more dynamic cases like
[3] pry(main)> c = {a:3, "c" => 42}
=> {:a=>3, "c"=>42}
[4] pry(main)> {b:1, **c}
TypeError: wrong argument type String (expected Symbol)and
[7] pry(main)> foo(a:1, **c)
TypeError: wrong argument type String (expected Symbol)but maybe these cases are already encoded as HashParseNode.
There was a problem hiding this comment.
@eregon I've pushed an update to use a wrapper node.
| @@ -54,6 +59,13 @@ private Object buildHash(DynamicObject[] parts) { | |||
| for (int i = 0; i < children.length; i++) { | |||
| try { | |||
| DynamicObject hash = children[i].executeDynamicObject(frame); | |||
| if (mergeChildren[i]) { | |||
| for (KeyValue keyValue : HashOperations.iterableKeyValues(hash)) { | |||
There was a problem hiding this comment.
Just a note: this would not be fine with the @ExplodeLoop above I think
bjfish
force-pushed
the
truffle-hash-merge-check
branch
from
6bf398b
to
d13be1d
Compare
| @Override | ||
| public Object execute(VirtualFrame frame) { | ||
| final Object hash = child.execute(frame); | ||
| assert RubyGuards.isRubyHash(hash); |
There was a problem hiding this comment.
iterateKeyValues will already assert, so not really necessary.
There was a problem hiding this comment.
I've removed the assert
| assert RubyGuards.isRubyHash(hash); | ||
| for (KeyValue keyValue : HashOperations.iterableKeyValues((DynamicObject) hash)) { | ||
| if (!RubyGuards.isRubySymbol(keyValue.getKey())) { | ||
| throw new RaiseException(coreExceptions().typeErrorWrongArgumentType(keyValue.getKey(), "Symbol", this)); |
There was a problem hiding this comment.
You want a BranchProfile here before the throw so that part is not compiled until we actually meet a non-Symbol.
The BranchProfile should be a final field of the node.
There was a problem hiding this comment.
I've added a branch profile
bjfish
force-pushed
the
truffle-hash-merge-check
branch
from
d13be1d
to
ad2bfbc
Compare
This should raise a type error:
As far as I can tell, this ConcatHashLiteralNode is used for merging hashes with the splat
**operator. I observed that the pair matching thepair.getKey() == nullstatement in the body translator is what is to be merged into the hash literal. These null key pairs need to check that all their keys are symbols after they are executed. So, I added an array of booleans to track which hashes were being merged in to pass to the concat node.This is done here in MRI:
core_hash_merge_kwdhttps://github.com/ruby/ruby/blob/22c0994bc8b34e4b8b80de3259363b0f27c24988/vm.c#L2675