Skip to content

Commit 3ad8fd7

Browse files
ivan-shrimpyouknowone
ivan-shrimp
authored and
youknowone
committed
fix expression list order
don't emit a no-op when unpacking a single element assume positional args stored as tuple in extended call
1 parent 160363f commit 3ad8fd7

File tree

4 files changed

+95
-60
lines changed

4 files changed

+95
-60
lines changed

‎compiler/codegen/src/compile.rs

+28-29
Original file line numberDiff line numberDiff line change
@@ -2430,23 +2430,25 @@ impl Compiler<'_> {
24302430
Expr::List(ExprList { elts, .. }) => {
24312431
let (size, unpack) = self.gather_elements(0, elts)?;
24322432
if unpack {
2433-
emit!(self, Instruction::BuildListUnpack { size });
2433+
emit!(self, Instruction::BuildListFromTuples { size });
24342434
} else {
24352435
emit!(self, Instruction::BuildList { size });
24362436
}
24372437
}
24382438
Expr::Tuple(ExprTuple { elts, .. }) => {
24392439
let (size, unpack) = self.gather_elements(0, elts)?;
24402440
if unpack {
2441-
emit!(self, Instruction::BuildTupleUnpack { size });
2441+
if size > 1 {
2442+
emit!(self, Instruction::BuildTupleFromTuples { size });
2443+
}
24422444
} else {
24432445
emit!(self, Instruction::BuildTuple { size });
24442446
}
24452447
}
24462448
Expr::Set(ExprSet { elts, .. }) => {
24472449
let (size, unpack) = self.gather_elements(0, elts)?;
24482450
if unpack {
2449-
emit!(self, Instruction::BuildSetUnpack { size });
2451+
emit!(self, Instruction::BuildSetFromTuples { size });
24502452
} else {
24512453
emit!(self, Instruction::BuildSet { size });
24522454
}
@@ -2819,7 +2821,7 @@ impl Compiler<'_> {
28192821
let call = if unpack || has_double_star {
28202822
// Create a tuple with positional args:
28212823
if unpack {
2822-
emit!(self, Instruction::BuildTupleUnpack { size });
2824+
emit!(self, Instruction::BuildTupleFromTuples { size });
28232825
} else {
28242826
emit!(self, Instruction::BuildTuple { size });
28252827
}
@@ -2863,34 +2865,31 @@ impl Compiler<'_> {
28632865

28642866
let size = if has_stars {
28652867
let mut size = 0;
2868+
let mut iter = elements.iter().peekable();
2869+
let mut run_size = before;
28662870

2867-
if before > 0 {
2868-
emit!(self, Instruction::BuildTuple { size: before });
2869-
size += 1;
2870-
}
2871+
loop {
2872+
if iter.peek().is_none_or(|e| matches!(e, Expr::Starred(_))) {
2873+
emit!(self, Instruction::BuildTuple { size: run_size });
2874+
run_size = 0;
2875+
size += 1;
2876+
}
28712877

2872-
let groups = elements
2873-
.iter()
2874-
.map(|element| {
2875-
if let Expr::Starred(ExprStarred { value, .. }) = &element {
2876-
(true, value.as_ref())
2877-
} else {
2878-
(false, element)
2878+
match iter.next() {
2879+
Some(Expr::Starred(ExprStarred { value, .. })) => {
2880+
self.compile_expression(value)?;
2881+
// We need to collect each unpacked element into a
2882+
// tuple, since any side-effects during the conversion
2883+
// should be made visible before evaluating remaining
2884+
// expressions.
2885+
emit!(self, Instruction::BuildTupleFromIter);
2886+
size += 1;
28792887
}
2880-
})
2881-
.chunk_by(|(starred, _)| *starred);
2882-
2883-
for (starred, run) in &groups {
2884-
let mut run_size = 0;
2885-
for (_, value) in run {
2886-
self.compile_expression(value)?;
2887-
run_size += 1
2888-
}
2889-
if starred {
2890-
size += run_size
2891-
} else {
2892-
emit!(self, Instruction::BuildTuple { size: run_size });
2893-
size += 1
2888+
Some(element) => {
2889+
self.compile_expression(element)?;
2890+
run_size += 1;
2891+
}
2892+
None => break,
28942893
}
28952894
}
28962895

‎compiler/core/src/bytecode.rs

+13-10
Original file line numberDiff line numberDiff line change
@@ -542,19 +542,20 @@ pub enum Instruction {
542542
BuildTuple {
543543
size: Arg<u32>,
544544
},
545-
BuildTupleUnpack {
545+
BuildTupleFromTuples {
546546
size: Arg<u32>,
547547
},
548+
BuildTupleFromIter,
548549
BuildList {
549550
size: Arg<u32>,
550551
},
551-
BuildListUnpack {
552+
BuildListFromTuples {
552553
size: Arg<u32>,
553554
},
554555
BuildSet {
555556
size: Arg<u32>,
556557
},
557-
BuildSetUnpack {
558+
BuildSetFromTuples {
558559
size: Arg<u32>,
559560
},
560561
BuildMap {
@@ -1260,11 +1261,12 @@ impl Instruction {
12601261
Raise { kind } => -(kind.get(arg) as u8 as i32),
12611262
BuildString { size }
12621263
| BuildTuple { size, .. }
1263-
| BuildTupleUnpack { size, .. }
1264+
| BuildTupleFromTuples { size, .. }
12641265
| BuildList { size, .. }
1265-
| BuildListUnpack { size, .. }
1266+
| BuildListFromTuples { size, .. }
12661267
| BuildSet { size, .. }
1267-
| BuildSetUnpack { size, .. } => -(size.get(arg) as i32) + 1,
1268+
| BuildSetFromTuples { size, .. } => -(size.get(arg) as i32) + 1,
1269+
BuildTupleFromIter => 0,
12681270
BuildMap { size } => {
12691271
let nargs = size.get(arg) * 2;
12701272
-(nargs as i32) + 1
@@ -1447,13 +1449,14 @@ impl Instruction {
14471449
Raise { kind } => w!(Raise, ?kind),
14481450
BuildString { size } => w!(BuildString, size),
14491451
BuildTuple { size } => w!(BuildTuple, size),
1450-
BuildTupleUnpack { size } => w!(BuildTupleUnpack, size),
1452+
BuildTupleFromTuples { size } => w!(BuildTupleFromTuples, size),
1453+
BuildTupleFromIter => w!(BuildTupleFromIter),
14511454
BuildList { size } => w!(BuildList, size),
1452-
BuildListUnpack { size } => w!(BuildListUnpack, size),
1455+
BuildListFromTuples { size } => w!(BuildListFromTuples, size),
14531456
BuildSet { size } => w!(BuildSet, size),
1454-
BuildSetUnpack { size } => w!(BuildSetUnpack, size),
1457+
BuildSetFromTuples { size } => w!(BuildSetFromTuples, size),
14551458
BuildMap { size } => w!(BuildMap, size),
1456-
BuildMapForCall { size } => w!(BuildMap, size),
1459+
BuildMapForCall { size } => w!(BuildMapForCall, size),
14571460
DictUpdate => w!(DictUpdate),
14581461
BuildSlice { step } => w!(BuildSlice, step),
14591462
ListAppend { i } => w!(ListAppend, i),

‎extra_tests/snippets/builtin_list.py

+21
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,27 @@ def iadd_slice():
636636
a = [*[1, 2], 3, *[4, 5]]
637637
assert a == [1, 2, 3, 4, 5]
638638

639+
# Test for list unpacking evaluation order (https://github.com/RustPython/RustPython/issues/5566)
640+
a = [1, 2]
641+
b = [a.append(3), *a, a.append(4), *a]
642+
assert a == [1, 2, 3, 4]
643+
assert b == [None, 1, 2, 3, None, 1, 2, 3, 4]
644+
645+
for base in object, list, tuple:
646+
# do not assume that a type inherited from some sequence type behaves like
647+
# that sequence type
648+
class C(base):
649+
def __iter__(self):
650+
a.append(2)
651+
def inner():
652+
yield 3
653+
a.append(4)
654+
return inner()
655+
656+
a = [1]
657+
b = [*a, *C(), *a.copy()]
658+
assert b == [1, 3, 1, 2, 4]
659+
639660
# Test for list entering daedlock or not (https://github.com/RustPython/RustPython/pull/2933)
640661
class MutatingCompare:
641662
def __eq__(self, other):

‎vm/src/frame.rs

+33-21
Original file line numberDiff line numberDiff line change
@@ -710,27 +710,28 @@ impl ExecutingFrame<'_> {
710710
self.push_value(list_obj.into());
711711
Ok(None)
712712
}
713-
bytecode::Instruction::BuildListUnpack { size } => {
714-
let elements = self.unpack_elements(vm, size.get(arg) as usize)?;
713+
bytecode::Instruction::BuildListFromTuples { size } => {
714+
// SAFETY: compiler guarantees `size` tuples are on the stack
715+
let elements = unsafe { self.flatten_tuples(size.get(arg) as usize) };
715716
let list_obj = vm.ctx.new_list(elements);
716717
self.push_value(list_obj.into());
717718
Ok(None)
718719
}
719720
bytecode::Instruction::BuildSet { size } => {
720721
let set = PySet::new_ref(&vm.ctx);
721-
{
722-
for element in self.pop_multiple(size.get(arg) as usize) {
723-
set.add(element, vm)?;
724-
}
722+
for element in self.pop_multiple(size.get(arg) as usize) {
723+
set.add(element, vm)?;
725724
}
726725
self.push_value(set.into());
727726
Ok(None)
728727
}
729-
bytecode::Instruction::BuildSetUnpack { size } => {
728+
bytecode::Instruction::BuildSetFromTuples { size } => {
730729
let set = PySet::new_ref(&vm.ctx);
731-
{
732-
for element in self.pop_multiple(size.get(arg) as usize) {
733-
vm.map_iterable_object(&element, |x| set.add(x, vm))??;
730+
for element in self.pop_multiple(size.get(arg) as usize) {
731+
// SAFETY: trust compiler
732+
let tup = unsafe { element.downcast_unchecked::<PyTuple>() };
733+
for item in tup.iter() {
734+
set.add(item.clone(), vm)?;
734735
}
735736
}
736737
self.push_value(set.into());
@@ -742,12 +743,21 @@ impl ExecutingFrame<'_> {
742743
self.push_value(list_obj.into());
743744
Ok(None)
744745
}
745-
bytecode::Instruction::BuildTupleUnpack { size } => {
746-
let elements = self.unpack_elements(vm, size.get(arg) as usize)?;
746+
bytecode::Instruction::BuildTupleFromTuples { size } => {
747+
// SAFETY: compiler guarantees `size` tuples are on the stack
748+
let elements = unsafe { self.flatten_tuples(size.get(arg) as usize) };
747749
let list_obj = vm.ctx.new_tuple(elements);
748750
self.push_value(list_obj.into());
749751
Ok(None)
750752
}
753+
bytecode::Instruction::BuildTupleFromIter => {
754+
if !self.top_value().class().is(vm.ctx.types.tuple_type) {
755+
let elements: Vec<_> = self.pop_value().try_to_value(vm)?;
756+
let list_obj = vm.ctx.new_tuple(elements);
757+
self.push_value(list_obj.into());
758+
}
759+
Ok(None)
760+
}
751761
bytecode::Instruction::BuildMap { size } => self.execute_build_map(vm, size.get(arg)),
752762
bytecode::Instruction::BuildMapForCall { size } => {
753763
self.execute_build_map_for_call(vm, size.get(arg))
@@ -1234,14 +1244,14 @@ impl ExecutingFrame<'_> {
12341244
})
12351245
}
12361246

1237-
#[cfg_attr(feature = "flame-it", flame("Frame"))]
1238-
fn unpack_elements(&mut self, vm: &VirtualMachine, size: usize) -> PyResult<Vec<PyObjectRef>> {
1239-
let mut result = Vec::<PyObjectRef>::new();
1240-
for element in self.pop_multiple(size) {
1241-
let items: Vec<_> = element.try_to_value(vm)?;
1242-
result.extend(items);
1247+
unsafe fn flatten_tuples(&mut self, size: usize) -> Vec<PyObjectRef> {
1248+
let mut elements = Vec::new();
1249+
for tup in self.pop_multiple(size) {
1250+
// SAFETY: caller ensures that the elements are tuples
1251+
let tup = unsafe { tup.downcast_unchecked::<PyTuple>() };
1252+
elements.extend(tup.iter().cloned());
12431253
}
1244-
Ok(result)
1254+
elements
12451255
}
12461256

12471257
#[cfg_attr(feature = "flame-it", flame("Frame"))]
@@ -1490,8 +1500,10 @@ impl ExecutingFrame<'_> {
14901500
} else {
14911501
IndexMap::new()
14921502
};
1493-
let args = self.pop_value();
1494-
let args = args.try_to_value(vm)?;
1503+
// SAFETY: trust compiler
1504+
let args = unsafe { self.pop_value().downcast_unchecked::<PyTuple>() }
1505+
.as_slice()
1506+
.to_vec();
14951507
Ok(FuncArgs { args, kwargs })
14961508
}
14971509

0 commit comments

Comments
 (0)