Skip to content

Commit bb8b5cd

Browse files
eneboenebo
committed
Most multi-assign ordering fixed
1 parent a27cdc1 commit bb8b5cd

File tree

1 file changed

+130
-28
lines changed

1 file changed

+130
-28
lines changed

‎src/main/java/org/jruby/prism/builder/IRBuilderPrism.java‎

Lines changed: 130 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.jruby.ir.operands.FrozenString;
3737
import org.jruby.ir.operands.Hash;
3838
import org.jruby.ir.operands.ImmutableLiteral;
39+
import org.jruby.ir.operands.Integer;
3940
import org.jruby.ir.operands.Label;
4041
import org.jruby.ir.operands.LocalVariable;
4142
import org.jruby.ir.operands.MutableString;
@@ -68,6 +69,7 @@
6869

6970
import java.math.BigInteger;
7071
import java.util.ArrayList;
72+
import java.util.HashMap;
7173
import java.util.HashSet;
7274
import java.util.List;
7375
import java.util.Map;
@@ -310,8 +312,9 @@ protected Operand build(Variable result, Node node) {
310312
return buildMissing((MissingNode) node);
311313
} else if (node instanceof ModuleNode) {
312314
return buildModule((ModuleNode) node);
313-
} else if (node instanceof MultiWriteNode) {
314-
return buildMultiWriteNode((MultiWriteNode) node);
315+
// MultiTargetNode handled a few places internally
316+
} else if (node instanceof MultiWriteNode multi) {
317+
return buildMultiWriteOrTargetNode(multi.lefts, multi.rest, multi.rights, multi.value);
315318
} else if (node instanceof NextNode) {
316319
return buildNext((NextNode) node);
317320
} else if (node instanceof NilNode) {
@@ -524,11 +527,17 @@ protected void buildAssignment(Node node, Operand rhsVal) {
524527
} else if (node instanceof InstanceVariableTargetNode) {
525528
addInstr(new PutFieldInstr(buildSelf(), ((InstanceVariableTargetNode) node).name, rhsVal));
526529
} else if (node instanceof MultiTargetNode) {
530+
var multi = (MultiTargetNode) node;
527531
Variable rhs = addResultInstr(new ToAryInstr(temp(), rhsVal));
528-
buildMultiAssignment(((MultiTargetNode) node).lefts, ((MultiTargetNode) node).rest, ((MultiTargetNode) node).rights, rhs);
529-
} else if (node instanceof MultiWriteNode) { // FIXME: Is this possible with Multitarget now existing?
530-
Variable rhs = addResultInstr(new ToAryInstr(temp(), rhsVal));
531-
buildMultiAssignment(((MultiWriteNode) node).lefts, ((MultiWriteNode) node).rest, ((MultiWriteNode) node).rights, rhs);
532+
Map<Node, Operand> reads = new HashMap<>();
533+
final List<Tuple<Node, ResultInstr>> assigns = new ArrayList<>(4);
534+
buildMultiAssignment(multi.lefts, multi.rest, multi.rights, rhs, reads, assigns);
535+
536+
for (Tuple<Node, ResultInstr> assign: assigns) {
537+
addInstr((Instr) assign.b);
538+
}
539+
540+
buildAssignment(reads, assigns);
532541
} else if (node instanceof RequiredParameterNode) {
533542
RequiredParameterNode variable = (RequiredParameterNode) node;
534543
copy(getLocalVariable(variable.name, 0), rhsVal);
@@ -1648,38 +1657,131 @@ private Operand buildModule(ModuleNode node) {
16481657
getLine(node), getEndLine(node));
16491658
}
16501659

1651-
private Operand buildMultiWriteNode(MultiWriteNode node) {
1652-
Variable value = getValueInTemporaryVariable(build(node.value));
1653-
Variable rhs = node.value instanceof ArrayNode ? value : addResultInstr(new ToAryInstr(temp(), value));
1660+
private Operand buildMultiWriteOrTargetNode(Node[] lefts, Node rest, Node[] rights, Object value) {
1661+
var values = temp();
1662+
Map<Node, Operand> reads = new HashMap<>();
1663+
final List<Tuple<Node, ResultInstr>> assigns = new ArrayList<>(4);
1664+
buildMultiAssignment(lefts, rest, rights, values, reads, assigns);
16541665

1655-
buildMultiAssignment(node.lefts, node.rest, node.rights, rhs);
1666+
Operand builtValue;
1667+
if (value instanceof Node node) {
1668+
builtValue = getValueInTemporaryVariable(build(node));
16561669

1657-
return value;
1670+
if (value instanceof ArrayNode) {
1671+
copy(values, builtValue);
1672+
} else {
1673+
addResultInstr(new ToAryInstr(values, builtValue));
1674+
}
1675+
} else {
1676+
builtValue = (Operand) value;
1677+
copy(values, builtValue);
1678+
}
1679+
1680+
for (Tuple<Node, ResultInstr> assign: assigns) {
1681+
addInstr((Instr) assign.b);
1682+
}
1683+
1684+
buildAssignment(reads, assigns);
1685+
1686+
return builtValue;
16581687
}
16591688

1660-
// SplatNode, MultiWriteNode, LocalVariableWrite and lots of other normal writes
1661-
private void buildMultiAssignment(Node[] pre, Node rest, Node[] post, Operand values) {
1662-
final List<Tuple<Node, Variable>> assigns = new ArrayList<>();
1689+
// This method is called to build assignments for a multiple-assignment instruction
1690+
protected void buildAssignment(Map<Node, Operand> reads, List<Tuple<Node, ResultInstr>> assigns) {
1691+
1692+
for (var assign: assigns) {
1693+
var node = assign.a;
1694+
var rhs = assign.b.getResult();
1695+
1696+
switch (node) {
1697+
case CallTargetNode call -> buildAttrAssignAssignment(call.receiver, call.name, Node.EMPTY_ARRAY, rhs);
1698+
case IndexTargetNode index -> {
1699+
var receiver = reads.get(index.receiver);
1700+
Array holders = (Array) reads.get(index.arguments);
1701+
int flags = ((Integer) holders.get(holders.size() - 1)).value;
1702+
Operand[] args = new Operand[holders.size() - 1];
1703+
System.arraycopy(holders.getElts(), 0, args, 0, args.length);
1704+
args = addArg(args, rhs);
1705+
addInstr(AttrAssignInstr.create(scope, receiver, symbol("[]="), args, flags, scope.maybeUsingRefinements()));
1706+
}
1707+
case ClassVariableTargetNode cvar -> addInstr(new PutClassVariableInstr(classVarDefinitionContainer(), cvar.name, rhs));
1708+
case ConstantPathTargetNode constpath -> putConstant(reads.get(constpath), constpath.name, rhs);
1709+
case ConstantTargetNode consty -> addInstr(new PutConstInstr(getCurrentModuleVariable(), consty.name, rhs));
1710+
case LocalVariableTargetNode lvar -> copy(getLocalVariable(lvar.name, lvar.depth), rhs);
1711+
case GlobalVariableTargetNode gvar -> addInstr(new PutGlobalVarInstr(gvar.name, rhs));
1712+
case InstanceVariableTargetNode ivar -> addInstr(new PutFieldInstr(buildSelf(), ivar.name, rhs));
1713+
case MultiTargetNode _m -> { } // handled in processReads()
1714+
case RequiredParameterNode req -> copy(getLocalVariable(req.name, 0), rhs);
1715+
case ImplicitRestNode _r -> { } // This is assigned to nothing
1716+
case SplatNode _r -> { } // This is splat where expression is null
1717+
default -> throw notCompilable("Can't build assignment node", node);
1718+
}
1719+
}
1720+
}
16631721

1664-
for (int i = 0; i < pre.length; i++) {
1665-
assigns.add(new Tuple<>(pre[i], addResultInstr(new ReqdArgMultipleAsgnInstr(temp(), values, i))));
1722+
// SplatNode, MultiWriteNode, LocalVariableWrite and lots of other normal writes
1723+
private void buildMultiAssignment(Node[] pre, Node rest, Node[] post, Operand values,
1724+
Map<Node, Operand> reads, List<Tuple<Node, ResultInstr>> assigns) {
1725+
int i = 0;
1726+
for (var an: pre) {
1727+
var get = new ReqdArgMultipleAsgnInstr(temp(), values, i);
1728+
assigns.add(new Tuple<>(an, get));
1729+
processReads(get.getResult(), assigns, reads, an);
1730+
i++;
16661731
}
16671732

16681733
if (rest != null) {
16691734
if (rest instanceof SplatNode) {
16701735
Node realTarget = ((SplatNode) rest).expression;
1671-
assigns.add(new Tuple<>(realTarget, addResultInstr(new RestArgMultipleAsgnInstr(temp(), values, 0, pre.length, post.length))));
1736+
var get = new RestArgMultipleAsgnInstr(temp(), values, 0, pre.length, post.length);
1737+
if (realTarget == null) {
1738+
assigns.add(new Tuple<>(rest, get));
1739+
} else {
1740+
assigns.add(new Tuple<>(realTarget, get));
1741+
}
1742+
processReads(get.getResult(), assigns, reads, rest);
16721743
} else {
1673-
assigns.add(new Tuple<>(null, addResultInstr(new RestArgMultipleAsgnInstr(temp(), values, 0, pre.length, post.length))));
1744+
var get = new RestArgMultipleAsgnInstr(temp(), values, 0, pre.length, post.length);
1745+
assigns.add(new Tuple<>(rest, get));
1746+
processReads(get.getResult(), assigns, reads, rest);
16741747
}
16751748
}
16761749

1677-
for (int j = 0; j < post.length; j++) {
1678-
assigns.add(new Tuple<>(post[j], addResultInstr(new ReqdArgMultipleAsgnInstr(temp(), values, j, pre.length, post.length))));
1750+
int j = 0;
1751+
for (var an: post) {
1752+
var get = new ReqdArgMultipleAsgnInstr(temp(), values, j, i, post.length);
1753+
assigns.add(new Tuple<>(an, get));
1754+
processReads(get.getResult(), assigns, reads, an);
1755+
j++;
16791756
}
1757+
}
16801758

1681-
for (Tuple<Node, Variable> assign: assigns) {
1682-
buildAssignment(assign.a, assign.b);
1759+
private void processReads(Variable result, List<Tuple<Node, ResultInstr>> assigns, Map<Node, Operand> reads,
1760+
Node node) {
1761+
switch(node) {
1762+
case IndexTargetNode index:
1763+
reads.put(index.receiver, build(index.receiver));
1764+
Node[] arguments = index.arguments == null ? Node.EMPTY_ARRAY : index.arguments.arguments;
1765+
int[] flags = new int[] { 0 };
1766+
Operand[] args = buildCallArgsArray(arguments, flags);
1767+
Operand[] hackyArgs = new Operand[args.length + 1];
1768+
System.arraycopy(args, 0, hackyArgs, 0, args.length);
1769+
hackyArgs[args.length] = new Integer(flags[0]);
1770+
reads.put(index.arguments, new Array(hackyArgs));
1771+
break;
1772+
case ClassVariableTargetNode cvar:
1773+
reads.put(node, classVarDefinitionContainer());
1774+
break;
1775+
case ConstantPathTargetNode constpath:
1776+
reads.put(node, buildModuleParent(((ConstantPathTargetNode) node).parent));
1777+
break;
1778+
case MultiTargetNode multi:
1779+
var subRet = temp();
1780+
assigns.add(new Tuple<>(multi, new ToAryInstr(subRet, result)));
1781+
buildMultiAssignment(multi.lefts, multi.rest, multi.rights, subRet, reads, assigns);
1782+
break;
1783+
default:
1784+
break;
16831785
}
16841786
}
16851787

@@ -1827,7 +1929,7 @@ private Operand buildRational(RationalNode node) {
18271929
}
18281930

18291931
private ImmutableLiteral asRationalValue(Object value) {
1830-
return value instanceof Integer ? new Fixnum((int) value) :
1932+
return value instanceof java.lang.Integer ? new Fixnum((int) value) :
18311933
value instanceof Long ? new Fixnum((long) value) :
18321934
new Bignum((BigInteger) value);
18331935
}
@@ -2176,12 +2278,12 @@ public void receivePreArg(Node node, Variable keywords, int argIndex) {
21762278
addArgumentDescription(ArgumentType.req, name);
21772279

21782280
addInstr(new ReceivePreReqdArgInstr(argumentResult(name), keywords, argIndex));
2179-
} else if (node instanceof MultiTargetNode) { // methods
2281+
} else if (node instanceof MultiTargetNode multi) { // methods
21802282
Variable v = temp();
21812283
addInstr(new ReceivePreReqdArgInstr(v, keywords, argIndex));
21822284
addArgumentDescription(ArgumentType.anonreq, null);
21832285
Variable rhs = addResultInstr(new ToAryInstr(temp(), v));
2184-
buildMultiAssignment(((MultiTargetNode) node).lefts, ((MultiTargetNode) node).rest, ((MultiTargetNode) node).rights, rhs);
2286+
buildMultiWriteOrTargetNode(multi.lefts, multi.rest, multi.rights, rhs);
21852287
} else if (node instanceof ClassVariableTargetNode) { // blocks/for
21862288
Variable v = temp();
21872289
addInstr(new ReceivePreReqdArgInstr(v, keywords, argIndex));
@@ -2231,7 +2333,7 @@ public void receivePostArg(Node node, Variable keywords, int argIndex, int preCo
22312333
addArgumentDescription(ArgumentType.req, argName);
22322334

22332335
addInstr(new ReceivePostReqdArgInstr(argumentResult(argName), keywords, argIndex, preCount, optCount, hasRest, postCount));
2234-
} else if (node instanceof MultiTargetNode) {
2336+
} else if (node instanceof MultiTargetNode multi) {
22352337
Variable v = temp();
22362338
addInstr(new ReceivePostReqdArgInstr(v, keywords, argIndex, preCount, optCount, hasRest, postCount));
22372339

@@ -2240,7 +2342,7 @@ public void receivePostArg(Node node, Variable keywords, int argIndex, int preCo
22402342
Variable tmp = temp();
22412343
addInstr(new ToAryInstr(tmp, v));
22422344

2243-
buildMultiAssignment(((MultiTargetNode) node).lefts, ((MultiTargetNode) node).rest, ((MultiTargetNode) node).rights, tmp);
2345+
buildMultiWriteOrTargetNode(multi.lefts, multi.rest, multi.rights, tmp);
22442346
} else {
22452347
throw notCompilable("Can't build required parameter node", node);
22462348
}
@@ -2268,7 +2370,7 @@ private Operand buildStatements(StatementsNode node) {
22682370

22692371
@Override
22702372
protected void buildWhenArgs(WhenNode whenNode, Operand testValue, Label bodyLabel,
2271-
Set<IRubyObject> seenLiterals, Map<IRubyObject, Integer> origLocs) {
2373+
Set<IRubyObject> seenLiterals, Map<IRubyObject, java.lang.Integer> origLocs) {
22722374
Variable eqqResult = temp();
22732375
Node[] exprNodes = whenNode.conditions;
22742376

0 commit comments

Comments
 (0)