fix expression list order

don't emit a no-op when unpacking a single element

assume positional args stored as tuple in extended call

compiler/codegen/src/compile.rs

@@ -2430,7 +2430,7 @@ impl Compiler<'_> {
             Expr::List(ExprList { elts, .. }) => {
                 let (size, unpack) = self.gather_elements(0, elts)?;
                 if unpack {
-                    emit!(self, Instruction::BuildListUnpack { size });
+                    emit!(self, Instruction::BuildListFromTuples { size });
                 } else {
                     emit!(self, Instruction::BuildList { size });
                 }
@@ -2438,7 +2438,9 @@ impl Compiler<'_> {
             Expr::Tuple(ExprTuple { elts, .. }) => {
                 let (size, unpack) = self.gather_elements(0, elts)?;
                 if unpack {
-                    emit!(self, Instruction::BuildTupleUnpack { size });
+                    if size > 1 {
+                        emit!(self, Instruction::BuildTupleFromTuples { size });
+                    }
                 } else {
                     emit!(self, Instruction::BuildTuple { size });
                 }
@@ -2446,7 +2448,7 @@ impl Compiler<'_> {
             Expr::Set(ExprSet { elts, .. }) => {
                 let (size, unpack) = self.gather_elements(0, elts)?;
                 if unpack {
-                    emit!(self, Instruction::BuildSetUnpack { size });
+                    emit!(self, Instruction::BuildSetFromTuples { size });
                 } else {
                     emit!(self, Instruction::BuildSet { size });
                 }
@@ -2819,7 +2821,7 @@ impl Compiler<'_> {
         let call = if unpack || has_double_star {
             // Create a tuple with positional args:
             if unpack {
-                emit!(self, Instruction::BuildTupleUnpack { size });
+                emit!(self, Instruction::BuildTupleFromTuples { size });
             } else {
                 emit!(self, Instruction::BuildTuple { size });
             }
@@ -2863,34 +2865,31 @@ impl Compiler<'_> {
 
         let size = if has_stars {
             let mut size = 0;
+            let mut iter = elements.iter().peekable();
+            let mut run_size = before;
 
-            if before > 0 {
-                emit!(self, Instruction::BuildTuple { size: before });
-                size += 1;
-            }
-
-            let groups = elements
-                .iter()
-                .map(|element| {
-                    if let Expr::Starred(ExprStarred { value, .. }) = &element {
-                        (true, value.as_ref())
-                    } else {
-                        (false, element)
-                    }
-                })
-                .chunk_by(|(starred, _)| *starred);
-
-            for (starred, run) in &groups {
-                let mut run_size = 0;
-                for (_, value) in run {
-                    self.compile_expression(value)?;
-                    run_size += 1
-                }
-                if starred {
-                    size += run_size
-                } else {
+            loop {
+                if iter.peek().is_none_or(|e| matches!(e, Expr::Starred(_))) {
                     emit!(self, Instruction::BuildTuple { size: run_size });
-                    size += 1
+                    run_size = 0;
+                    size += 1;
+                }
+
+                match iter.next() {
+                    Some(Expr::Starred(ExprStarred { value, .. })) => {
+                        self.compile_expression(value)?;
+                        // We need to collect each unpacked element into a
+                        // tuple, since any side-effects during the conversion
+                        // should be made visible before evaluating remaining
+                        // expressions.
+                        emit!(self, Instruction::BuildTupleFromIter);
+                        size += 1;
+                    }
+                    Some(element) => {
+                        self.compile_expression(element)?;
+                        run_size += 1;
+                    }
+                    None => break,
                 }
             }
 

compiler/core/src/bytecode.rs

@@ -542,19 +542,20 @@ pub enum Instruction {
     BuildTuple {
         size: Arg<u32>,
     },
-    BuildTupleUnpack {
+    BuildTupleFromTuples {
         size: Arg<u32>,
     },
+    BuildTupleFromIter,
     BuildList {
         size: Arg<u32>,
     },
-    BuildListUnpack {
+    BuildListFromTuples {
         size: Arg<u32>,
     },
     BuildSet {
         size: Arg<u32>,
     },
-    BuildSetUnpack {
+    BuildSetFromTuples {
         size: Arg<u32>,
     },
     BuildMap {
@@ -1260,11 +1261,12 @@ impl Instruction {
             Raise { kind } => -(kind.get(arg) as u8 as i32),
             BuildString { size }
             | BuildTuple { size, .. }
-            | BuildTupleUnpack { size, .. }
+            | BuildTupleFromTuples { size, .. }
             | BuildList { size, .. }
-            | BuildListUnpack { size, .. }
+            | BuildListFromTuples { size, .. }
             | BuildSet { size, .. }
-            | BuildSetUnpack { size, .. } => -(size.get(arg) as i32) + 1,
+            | BuildSetFromTuples { size, .. } => -(size.get(arg) as i32) + 1,
+            BuildTupleFromIter => 0,
             BuildMap { size } => {
                 let nargs = size.get(arg) * 2;
                 -(nargs as i32) + 1
@@ -1447,13 +1449,14 @@ impl Instruction {
             Raise { kind } => w!(Raise, ?kind),
             BuildString { size } => w!(BuildString, size),
             BuildTuple { size } => w!(BuildTuple, size),
-            BuildTupleUnpack { size } => w!(BuildTupleUnpack, size),
+            BuildTupleFromTuples { size } => w!(BuildTupleFromTuples, size),
+            BuildTupleFromIter => w!(BuildTupleFromIter),
             BuildList { size } => w!(BuildList, size),
-            BuildListUnpack { size } => w!(BuildListUnpack, size),
+            BuildListFromTuples { size } => w!(BuildListFromTuples, size),
             BuildSet { size } => w!(BuildSet, size),
-            BuildSetUnpack { size } => w!(BuildSetUnpack, size),
+            BuildSetFromTuples { size } => w!(BuildSetFromTuples, size),
             BuildMap { size } => w!(BuildMap, size),
-            BuildMapForCall { size } => w!(BuildMap, size),
+            BuildMapForCall { size } => w!(BuildMapForCall, size),
             DictUpdate => w!(DictUpdate),
             BuildSlice { step } => w!(BuildSlice, step),
             ListAppend { i } => w!(ListAppend, i),

extra_tests/snippets/builtin_list.py

@@ -636,6 +636,27 @@ assert it.__length_hint__() == 0
 a = [*[1, 2], 3, *[4, 5]]
 assert a == [1, 2, 3, 4, 5]
 
+# Test for list unpacking evaluation order (https://github.com/RustPython/RustPython/issues/5566)
+a = [1, 2]
+b = [a.append(3), *a, a.append(4), *a]
+assert a == [1, 2, 3, 4]
+assert b == [None, 1, 2, 3, None, 1, 2, 3, 4]
+
+for base in object, list, tuple:
+    # do not assume that a type inherited from some sequence type behaves like
+    # that sequence type
+    class C(base):
+        def __iter__(self):
+            a.append(2)
+            def inner():
+                yield 3
+                a.append(4)
+            return inner()
+
+    a = [1]
+    b = [*a, *C(), *a.copy()]
+    assert b == [1, 3, 1, 2, 4]
+
 # Test for list entering daedlock or not (https://github.com/RustPython/RustPython/pull/2933)
 class MutatingCompare:
     def __eq__(self, other):

vm/src/frame.rs

@@ -710,27 +710,28 @@ impl ExecutingFrame<'_> {
                 self.push_value(list_obj.into());
                 Ok(None)
             }
-            bytecode::Instruction::BuildListUnpack { size } => {
-                let elements = self.unpack_elements(vm, size.get(arg) as usize)?;
+            bytecode::Instruction::BuildListFromTuples { size } => {
+                // SAFETY: compiler guarantees `size` tuples are on the stack
+                let elements = unsafe { self.flatten_tuples(size.get(arg) as usize) };
                 let list_obj = vm.ctx.new_list(elements);
                 self.push_value(list_obj.into());
                 Ok(None)
             }
             bytecode::Instruction::BuildSet { size } => {
                 let set = PySet::new_ref(&vm.ctx);
-                {
-                    for element in self.pop_multiple(size.get(arg) as usize) {
-                        set.add(element, vm)?;
-                    }
+                for element in self.pop_multiple(size.get(arg) as usize) {
+                    set.add(element, vm)?;
                 }
                 self.push_value(set.into());
                 Ok(None)
             }
-            bytecode::Instruction::BuildSetUnpack { size } => {
+            bytecode::Instruction::BuildSetFromTuples { size } => {
                 let set = PySet::new_ref(&vm.ctx);
-                {
-                    for element in self.pop_multiple(size.get(arg) as usize) {
-                        vm.map_iterable_object(&element, |x| set.add(x, vm))??;
+                for element in self.pop_multiple(size.get(arg) as usize) {
+                    // SAFETY: trust compiler
+                    let tup = unsafe { element.downcast_unchecked::<PyTuple>() };
+                    for item in tup.iter() {
+                        set.add(item.clone(), vm)?;
                     }
                 }
                 self.push_value(set.into());
@@ -742,12 +743,21 @@ impl ExecutingFrame<'_> {
                 self.push_value(list_obj.into());
                 Ok(None)
             }
-            bytecode::Instruction::BuildTupleUnpack { size } => {
-                let elements = self.unpack_elements(vm, size.get(arg) as usize)?;
+            bytecode::Instruction::BuildTupleFromTuples { size } => {
+                // SAFETY: compiler guarantees `size` tuples are on the stack
+                let elements = unsafe { self.flatten_tuples(size.get(arg) as usize) };
                 let list_obj = vm.ctx.new_tuple(elements);
                 self.push_value(list_obj.into());
                 Ok(None)
             }
+            bytecode::Instruction::BuildTupleFromIter => {
+                if !self.top_value().class().is(vm.ctx.types.tuple_type) {
+                    let elements: Vec<_> = self.pop_value().try_to_value(vm)?;
+                    let list_obj = vm.ctx.new_tuple(elements);
+                    self.push_value(list_obj.into());
+                }
+                Ok(None)
+            }
             bytecode::Instruction::BuildMap { size } => self.execute_build_map(vm, size.get(arg)),
             bytecode::Instruction::BuildMapForCall { size } => {
                 self.execute_build_map_for_call(vm, size.get(arg))
@@ -1234,14 +1244,14 @@ impl ExecutingFrame<'_> {
             })
     }
 
-    #[cfg_attr(feature = "flame-it", flame("Frame"))]
-    fn unpack_elements(&mut self, vm: &VirtualMachine, size: usize) -> PyResult<Vec<PyObjectRef>> {
-        let mut result = Vec::<PyObjectRef>::new();
-        for element in self.pop_multiple(size) {
-            let items: Vec<_> = element.try_to_value(vm)?;
-            result.extend(items);
+    unsafe fn flatten_tuples(&mut self, size: usize) -> Vec<PyObjectRef> {
+        let mut elements = Vec::new();
+        for tup in self.pop_multiple(size) {
+            // SAFETY: caller ensures that the elements are tuples
+            let tup = unsafe { tup.downcast_unchecked::<PyTuple>() };
+            elements.extend(tup.iter().cloned());
         }
-        Ok(result)
+        elements
     }
 
     #[cfg_attr(feature = "flame-it", flame("Frame"))]
@@ -1490,8 +1500,10 @@ impl ExecutingFrame<'_> {
         } else {
             IndexMap::new()
         };
-        let args = self.pop_value();
-        let args = args.try_to_value(vm)?;
+        // SAFETY: trust compiler
+        let args = unsafe { self.pop_value().downcast_unchecked::<PyTuple>() }
+            .as_slice()
+            .to_vec();
         Ok(FuncArgs { args, kwargs })
     }