Open
Conversation
Implements the `del` statement across the full pipeline: parsing, prepare phase, bytecode compilation, and VM execution. Supports `del x` (variable unbinding), `del d['key']` (dict item removal), `del lst[i]` (list item removal), and multi-target `del a, b`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bles - `delete_local` and `delete_global` now raise errors when the target is already unbound (NameError or UnboundLocalError, matching CPython) - Fix `load_global` to raise NameError (not UnboundLocalError) for module-level variables — globals never produce UnboundLocalError in CPython - Reject `del` with slice targets at parse time (NotImplementedError) - Fix test syntax errors (statements on same line without separator) - Add tests for double-delete, missing global delete, error messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `DeleteCell` and `DeleteLocalW` opcodes (appended to preserve discriminant stability) so `del` works correctly on cell variables captured by closures and on local slots above 255. - `DeleteCell` stores `Value::Undefined` in the cell, making both the outer scope (UnboundLocalError) and inner closures (NameError) see the variable as unbound — matching CPython semantics for `del x` where `x` is captured, and `del` via `nonlocal` - `DeleteLocalW` is the wide variant of `DeleteLocal` for functions with more than 255 locals - `load_cell` now distinguishes owned cell vars (UnboundLocalError) from free vars (NameError) via `is_owned_cell_slot` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the parse-time rejection of slice targets in `del` statements. List `py_delitem` now handles slice keys by collecting the selected indices and removing them highest-first to preserve index validity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- TypeError: "doesn't" not "does not" for item deletion - IndexError: "list assignment index out of range" for del (matches setitem) - Fix rustfmt formatting in parse_errors test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…und locals `load_global` was changed to raise `NameError` for all assigned locals, but comprehension loop variables (also globals in Monty) need `UnboundLocalError`. Track explicitly deleted globals in a `HashSet` so `load_global` can distinguish "deleted via `del`" (NameError) from "unbound comprehension variable" (UnboundLocalError). Also removes unused `clear_assigned_local` method from `Code`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
|
@kmad unfortunately this seems to have had a meaningful impact on performance, can you figure out why, or do you need help? |
Author
|
Will look & revert back, sorry about that. |
The `HashSet<u16>` field added 48 bytes to the VM struct even when empty, hurting cache locality in the hot execution loop and causing ~14-17% regression on benchmarks that never use `del`. Replace with `Option<Box<HashSet<u16>>>` (8 bytes when None) and guard all accesses so the common path (no `del` used) pays zero cost: - `store_global`: skip remove when None - `load_global`: skip contains check when None - `delete_global`: lazily allocate on first use Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@samuelcolvin the performance regression has been resolved. |
| /// | ||
| /// Boxed and wrapped in `Option` to keep the VM struct small (8 bytes vs 48) | ||
| /// since `del` on globals is rare and the VM is accessed on every instruction. | ||
| deleted_globals: Option<Box<HashSet<u16>>>, |
Member
There was a problem hiding this comment.
I don't really understand the point of deleted_globals. suspect we can remove it.
Even if there are subtle differences between cpython and monty on this, it seems like a price worth paying.
Member
There was a problem hiding this comment.
If we really need this to be the same, maybe we could add a new variant to Value, e.g. Value::GlobalDeleted instead of adding this deleted_globals?
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
Member
There was a problem hiding this comment.
I'd rather not add tests in code unless absolutely necessary.
| // Wide variant not implemented yet | ||
| todo!("DeleteLocalW for slot > 255"); | ||
| if matches!(target.scope, NameScope::Local) { | ||
| self.code.register_assigned_local(slot); |
Member
There was a problem hiding this comment.
I'm not clear why this is necessary?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
delstatement supportImplements
delacross the full pipeline — parsing, prepare, bytecode compilation, and VMexecution.
What works
del x— unbinds variables (locals, globals, cells), subsequent access raisesNameErrororUnboundLocalErrorexactly as CPython doesdel d['key']— dict item removal via__delitem__del lst[i]— list item removal with negative index supportdel a, b— multi-target, executed left-to-rightdelon captured variables correctly propagates: outer scope getsUnboundLocalError, inner closures getNameErrorfor free variablesnonlocal+del— deleting vianonlocalunbinds the cell in the enclosing scopeNew opcodes
DeleteSubscr— pops index + container, calls__delitem__DeleteLocalW— wide variant ofDeleteLocalfor slots > 255DeleteCell— storesUndefinedin a closure cellAll three appended to the enum to preserve discriminant stability.
Bug fix (pre-existing)
load_globalwas raisingUnboundLocalErrorfor undefined module-level variables that hadpreviously been assigned. CPython always raises
NameErrorfor globals. Fixed.Not yet supported
del obj.attr— blocked on user-defined classesWritten in collaboration with claude-opus-4-6 and gpt-5.4-high