Skip to content

Cleanup match statement codegen #5700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 18, 2025
Merged

Cleanup match statement codegen #5700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
49b348c
Remove Instruction::IsOperation
arihant2math Apr 15, 2025
c7042fd
remove unneeded validation
arihant2math Apr 15, 2025
456e555
better error
arihant2math Apr 15, 2025
d47944b
error handling
arihant2math Apr 15, 2025
09c199a
match cleanup
arihant2math Apr 15, 2025
e949c9a
rename
arihant2math Apr 15, 2025
628287c
update snapshot
arihant2math Apr 15, 2025
d44324d
clippy
arihant2math Apr 15, 2025
2127202
improve error handling
arihant2math Apr 15, 2025
4d53f59
remove match test
arihant2math Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 44 additions & 76 deletions compiler/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1944,32 +1944,35 @@ impl Compiler<'_> {
n: Option<&Identifier>,
pc: &mut PatternContext,
) -> CompileResult<()> {
// If no name is provided, simply pop the top of the stack.
if n.is_none() {
emit!(self, Instruction::Pop);
return Ok(());
}
let name = n.unwrap();

// Check if the name is forbidden for storing.
if self.forbidden_name(name.as_str(), NameUsage::Store)? {
return Err(self.compile_error_forbidden_name(name.as_str()));
}
match n {
// If no name is provided, simply pop the top of the stack.
None => {
emit!(self, Instruction::Pop);
Ok(())
}
Comment on lines +1947 to +1952
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use let-else, to prevent rightward drift.

        let Some(name) = n else {
            // If no name is provided, simply pop the top of the stack.
            emit!(self, Instruction::Pop);
            return Ok(());
        };

Some(name) => {
// Check if the name is forbidden for storing.
if self.forbidden_name(name.as_str(), NameUsage::Store)? {
return Err(self.compile_error_forbidden_name(name.as_str()));
}

// Ensure we don't store the same name twice.
if pc.stores.contains(&name.to_string()) {
return Err(self.error(CodegenErrorType::DuplicateStore(name.as_str().to_string())));
}
// Ensure we don't store the same name twice.
// TODO: maybe pc.stores should be a set?
if pc.stores.contains(&name.to_string()) {
return Err(
self.error(CodegenErrorType::DuplicateStore(name.as_str().to_string()))
);
}

// Calculate how many items to rotate:
// the count is the number of items to preserve on top plus the current stored names,
// plus one for the new value.
let rotations = pc.on_top + pc.stores.len() + 1;
self.pattern_helper_rotate(rotations)?;
// Calculate how many items to rotate:
let rotations = pc.on_top + pc.stores.len() + 1;
self.pattern_helper_rotate(rotations)?;

// Append the name to the captured stores.
pc.stores.push(name.to_string());
Ok(())
// Append the name to the captured stores.
pc.stores.push(name.to_string());
Ok(())
}
}
}

fn pattern_unpack_helper(&mut self, elts: &[Pattern]) -> CompileResult<()> {
Expand Down Expand Up @@ -2155,10 +2158,7 @@ impl Compiler<'_> {
for ident in attrs.iter().take(n_attrs).skip(i + 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't the code you changed, but if you're looking to make it more rusty, you could use .enumerate() here for the outer loop (for (i, attr) in attrs.iter().enumerate()) and then just for ident in &attrs[i + 1..] for the inner loop.

let other = ident.as_str();
if attr == other {
todo!();
// return Err(self.compiler_error(
// &format!("attribute name repeated in class pattern: {}", attr),
// ));
return Err(self.error(CodegenErrorType::RepeatedAttributePattern));
}
}
}
Expand All @@ -2185,16 +2185,6 @@ impl Compiler<'_> {

let nargs = patterns.len();
let n_attrs = kwd_attrs.len();
let nkwd_patterns = kwd_patterns.len();

// Validate that keyword attribute names and patterns match in length.
if n_attrs != nkwd_patterns {
let msg = format!(
"kwd_attrs ({}) / kwd_patterns ({}) length mismatch in class pattern",
n_attrs, nkwd_patterns
);
unreachable!("{}", msg);
}

