Avoid executing arg nodes multiple times with op element assignment.

Author: aardvark179

CHANGELOG.md

@@ -41,6 +41,7 @@ Bug fixes:
 * Fix `/#{...}/o` to evaluate only once per context when splitting happens (@eregon).
 * Fix `Kernel#sprintf` formatting of floats to be like CRuby (@aardvark179).
 * Fix `Process.egid=` to accept `String`s (#2615, @ngtban)
+* Fix optional assignment to only evaluate index arguments once (#2658, @aardvark179).
 
 Compatibility:
 

src/main/java/org/truffleruby/parser/BodyTranslator.java

@@ -14,6 +14,7 @@
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Deque;
 import java.util.Iterator;
 import java.util.List;
@@ -2469,58 +2470,87 @@ public RubyNode visitOpElementAsgnNode(OpElementAsgnParseNode node) {
                 tempName,
                 0,
                 node.getReceiverNode());
+        final ArrayList<ValueFromNode> argValues = argsToTemp(node);
 
         final String op = node.getOperatorName();
         final boolean logicalOperation = op.equals("&&") || op.equals("||");
 
         if (logicalOperation) {
-            final ParseNode write = write(node, readReceiverFromTemp, value);
-            final ParseNode operation = operation(node, readReceiverFromTemp, op, write);
+            final ParseNode write = write(node, readReceiverFromTemp, argValues, value);
+            final ParseNode operation = operation(node, readReceiverFromTemp, argValues, op, write);
 
-            return block(node, writeReceiverToTemp, operation);
+            return block(node, writeReceiverToTemp, argValues, operation);
         } else {
-            final ParseNode operation = operation(node, readReceiverFromTemp, op, value);
-            final ParseNode write = write(node, readReceiverFromTemp, operation);
+            final ParseNode operation = operation(node, readReceiverFromTemp, argValues, op, value);
+            final ParseNode write = write(node, readReceiverFromTemp, argValues, operation);
 
-            return block(node, writeReceiverToTemp, write);
+            return block(node, writeReceiverToTemp, argValues, write);
         }
     }
 
-    private RubyNode block(OpElementAsgnParseNode node, ParseNode writeReceiverToTemp, ParseNode main) {
+    private RubyNode block(OpElementAsgnParseNode node, ParseNode writeReceiverToTemp,
+            ArrayList<ValueFromNode> argValues, ParseNode main) {
         final BlockParseNode block = new BlockParseNode(node.getPosition());
         block.add(writeReceiverToTemp);
         block.add(main);
 
-        final RubyNode ret = block.accept(this);
+        RubyNode ret = block.accept(this);
+        Collections.reverse(argValues);
+        for (var arg : argValues) {
+            ret = arg.prepareAndThen(node.getPosition(), ret);
+        }
         return addNewlineIfNeeded(node, ret);
     }
 
-    private ParseNode write(OpElementAsgnParseNode node, ParseNode readReceiverFromTemp, ParseNode value) {
-        final ParseNode readArguments = node.getArgsNode();
+    private ParseNode write(OpElementAsgnParseNode node, ParseNode readReceiverFromTemp,
+            ArrayList<ValueFromNode> argValues, ParseNode value) {
         final ParseNode writeArguments;
         // Like ParserSupport#arg_add, but copy the first node
-        if (readArguments instanceof ArrayParseNode) {
+        if (node.getArgsNode() instanceof ArrayParseNode) {
             final ArrayParseNode readArgsCopy = new ArrayParseNode(node.getPosition());
-            readArgsCopy.addAll((ArrayParseNode) readArguments).add(value);
+            for (var arg : argValues) {
+                readArgsCopy.add(arg.get(node.getPosition()));
+            }
+            readArgsCopy.add(value);
             writeArguments = readArgsCopy;
         } else {
-            writeArguments = new ArgsPushParseNode(node.getPosition(), readArguments, value);
+            writeArguments = new ArgsPushParseNode(node.getPosition(), argValues.get(0).get(node.getPosition()), value);
         }
 
         return new AttrAssignParseNode(node.getPosition(), readReceiverFromTemp, "[]=", writeArguments, false);
     }
 
+    private ArrayList<ValueFromNode> argsToTemp(OpElementAsgnParseNode node) {
+        ArrayList<ValueFromNode> argValues = new ArrayList<>();
+
+        final ParseNode readArguments = node.getArgsNode();
+        if (readArguments instanceof ArrayParseNode) {
+            for (ParseNode child : ((ArrayParseNode) readArguments).children()) {
+                argValues.add(ValueFromNode.valueFromNode(this, child));
+            }
+        } else {
+            argValues.add(ValueFromNode.valueFromNode(this, readArguments));
+        }
+
+        return argValues;
+    }
+
     private ParseNode operation(
             OpElementAsgnParseNode node,
             ParseNode readReceiverFromTemp,
+            ArrayList<ValueFromNode> argValues,
             String op,
             ParseNode right) {
+        final ArrayParseNode readArguments = new ArrayParseNode(node.getPosition());
+        for (var arg : argValues) {
+            readArguments.add(arg.get(node.getPosition()));
+        }
 
         final ParseNode read = new CallParseNode(
                 node.getPosition(),
                 readReceiverFromTemp,
                 "[]",
-                node.getArgsNode(),
+                readArguments,
                 null);
         ParseNode operation;
         switch (op) {

src/main/java/org/truffleruby/parser/ValueFromNode.java

@@ -15,6 +15,7 @@
 import org.truffleruby.parser.ast.LocalVarParseNode;
 import org.truffleruby.parser.ast.ParseNode;
 import org.truffleruby.parser.ast.SelfParseNode;
+import org.truffleruby.parser.ast.SplatParseNode;
 
 import java.util.Arrays;
 
@@ -80,9 +81,30 @@ public ParseNode get(SourceIndexLength sourceSection) {
 
     }
 
+    class ValueFromSplatNode implements ValueFromNode {
+
+        private final ValueFromNode value;
+
+        public ValueFromSplatNode(BodyTranslator translator, SplatParseNode node) {
+            value = valueFromNode(translator, node.getValue());
+        }
+
+        @Override
+        public RubyNode prepareAndThen(SourceIndexLength sourceSection, RubyNode subsequent) {
+            return value.prepareAndThen(sourceSection, subsequent);
+        }
+
+        @Override
+        public ParseNode get(SourceIndexLength sourceSection) {
+            return new SplatParseNode(sourceSection, value.get(sourceSection));
+        }
+    }
+
     static ValueFromNode valueFromNode(BodyTranslator translator, ParseNode node) {
         if (node instanceof SelfParseNode) {
             return new ValueFromSelfNode();
+        } else if (node instanceof SplatParseNode) {
+            return new ValueFromSplatNode(translator, (SplatParseNode) node);
         } else {
             return new ValueFromEffectNode(translator, node);
         }