-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: this would not be fine with the @ExplodeLoop
above I think
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iterateKeyValues will already assert, so not really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a branch profile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better and simpler!
Please fix my two comments.
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() == null
statement 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_kwd
https://github.com/ruby/ruby/blob/22c0994bc8b34e4b8b80de3259363b0f27c24988/vm.c#L2675