Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions crates/monty/src/bytecode/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,18 @@ impl<'a> Compiler<'a> {
self.code.emit(Opcode::StoreSubscr);
}

Node::SubscriptDelete {
target,
index,
target_position,
} => {
self.compile_name(target);
self.compile_expr(index)?;
// Set location to the target (e.g., `lst[10]`) for proper caret in tracebacks
self.code.set_location(*target_position, None);
self.code.emit(Opcode::DeleteSubscr);
}

Node::AttrAssign {
object,
attr,
Expand Down
9 changes: 5 additions & 4 deletions crates/monty/src/bytecode/vm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,12 +744,13 @@ impl<'a, T: ResourceTracker, P: PrintWriter> VM<'a, T, P> {
}
}
Opcode::DeleteSubscr => {
// TODO: Implement py_delitem on Value
let index = self.pop();
let obj = self.pop();
let mut obj = self.pop();
let result = obj.py_delitem(index, self.heap, self.interns);
obj.drop_with_heap(self.heap);
index.drop_with_heap(self.heap);
todo!("DeleteSubscr: py_delitem not yet implemented")
if let Err(e) = result {
catch_sync!(self, cached_frame, e);
}
}
Opcode::LoadAttr => {
let name_idx = fetch_u16!(cached_frame);
Expand Down
9 changes: 9 additions & 0 deletions crates/monty/src/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,15 @@ pub enum Node<F> {
/// Source position for error reporting.
position: CodeRange,
},
/// Subscript deletion (e.g., `del lst[10]` or `del d['key']`).
///
/// Deletes an item from a subscriptable object. For lists, this removes the element at the index.
/// For dicts, this removes the key-value pair. Returns an error if the index/key doesn't exist.
SubscriptDelete {
target: Identifier,
index: ExprLoc,
target_position: CodeRange,
},
}

/// A prepared function definition with resolved names and scope information.
Expand Down
11 changes: 11 additions & 0 deletions crates/monty/src/heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,17 @@ impl PyTrait for HeapData {
_ => Err(ExcType::type_error_not_sub_assignment(self.py_type(heap))),
}
}

fn py_delitem(&mut self, key: Value, heap: &mut Heap<impl ResourceTracker>, interns: &Interns) -> RunResult<()> {
match self {
Self::List(l) => l.py_delitem(key, heap, interns),
Self::Dict(d) => d.py_delitem(key, heap, interns),
_ => Err(ExcType::type_error(format!(
"'{}' object does not support item deletion",
self.py_type(heap)
))),
}
}
}