// Check for too many sub-patterns.
if nargs > u32::MAX as usize || (nargs + n_attrs).saturating_sub(1) > i32::MAX as usize {
Expand Down Expand Up @@ -2223,6 +2213,8 @@ impl Compiler<'_> {
});
}

use bytecode::TestOperator::*;

// Emit instructions:
// 1. Load the new tuple of attribute names.
self.emit_load_const(ConstantData::Tuple {
Expand All @@ -2235,7 +2227,7 @@ impl Compiler<'_> {
// 4. Load None.
self.emit_load_const(ConstantData::None);
// 5. Compare with IS_OP 1.
emit!(self, Instruction::IsOperation(true));
emit!(self, Instruction::TestOperation { op: IsNot });

// At this point the TOS is a tuple of (nargs + n_attrs) attributes (or None).
pc.on_top += 1;
Expand All @@ -2253,20 +2245,12 @@ impl Compiler<'_> {
pc.on_top -= 1;

// Process each sub-pattern.
for i in 0..total {
// Decrement the on_top counter as each sub-pattern is processed.
for subpattern in patterns.iter().chain(kwd_patterns.iter()) {
// Decrement the on_top counter as each sub-pattern is processed
// (on_top should be zero at the end of the algorithm as a sanity check).
pc.on_top -= 1;
let subpattern = if i < nargs {
// Positional sub-pattern.
&patterns[i]
} else {
// Keyword sub-pattern.
&kwd_patterns[i - nargs]
};
if subpattern.is_wildcard() {
// For wildcard patterns, simply pop the top of the stack.
emit!(self, Instruction::Pop);
continue;
}
// Compile the subpattern without irrefutability checks.
self.compile_pattern_subpattern(subpattern, pc)?;
Expand Down Expand Up @@ -2351,7 +2335,7 @@ impl Compiler<'_> {
// emit!(self, Instruction::CopyItem { index: 1_u32 });
// self.emit_load_const(ConstantData::None);
// // TODO: should be is
// emit!(self, Instruction::IsOperation(true));
// emit!(self, Instruction::TestOperation::IsNot);
// self.jump_to_fail_pop(pc, JumpOp::PopJumpIfFalse)?;

// // Unpack the tuple of values.
Expand Down Expand Up @@ -2428,15 +2412,16 @@ impl Compiler<'_> {
} else {
let control_vec = control.as_ref().unwrap();
if nstores != control_vec.len() {
todo!();
// return self.compiler_error("alternative patterns bind different names");
return Err(self.error(CodegenErrorType::ConflictingNameBindPattern));
} else if nstores > 0 {
// Check that the names occur in the same order.
for icontrol in (0..nstores).rev() {
let name = &control_vec[icontrol];
// Find the index of `name` in the current stores.
let istores = pc.stores.iter().position(|n| n == name).unwrap();
// .ok_or_else(|| self.compiler_error("alternative patterns bind different names"))?;
let istores =
pc.stores.iter().position(|n| n == name).ok_or_else(|| {
self.error(CodegenErrorType::ConflictingNameBindPattern)
})?;
if icontrol != istores {
// The orders differ; we must reorder.
assert!(istores < icontrol, "expected istores < icontrol");
Expand Down Expand Up @@ -2480,14 +2465,14 @@ impl Compiler<'_> {
self.switch_to_block(end);

// Adjust the final captures.
let nstores = control.as_ref().unwrap().len();
let nrots = nstores + 1 + pc.on_top + pc.stores.len();
for i in 0..nstores {
let n_stores = control.as_ref().unwrap().len();
let n_rots = n_stores + 1 + pc.on_top + pc.stores.len();
for i in 0..n_stores {
// Rotate the capture to its proper place.
self.pattern_helper_rotate(nrots)?;
self.pattern_helper_rotate(n_rots)?;
let name = &control.as_ref().unwrap()[i];
// Check for duplicate binding.
if pc.stores.iter().any(|n| n == name) {
if pc.stores.contains(name) {
return Err(self.error(CodegenErrorType::DuplicateStore(name.to_string())));
}
pc.stores.push(name.clone());
Expand Down Expand Up @@ -4608,23 +4593,6 @@ for stop_exc in (StopIteration('spam'), StopAsyncIteration('ham')):
self.assertIs(ex, stop_exc)
else:
self.fail(f'{stop_exc} was suppressed')
"
));
}

#[test]
fn test_match() {
assert_dis_snapshot!(compile_exec(
"\
class Test:
pass

t = Test()
match t:
case Test():
assert True
case _:
assert False
"
));
}
Expand Down
8 changes: 8 additions & 0 deletions compiler/codegen/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ pub enum CodegenErrorType {
ForbiddenName,
DuplicateStore(String),
UnreachablePattern(PatternUnreachableReason),
RepeatedAttributePattern,
ConflictingNameBindPattern,
NotImplementedYet, // RustPython marker for unimplemented features
}

Expand Down Expand Up @@ -119,6 +121,12 @@ impl fmt::Display for CodegenErrorType {
UnreachablePattern(reason) => {
write!(f, "{reason} makes remaining patterns unreachable")
}
RepeatedAttributePattern => {
write!(f, "attribute name repeated in class pattern")
}
ConflictingNameBindPattern => {
write!(f, "alternative patterns bind different names")
}
NotImplementedYet => {
write!(f, "RustPython does not implement this feature yet")
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/codegen/src/ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ impl CodeInfo {
let instr_display = instr.display(display_arg, self);
eprint!("{instr_display}: {depth} {effect:+} => ");
}
if effect < 0 && depth < effect.unsigned_abs() {
panic!("The stack will underflow at {depth} with {effect} effect on {instr:?}");
}
let new_depth = depth.checked_add_signed(effect).unwrap();
if DEBUG {
eprintln!("{new_depth}");
Expand Down
53 changes: 0 additions & 53 deletions compiler/codegen/src/snapshots/rustpython_codegen__compile__tests__match.snap
View file
Open in desktop

This file was deleted.

7 changes: 1 addition & 6 deletions compiler/core/src/bytecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,9 +437,6 @@ pub enum Instruction {
TestOperation {
op: Arg<TestOperator>,
},
/// If the argument is true, perform IS NOT. Otherwise perform the IS operation.
// TODO: duplication of TestOperator::{Is,IsNot}. Fix later.
IsOperation(Arg<bool>),
CompareOperation {
op: Arg<ComparisonOperator>,
},
Expand Down Expand Up @@ -1227,8 +1224,7 @@ impl Instruction {
BinaryOperation { .. }
| BinaryOperationInplace { .. }
| TestOperation { .. }
| CompareOperation { .. }
| IsOperation(..) => -1,
| CompareOperation { .. } => -1,
BinarySubscript => -1,
CopyItem { .. } => 1,
Pop => -1,
Expand Down Expand Up @@ -1436,7 +1432,6 @@ impl Instruction {
BinarySubscript => w!(BinarySubscript),
LoadAttr { idx } => w!(LoadAttr, name = idx),
TestOperation { op } => w!(TestOperation, ?op),
IsOperation(neg) => w!(IsOperation, neg),
CompareOperation { op } => w!(CompareOperation, ?op),
CopyItem { index } => w!(CopyItem, index),
Pop => w!(Pop),
Expand Down
8 changes: 0 additions & 8 deletions vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,14 +851,6 @@ impl ExecutingFrame<'_> {
bytecode::Instruction::UnaryOperation { op } => self.execute_unary_op(vm, op.get(arg)),
bytecode::Instruction::TestOperation { op } => self.execute_test(vm, op.get(arg)),
bytecode::Instruction::CompareOperation { op } => self.execute_compare(vm, op.get(arg)),
bytecode::Instruction::IsOperation(neg) => {
let a = self.pop_value();
let b = self.pop_value();
// xor with neg to invert the result if needed
let result = vm.ctx.new_bool(a.is(b.as_ref()) ^ neg.get(arg));
self.push_value(result.into());
Ok(None)
}
bytecode::Instruction::ReturnValue => {
let value = self.pop_value();
self.unwind_blocks(vm, UnwindReason::Returning { value })
Expand Down
Loading