JavaScript: for each evaluates its target before reading the value

A for each loop read the property's value before evaluating its target,
so a target such as (o.p = 2, t).v, t[(o.p = 3, 'w')] or a bare
@[(o.p = 2, 'k')] stored the value from before the target ran. The
target's object and name are now evaluated first and the value is read
after them (ECMA-357 section 12.3).

crates/js/src/bytecode.rs

@@ -173,8 +173,9 @@ pub(crate) enum Op {
     XmlFilterTest,
     /// `state` → the XMLList of the items kept.
     XmlFilterEnd,
-    /// `iterator name` → `iterator value`: the value under the name a `for each` got.
-    ForEachValue,
+    /// `iterator name x₁…xₙ` → `iterator name x₁…xₙ value`: the value under the name a `for each`
+    /// got, read after the `n` values its target pushed.
+    ForEachValue(u32),
     /// `value` → nothing: sets the default XML namespace of the running code.
     SetDefaultNamespace,
 }
@@ -312,7 +313,7 @@ fn effect(op: Op, finally_tables: &[Vec<u32>]) -> Result<Effect, String> {
         Op::XmlEscape(_) | Op::XmlScopeGet | Op::XmlFilterStart | Op::XmlFilterEnd => e(1, 1),
         Op::XmlLiteral { parts, .. } => e(parts, 1),
         Op::XmlScopeRef => e(1, 2),
-        Op::ForEachValue => e(2, 2),
+        Op::ForEachValue(n) => e(n + 2, n + 3),
         Op::SetDefaultNamespace => e(1, 0),
     })
 }

crates/js/src/compiler.rs

@@ -497,16 +497,21 @@ impl<'a> FnCompiler<'a> {
         let brk = self.new_label();
         self.bind(head);
         self.jump(Op::ForInNext, brk);
-        if each {
-            self.emit(Op::ForEachValue);
-        }
         match target {
             ForInTarget::Var(d) => {
                 let n = self.var_name(&d.name.name);
+                if each {
+                    self.emit(Op::ForEachValue(0));
+                }
                 self.emit(Op::SetVar(n));
             }
+            ForInTarget::Expr(e) if each => self.store_value_to(*e)?,
             ForInTarget::Expr(e) => self.store_key_to(*e)?,
         }
+        if each {
+            // The value; the name under it goes next.
+            self.emit(Op::Pop);
+        }
         self.emit(Op::Pop);
         self.loop_body(body, labels, brk, head)?;
         self.jump(Op::Jump, head);
@@ -552,6 +557,46 @@ impl<'a> FnCompiler<'a> {
         Ok(())
     }
 
+    /// Assigns the value under the name on top of the stack to a `for each` target expression,
+    /// leaving the name and then the value on top. The target is evaluated before the value is
+    /// read (ECMA-357 section 12.3).
+    fn store_value_to(&mut self, target: ExprId) -> CResult<()> {
+        let target = self.ast.unparenthesized(target);
+        self.at(self.ast.expr_span(target).start);
+        match self.ast.expr(target) {
+            Some(Expr::Ident(name)) => {
+                let n = self.var_name(name);
+                self.emit(Op::ForEachValue(0));
+                self.emit(Op::SetVar(n));
+            }
+            Some(Expr::Member { object, property }) => {
+                self.expr(*object)?;
+                self.emit(Op::ForEachValue(1));
+                let n = self.name_key(key_of(&property.name));
+                self.emit(Op::SetMember(n));
+            }
+            Some(Expr::Index { object, index }) => {
+                self.expr(*object)?;
+                self.expr(*index)?;
+                self.emit(Op::ToKey);
+                self.emit(Op::ForEachValue(2));
+                self.emit(Op::SetIndex);
+            }
+            Some(Expr::XmlScopeName(name)) => {
+                self.xml_scope_ref(target, *name)?;
+                self.emit(Op::ForEachValue(2));
+                self.emit(Op::SetIndex);
+            }
+            _ => {
+                self.expr(target)?;
+                self.emit(Op::Pop);
+                self.emit(Op::ForEachValue(0));
+                self.throw_invalid_target();
+            }
+        }
+        Ok(())
+    }
+
     fn throw_invalid_target(&mut self) {
         let s = self.string(&"invalid assignment target".encode_utf16().collect::<Vec<u16>>());
         let depth = self.depth;

crates/js/src/interp.rs

@@ -927,7 +927,7 @@ impl Interp {
             Op::XmlFilterNext(t) => self.op_xml_filter_next(t),
             Op::XmlFilterTest => self.op_xml_filter_test(),
             Op::XmlFilterEnd => self.op_xml_filter_end(),
-            Op::ForEachValue => self.op_for_each_value(),
+            Op::ForEachValue(n) => self.op_for_each_value(n as usize),
             Op::SetDefaultNamespace => self.op_set_default_namespace(),
             Op::Return | Op::ReturnCompletion => internal("return reached the dispatcher"),
         }

crates/js/src/xml.rs

@@ -2895,9 +2895,9 @@ impl Interp {
         self.replace(1, Value::Object(out))
     }
 
-    pub(crate) fn op_for_each_value(&mut self) -> JsResult<()> {
-        let name = self.peek(0)?;
-        let Value::Object(iter) = self.peek(1)? else { return Err(Abrupt::Internal("for-in iterator is not an object".into())) };
+    pub(crate) fn op_for_each_value(&mut self, above: usize) -> JsResult<()> {
+        let name = self.peek(above)?;
+        let Value::Object(iter) = self.peek(above + 1)? else { return Err(Abrupt::Internal("for-in iterator is not an object".into())) };
         let object = match self.heap.object(iter).map(|o| &o.class) {
             Some(Class::ForIn(state)) => state.object,
             _ => return Err(Abrupt::Internal("not a for-in iterator".into())),
@@ -2909,7 +2909,7 @@ impl Interp {
             }
             None => Value::Undefined,
         };
-        self.replace(1, v)
+        self.push(v)
     }
 
     /// The URI of a Namespace value or of its string form.

crates/js/src/xml_tests.rs

@@ -716,6 +716,14 @@ fn bare_scope_for_targets_assign_each_iteration() {
     assert_eq!(thrown_name("with (<a/>) { for (@k in { p: 1 }); }"), "ReferenceError");
 }
 
+#[test]
+fn for_each_evaluates_its_target_before_reading_the_value() {
+    truth("var o = { p: 1 }; var t = {}; for each ((o.p = 2, t).v in o); t.v === 2");
+    truth("var o = { p: 1 }; var t = {}; for each (t[(o.p = 3, 'w')] in o); t.w === 3");
+    truth("var x = <a k=''/>; var o = { p: 1 }; with (x) { for each (@[(o.p = 2, 'k')] in o); } x.@k == '2'");
+    truth("var o = { p: 1, q: 2 }; var s = ''; for each (var v in o) s += v; var t; for each (t in o) s += t; s === '1212' && t === 2");
+}
+
 #[test]
 fn bare_scope_targets_need_e4x_option() {
     for src in ["@a = 1", "ns::a = 1", "@a += 1", "++@a", "@a--", "for (@a in o);", "for each (@a in o);", "\"use strict\"; @a = 1"] {