/// Hash caching state stored alongside each heap entry.
Expand Down
26 changes: 22 additions & 4 deletions crates/monty/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,10 @@ impl<'a> Parser<'a> {
Some(value) => Ok(Node::Return(self.parse_expression(*value)?)),
None => Ok(Node::ReturnNone),
},
Stmt::Delete(d) => Err(ParseError::not_implemented(
"the 'del' statement",
self.convert_range(d.range),
)),
Stmt::Delete(ast::StmtDelete { targets, range, .. }) => {
let target = first(targets, self.convert_range(range))?;
self.parse_delete(target)
}
Stmt::TypeAlias(t) => Err(ParseError::not_implemented("type aliases", self.convert_range(t.range))),
Stmt::Assign(ast::StmtAssign {
targets, value, range, ..
Expand Down Expand Up @@ -511,6 +511,24 @@ impl<'a> Parser<'a> {
}
}

fn parse_delete(&mut self, target: AstExpr) -> Result<ParseNode, ParseError> {
match target {
// subscript deletion like `del list[index]` or `del dict[key]`
AstExpr::Subscript(ast::ExprSubscript {
value, slice, range, ..
}) => Ok(Node::SubscriptDelete {
target: self.parse_identifier(*value)?,
index: self.parse_expression(*slice)?,
target_position: self.convert_range(range),
}),
// todo: other deletion targets are not yet supported
_ => Err(ParseError::not_implemented(
"del with non-subscript targets",
self.convert_range(target.range()),
)),
}
}

fn parse_expression(&mut self, expression: AstExpr) -> Result<ExprLoc, ParseError> {
match expression {
AstExpr::BoolOp(ast::ExprBoolOp { op, values, range, .. }) => {
Expand Down
24 changes: 24 additions & 0 deletions crates/monty/src/prepare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,20 @@ impl<'i> Prepare<'i> {
target_position,
});
}
Node::SubscriptDelete {
target,
index,
target_position,
} => {
// SubscriptDelete doesn't assign to the target itself, just modifies it
let target = self.get_id(target).0;
let index = self.prepare_expression(index)?;
new_nodes.push(Node::SubscriptDelete {
target,
index,
target_position,
});
}
Node::AttrAssign {
object,
attr,
Expand Down Expand Up @@ -1706,6 +1720,9 @@ fn collect_scope_info_from_node(
Node::SubscriptAssign { .. } => {
// Subscript assignment doesn't create a new name, it modifies existing container
}
Node::SubscriptDelete { .. } => {
// Subscript deletion doesn't create a new name, it modifies existing container
}
Node::AttrAssign { .. } => {
// Attribute assignment doesn't create a new name, it modifies existing object
}
Expand Down Expand Up @@ -1884,6 +1901,9 @@ fn collect_cell_vars_from_node(
collect_cell_vars_from_expr(index, our_locals, cell_vars, interner);
collect_cell_vars_from_expr(value, our_locals, cell_vars, interner);
}
Node::SubscriptDelete { index, .. } => {
collect_cell_vars_from_expr(index, our_locals, cell_vars, interner);
}
Node::AttrAssign { object, value, .. } => {
collect_cell_vars_from_expr(object, our_locals, cell_vars, interner);
collect_cell_vars_from_expr(value, our_locals, cell_vars, interner);
Expand Down Expand Up @@ -2107,6 +2127,10 @@ fn collect_referenced_names_from_node(node: &ParseNode, referenced: &mut AHashSe
collect_referenced_names_from_expr(index, referenced, interner);
collect_referenced_names_from_expr(value, referenced, interner);
}
Node::SubscriptDelete { target, index, .. } => {
referenced.insert(interner.get_str(target.name_id).to_string());
collect_referenced_names_from_expr(index, referenced, interner);
}
Node::AttrAssign { object, value, .. } => {
collect_referenced_names_from_expr(object, referenced, interner);
collect_referenced_names_from_expr(value, referenced, interner);
Expand Down
15 changes: 15 additions & 0 deletions crates/monty/src/types/dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,21 @@ impl PyTrait for Dict {
Ok(())
}

fn py_delitem(&mut self, key: Value, heap: &mut Heap<impl ResourceTracker>, interns: &Interns) -> RunResult<()> {
if let Some((old_key, old_value)) = self.pop(&key, heap, interns)? {
old_key.drop_with_heap(heap);
old_value.drop_with_heap(heap);
key.drop_with_heap(heap);

Ok(())
} else {
let err = ExcType::key_error(&key, heap, interns);
key.drop_with_heap(heap);

Err(err)
}
}

fn py_call_attr(
&mut self,
heap: &mut Heap<impl ResourceTracker>,
Expand Down
64 changes: 44 additions & 20 deletions crates/monty/src/types/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,24 @@ impl List {
let heap_id = heap.allocate(HeapData::List(Self::new(items)))?;
Ok(Value::Ref(heap_id))
}

fn try_derive_normalized_index(&self, index: i64) -> Option<usize> {
// Convert to usize, handling negative indices (Python-style: -1 = last element)
let len = i64::try_from(self.items.len()).expect("list length exceeds i64::MAX");

// branchless normalization using bit manipulation
// if index < 0: mask = -1 (all bits set), so we add len
// if index >= 0: mask = 0, so we add 0
let mask = index >> 63;
let normalized_index = index + (mask & len);
Comment on lines +193 to +194
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very harmless and branchless way of doing:

let normalized_index = if index < 0 { index + len } else { index };

The idea being:

  • if index < 0, then mask is -1 (all bits set)
  • if index >= 0, mask = 0, so we add 0


let in_bounds = normalized_index.cast_unsigned() < len.cast_unsigned();

// safety: if in_bounds is true, normalized_index is guaranteed
// to be in [0, len), so the cast is safe
#[expect(clippy::cast_sign_loss, clippy::cast_possible_truncation)]
in_bounds.then_some(normalized_index as usize)
}
}

impl From<List> for Vec<Value> {
Expand Down Expand Up @@ -220,18 +238,10 @@ impl PyTrait for List {
_ => return Err(ExcType::type_error_indices(Type::List, key.py_type(heap))),
};

// Convert to usize, handling negative indices (Python-style: -1 = last element)
let len = i64::try_from(self.items.len()).expect("list length exceeds i64::MAX");
let normalized_index = if index < 0 { index + len } else { index };

// Bounds check
if normalized_index < 0 || normalized_index >= len {
let Some(idx) = self.try_derive_normalized_index(index) else {
return Err(ExcType::list_index_error());
}
};

// Return clone of the item with proper refcount increment
// Safety: normalized_index is validated to be in [0, len) above
let idx = usize::try_from(normalized_index).expect("list index validated non-negative");
Ok(self.items[idx].clone_with_heap(heap))
}

Expand All @@ -254,18 +264,11 @@ impl PyTrait for List {
}
};

// Normalize negative indices (Python-style: -1 = last element)
let len = i64::try_from(self.items.len()).expect("list length exceeds i64::MAX");
let normalized_index = if index < 0 { index + len } else { index };

// Bounds check
if normalized_index < 0 || normalized_index >= len {
// Replace value, drop old one
let Some(idx) = self.try_derive_normalized_index(index) else {
value.drop_with_heap(heap);
return Err(ExcType::list_assignment_index_error());
}

// Replace value, drop old one
let idx = usize::try_from(normalized_index).expect("index validated non-negative");
};
let old_value = std::mem::replace(&mut self.items[idx], value);
old_value.drop_with_heap(heap);

Expand All @@ -278,6 +281,27 @@ impl PyTrait for List {
Ok(())
}

fn py_delitem(&mut self, key: Value, heap: &mut Heap<impl ResourceTracker>, _interns: &Interns) -> RunResult<()> {
// Extract integer index, accepting both Int and Bool (True=1, False=0)
let index = match key {
Value::Int(i) => i,
Value::Bool(b) => i64::from(b),
_ => {
let key_type = key.py_type(heap);
key.drop_with_heap(heap);
return Err(ExcType::type_error_list_assignment_indices(key_type));
}
};

let Some(idx) = self.try_derive_normalized_index(index) else {
return Err(ExcType::list_index_error());
};
let removed_value = self.items.remove(idx);
removed_value.drop_with_heap(heap);

Ok(())
}

fn py_eq(&self, other: &Self, heap: &mut Heap<impl ResourceTracker>, interns: &Interns) -> bool {
if self.items.len() != other.items.len() {
return false;
Expand Down
16 changes: 16 additions & 0 deletions crates/monty/src/types/py_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,20 @@ pub trait PyTrait {
)
.into())
}

/// Deletes an item from this value, e.g. `del obj[key]`
///
/// Returns Ok(()) on success, or an error if the key doesn't exist or deletion isn't supported
/// Takes ownership of `key` and drops it on error
///
/// The `interns` parameter provides access to interned string content
///
/// Default implementation returns TypeError
fn py_delitem(&mut self, _key: Value, heap: &mut Heap<impl ResourceTracker>, _interns: &Interns) -> RunResult<()> {
Err(SimpleException::new_msg(
ExcType::TypeError,
format!("'{}' object does not support item deletion", self.py_type(heap)),
)
.into())
}
}
13 changes: 13 additions & 0 deletions crates/monty/src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,19 @@ impl PyTrait for Value {
))),
}
}

