From bef2639b584a0113f15d1dc44c6bac6b89c4b9d7 Mon Sep 17 00:00:00 2001 From: ivan-shrimp <70307174+ivan-shrimp@users.noreply.github.com> Date: Sun, 2 Mar 2025 23:50:36 +0800 Subject: [PATCH] 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 | 57 ++++++++++++++-------------- compiler/core/src/bytecode.rs | 23 ++++++----- extra_tests/snippets/builtin_list.py | 21 ++++++++++ vm/src/frame.rs | 54 ++++++++++++++++---------- 4 files changed, 95 insertions(+), 60 deletions(-) diff --git a/compiler/codegen/src/compile.rs b/compiler/codegen/src/compile.rs index 38d5b8fb12..83e2f5cf44 100644 --- a/compiler/codegen/src/compile.rs +++ b/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; - } + loop { + if iter.peek().is_none_or(|e| matches!(e, Expr::Starred(_))) { + emit!(self, Instruction::BuildTuple { size: run_size }); + run_size = 0; + size += 1; + } - let groups = elements - .iter() - .map(|element| { - if let Expr::Starred(ExprStarred { value, .. }) = &element { - (true, value.as_ref()) - } else { - (false, element) + 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; } - }) - .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 { - emit!(self, Instruction::BuildTuple { size: run_size }); - size += 1 + Some(element) => { + self.compile_expression(element)?; + run_size += 1; + } + None => break, } } diff --git a/compiler/core/src/bytecode.rs b/compiler/core/src/bytecode.rs index 2e8ff29014..94d080ace4 100644 --- a/compiler/core/src/bytecode.rs +++ b/compiler/core/src/bytecode.rs @@ -542,19 +542,20 @@ pub enum Instruction { BuildTuple { size: Arg, }, - BuildTupleUnpack { + BuildTupleFromTuples { size: Arg, }, + BuildTupleFromIter, BuildList { size: Arg, }, - BuildListUnpack { + BuildListFromTuples { size: Arg, }, BuildSet { size: Arg, }, - BuildSetUnpack { + BuildSetFromTuples { size: Arg, }, 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), diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py index 6fde1011ea..b5c08796ba 100644 --- a/extra_tests/snippets/builtin_list.py +++ b/extra_tests/snippets/builtin_list.py @@ -636,6 +636,27 @@ def iadd_slice(): 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): diff --git a/vm/src/frame.rs b/vm/src/frame.rs index b148934467..78f03a04d8 100644 --- a/vm/src/frame.rs +++ b/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::() }; + 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> { - let mut result = Vec::::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 { + 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::() }; + 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::() } + .as_slice() + .to_vec(); Ok(FuncArgs { args, kwargs }) }