Skip to content

Split TestOperator instruction #6306

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
youknowone merged 3 commits into from
Nov 29, 2025
Merged

Split TestOperator instruction #6306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91f5965
Split `TestOperator` inatruction
ShaharNaveh Nov 28, 2025
625480d
Update snapshot
ShaharNaveh Nov 28, 2025
c323c6c
Set as have label
ShaharNaveh Nov 28, 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
48 changes: 10 additions & 38 deletions crates/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use rustpython_compiler_core::{
Mode, OneIndexed, PositionEncoding, SourceFile, SourceLocation,
bytecode::{
self, Arg as OpArgMarker, BinaryOperator, CodeObject, ComparisonOperator, ConstantData,
Instruction, OpArg, OpArgType, UnpackExArgs,
Instruction, Invert, OpArg, OpArgType, UnpackExArgs,
},
};
use rustpython_wtf8::Wtf8Buf;
Expand Down Expand Up @@ -2034,20 +2034,7 @@ impl Compiler {

// Check exception type:
self.compile_expression(exc_type)?;
emit!(
self,
Instruction::TestOperation {
op: bytecode::TestOperator::ExceptionMatch,
}
);

// We cannot handle this exception type:
emit!(
self,
Instruction::PopJumpIfFalse {
target: next_handler,
}
);
emit!(self, Instruction::JumpIfNotExcMatch(next_handler));

// We have a match, store in name (except x as y)
if let Some(alias) = name {
Expand Down Expand Up @@ -3477,12 +3464,7 @@ impl Compiler {
// 4. Load None.
self.emit_load_const(ConstantData::None);
// 5. Compare with IS_OP 1.
emit!(
self,
Instruction::TestOperation {
op: bytecode::TestOperator::IsNot
}
);
emit!(self, Instruction::IsOp(Invert::Yes));

// At this point the TOS is a tuple of (nargs + n_attrs) attributes (or None).
pc.on_top += 1;
Expand Down Expand Up @@ -3648,12 +3630,8 @@ impl Compiler {

// Check if copy is None (consumes the copy like POP_JUMP_IF_NONE)
self.emit_load_const(ConstantData::None);
emit!(
self,
Instruction::TestOperation {
op: bytecode::TestOperator::IsNot
}
);
emit!(self, Instruction::IsOp(Invert::Yes));

// Stack: [subject, keys_tuple, values_tuple, bool]
self.jump_to_fail_pop(pc, JumpOp::PopJumpIfFalse)?;
// Stack: [subject, keys_tuple, values_tuple]
Expand Down Expand Up @@ -3948,12 +3926,7 @@ impl Compiler {
Singleton::True => ConstantData::Boolean { value: true },
});
// Compare using the "Is" operator.
emit!(
self,
Instruction::TestOperation {
op: bytecode::TestOperator::Is
}
);
emit!(self, Instruction::IsOp(Invert::No));
// Jump to the failure label if the comparison is false.
self.jump_to_fail_pop(pc, JumpOp::PopJumpIfFalse)?;
Ok(())
Expand Down Expand Up @@ -4082,7 +4055,6 @@ impl Compiler {
let (last_val, mid_exprs) = exprs.split_last().unwrap();

use bytecode::ComparisonOperator::*;
use bytecode::TestOperator::*;
let compile_cmpop = |c: &mut Self, op: &CmpOp| match op {
CmpOp::Eq => emit!(c, Instruction::CompareOperation { op: Equal }),
CmpOp::NotEq => emit!(c, Instruction::CompareOperation { op: NotEqual }),
Expand All @@ -4092,10 +4064,10 @@ impl Compiler {
CmpOp::GtE => {
emit!(c, Instruction::CompareOperation { op: GreaterOrEqual })
}
CmpOp::In => emit!(c, Instruction::TestOperation { op: In }),
CmpOp::NotIn => emit!(c, Instruction::TestOperation { op: NotIn }),
CmpOp::Is => emit!(c, Instruction::TestOperation { op: Is }),
CmpOp::IsNot => emit!(c, Instruction::TestOperation { op: IsNot }),
CmpOp::In => emit!(c, Instruction::ContainsOp(Invert::No)),
CmpOp::NotIn => emit!(c, Instruction::ContainsOp(Invert::Yes)),
CmpOp::Is => emit!(c, Instruction::IsOp(Invert::No)),
CmpOp::IsNot => emit!(c, Instruction::IsOp(Invert::Yes)),
};

// a == b == c == d
Expand Down
53 changes: 32 additions & 21 deletions crates/compiler-core/src/bytecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,10 @@ pub enum Instruction {
Subscript,
StoreSubscript,
DeleteSubscript,
/// Performs `is` comparison, or `is not` if `invert` is 1.
IsOp(Arg<Invert>),
/// Performs `in` comparison, or `not in` if `invert` is 1.
ContainsOp(Arg<Invert>),
StoreAttr {
idx: Arg<NameIdx>,
},
Expand All @@ -600,9 +604,6 @@ pub enum Instruction {
LoadAttr {
idx: Arg<NameIdx>,
},
TestOperation {
op: Arg<TestOperator>,
},
CompareOperation {
op: Arg<ComparisonOperator>,
},
Expand All @@ -628,6 +629,10 @@ pub enum Instruction {
Break {
target: Arg<Label>,
},
/// Performs exception matching for except.
/// Tests whether the STACK[-2] is an exception matching STACK[-1].
/// Pops STACK[-1] and pushes the boolean result of the test.
JumpIfNotExcMatch(Arg<Label>),
Jump {
target: Arg<Label>,
},
Expand Down Expand Up @@ -1088,19 +1093,6 @@ op_arg_enum!(
}
);

op_arg_enum!(
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(u8)]
pub enum TestOperator {
In = 0,
NotIn = 1,
Is = 2,
IsNot = 3,
/// two exceptions that match?
ExceptionMatch = 4,
}
);

op_arg_enum!(
/// The possible Binary operators
/// # Examples
Expand Down Expand Up @@ -1141,6 +1133,24 @@ op_arg_enum!(
}
);

op_arg_enum!(
/// Whether or not to invert the operation.
#[repr(u8)]
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum Invert {
/// ```py
/// foo is bar
/// x in lst
/// ```
No = 0,
/// ```py
/// foo is not bar
/// x not in lst
/// ```
Yes = 1,
}
);

#[derive(Copy, Clone)]
pub struct UnpackExArgs {
pub before: u8,
Expand Down Expand Up @@ -1462,10 +1472,7 @@ impl Instruction {
DeleteAttr { .. } => -1,
LoadConst { .. } => 1,
UnaryOperation { .. } => 0,
BinaryOperation { .. }
| BinaryOperationInplace { .. }
| TestOperation { .. }
| CompareOperation { .. } => -1,
BinaryOperation { .. } | BinaryOperationInplace { .. } | CompareOperation { .. } => -1,
BinarySubscript => -1,
CopyItem { .. } => 1,
Pop => -1,
Expand Down Expand Up @@ -1508,6 +1515,8 @@ impl Instruction {
1
}
}
IsOp(_) | ContainsOp(_) => -1,
JumpIfNotExcMatch(_) => -2,
ReturnValue => -1,
ReturnConst { .. } => 0,
Resume { .. } => 0,
Expand Down Expand Up @@ -1654,6 +1663,9 @@ impl Instruction {
DeleteGlobal(idx) => w!(DeleteGlobal, name = idx),
DeleteDeref(idx) => w!(DeleteDeref, cell_name = idx),
LoadClosure(i) => w!(LoadClosure, cell_name = i),
IsOp(inv) => w!(IS_OP, ?inv),
ContainsOp(inv) => w!(CONTAINS_OP, ?inv),
JumpIfNotExcMatch(target) => w!(JUMP_IF_NOT_EXC_MATCH, target),
Subscript => w!(Subscript),
StoreSubscript => w!(StoreSubscript),
DeleteSubscript => w!(DeleteSubscript),
Expand All @@ -1665,7 +1677,6 @@ impl Instruction {
BinaryOperationInplace { op } => w!(BinaryOperationInplace, ?op),
BinarySubscript => w!(BinarySubscript),
LoadAttr { idx } => w!(LoadAttr, name = idx),
TestOperation { op } => w!(TestOperation, ?op),
CompareOperation { op } => w!(CompareOperation, ?op),
CopyItem { index } => w!(CopyItem, index),
Pop => w!(Pop),
Expand Down
81 changes: 46 additions & 35 deletions crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,52 @@ impl ExecutingFrame<'_> {
bytecode::Instruction::StoreAttr { idx } => self.store_attr(vm, idx.get(arg)),
bytecode::Instruction::DeleteAttr { idx } => self.delete_attr(vm, idx.get(arg)),
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::IsOp(invert) => {
let b = self.pop_value();
let a = self.pop_value();
let res = a.is(&b);

let value = match invert.get(arg) {
bytecode::Invert::No => res,
bytecode::Invert::Yes => !res,
};
self.push_value(vm.ctx.new_bool(value).into());
Ok(None)
}
bytecode::Instruction::ContainsOp(invert) => {
let b = self.pop_value();
let a = self.pop_value();

let value = match invert.get(arg) {
bytecode::Invert::No => self._in(vm, &a, &b)?,
bytecode::Invert::Yes => self._not_in(vm, &a, &b)?,
};
self.push_value(vm.ctx.new_bool(value).into());
Ok(None)
}
bytecode::Instruction::JumpIfNotExcMatch(target) => {
let b = self.pop_value();
let a = self.pop_value();
if let Some(tuple_of_exceptions) = b.downcast_ref::<PyTuple>() {
for exception in tuple_of_exceptions {
if !exception
.is_subclass(vm.ctx.exceptions.base_exception_type.into(), vm)?
{
return Err(vm.new_type_error(
"catching classes that do not inherit from BaseException is not allowed",
));
}
}
} else if !b.is_subclass(vm.ctx.exceptions.base_exception_type.into(), vm)? {
return Err(vm.new_type_error(
"catching classes that do not inherit from BaseException is not allowed",
));
}

let value = a.is_instance(&b, vm)?;
self.push_value(vm.ctx.new_bool(value).into());
self.pop_jump_if(vm, target.get(arg), false)
}
bytecode::Instruction::CompareOperation { op } => self.execute_compare(vm, op.get(arg)),
bytecode::Instruction::ReturnValue => {
let value = self.pop_value();
Expand Down Expand Up @@ -2234,40 +2279,6 @@ impl ExecutingFrame<'_> {
Ok(!self._in(vm, needle, haystack)?)
}

#[cfg_attr(feature = "flame-it", flame("Frame"))]
fn execute_test(&mut self, vm: &VirtualMachine, op: bytecode::TestOperator) -> FrameResult {
let b = self.pop_value();
let a = self.pop_value();
let value = match op {
bytecode::TestOperator::Is => a.is(&b),
bytecode::TestOperator::IsNot => !a.is(&b),
bytecode::TestOperator::In => self._in(vm, &a, &b)?,
bytecode::TestOperator::NotIn => self._not_in(vm, &a, &b)?,
bytecode::TestOperator::ExceptionMatch => {
if let Some(tuple_of_exceptions) = b.downcast_ref::<PyTuple>() {
for exception in tuple_of_exceptions {
if !exception
.is_subclass(vm.ctx.exceptions.base_exception_type.into(), vm)?
{
return Err(vm.new_type_error(
"catching classes that do not inherit from BaseException is not allowed",
));
}
}
} else if !b.is_subclass(vm.ctx.exceptions.base_exception_type.into(), vm)? {
return Err(vm.new_type_error(
"catching classes that do not inherit from BaseException is not allowed",
));
}

a.is_instance(&b, vm)?
}
};

self.push_value(vm.ctx.new_bool(value).into());
Ok(None)
}

#[cfg_attr(feature = "flame-it", flame("Frame"))]
fn execute_compare(
&mut self,
Expand Down
Loading