fn py_delitem(&mut self, key: Self, heap: &mut Heap<impl ResourceTracker>, interns: &Interns) -> RunResult<()> {
match self {
Self::Ref(id) => {
let id = *id;
heap.with_entry_mut(id, |heap, data| data.py_delitem(key, heap, interns))
}
_ => Err(ExcType::type_error(format!(
"'{}' object does not support item deletion",
self.py_type(heap)
))),
}
}
}

impl Value {
Expand Down
22 changes: 22 additions & 0 deletions crates/monty/test_cases/del_subscript.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Test del operation on lists and dicts

x = [1, 2, 3, 4, 5]
del x[2]
assert x == [1, 2, 4, 5], f'Expected [1, 2, 4, 5], got {x}'

y = [10, 20, 30, 40]
del y[-1]
assert y == [10, 20, 30], f'Expected [10, 20, 30], got {y}'

z = ['a', 'b', 'c']
del z[0]
assert z == ['b', 'c'], f"Expected ['b', 'c'], got {z}"

d = {'a': 1, 'b': 2, 'c': 3}
del d['b']
assert d == {'a': 1, 'c': 3}, f"Expected {{'a': 1, 'c': 3}}, got {d}"
assert len(d) == 2, f'Expected length 2, got {len(d)}'

d2 = {1: 'one', 2: 'two', 3: 'three'}
del d2[2]
assert d2 == {1: 'one', 3: 'three'}, f"Expected {{1: 'one', 3: 'three'}}, got {d2}"
36 changes: 36 additions & 0 deletions crates/monty/test_cases/del_subscript_errors.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Test error cases for del operation

try:
x = [1, 2, 3]
del x[10]
assert False, 'Should have raised IndexError'
except IndexError:
print('IndexError for out of bounds - OK')

try:
y = [1, 2, 3]
del y[-10]
assert False, 'Should have raised IndexError'
except IndexError:
print('IndexError for negative out of bounds - OK')

try:
d = {'a': 1, 'b': 2}
del d['c']
assert False, 'Should have raised KeyError'
except KeyError:
print('KeyError for missing key - OK')

try:
num = 42
del num[0] # pyright: ignore[reportIndexIssue]
assert False, 'Should have raised TypeError'
except TypeError as e:
assert 'does not support item deletion' in str(e)

try:
lst = [1, 2, 3]
del lst['string']
assert False, 'Should have raised TypeError'
except TypeError as e:
assert 'indices' in str(e)