User-defined classes#515
Open
samuelcolvin wants to merge 3 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty/src/heap_data.rs">
<violation number="1" location="crates/monty/src/heap_data.rs:729">
P2: Bound methods are introduced as identity-equality values but are omitted from `py_hash` dispatch, making them unexpectedly unhashable. This breaks using `obj.method` as dict/set keys and is inconsistent with other identity-hashed callable objects.</violation>
</file>
<file name="crates/monty/src/bytecode/op.rs">
<violation number="1" location="crates/monty/src/bytecode/op.rs:748">
P2: BuildClass stack-effect arithmetic overflows for large but accepted class bodies. Either cap class member_count to the representable stack-effect range or compute/report this case before emitting the opcode.</violation>
</file>
<file name="crates/monty/src/value.rs">
<violation number="1" location="crates/monty/src/value.rs:1815">
P3: Update py_set_attr docs to include user-defined instances; current comment is now false and will mislead callers about supported setattr targets.</violation>
</file>
<file name="crates/monty/src/types/instance.rs">
<violation number="1" location="crates/monty/src/types/instance.rs:255">
P2: Bound-method attribute access leaks references when heap allocation fails. Handle the allocation error by dropping the owned BoundMethod values with the heap, or avoid taking extra refs until allocation succeeds.</violation>
</file>
<file name="crates/monty/src/bytecode/vm/collections.rs">
<violation number="1" location="crates/monty/src/bytecode/vm/collections.rs:71">
P2: Drop replaced class-member values when duplicate names overwrite earlier entries. Ignoring Dict::set's return leaks the old value's heap ownership/refcount for duplicate methods or class variables.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Self::Dataclass(dc) => dc.py_hash(self_id, vm), | ||
| // Classes and instances hash by identity. | ||
| Self::Class(class) => class.py_hash(self_id, vm), | ||
| Self::Instance(instance) => instance.py_hash(self_id, vm), |
Contributor
There was a problem hiding this comment.
P2: Bound methods are introduced as identity-equality values but are omitted from py_hash dispatch, making them unexpectedly unhashable. This breaks using obj.method as dict/set keys and is inconsistent with other identity-hashed callable objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/heap_data.rs, line 729:
<comment>Bound methods are introduced as identity-equality values but are omitted from `py_hash` dispatch, making them unexpectedly unhashable. This breaks using `obj.method` as dict/set keys and is inconsistent with other identity-hashed callable objects.</comment>
<file context>
@@ -691,6 +724,9 @@ impl<'h> PyTrait<'h> for HeapReadOutput<'h> {
Self::Dataclass(dc) => dc.py_hash(self_id, vm),
+ // Classes and instances hash by identity.
+ Self::Class(class) => class.py_hash(self_id, vm),
+ Self::Instance(instance) => instance.py_hash(self_id, vm),
Self::Range(r) => r.py_hash(self_id, vm),
Self::Slice(s) => s.py_hash(self_id, vm),
</file context>
Suggested change
| Self::Instance(instance) => instance.py_hash(self_id, vm), | |
| Self::Instance(instance) => instance.py_hash(self_id, vm), | |
| Self::BoundMethod(_) => { | |
| let mut hasher = DefaultHasher::new(); | |
| self_id.hash(&mut hasher); | |
| Ok(Some(HashValue::new(hasher.finish()))) | |
| }, |
| (LoadGlobalCallable, Operand::U16U16(..)) => 1, | ||
| // `BuildClass(name_const, member_count)` pops `2 * member_count` | ||
| // key/value pairs and pushes the class object. | ||
| (BuildClass, Operand::U16U16(_, member_count)) => 1 - 2 * member_count.cast_signed(), |
Contributor
There was a problem hiding this comment.
P2: BuildClass stack-effect arithmetic overflows for large but accepted class bodies. Either cap class member_count to the representable stack-effect range or compute/report this case before emitting the opcode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/bytecode/op.rs, line 748:
<comment>BuildClass stack-effect arithmetic overflows for large but accepted class bodies. Either cap class member_count to the representable stack-effect range or compute/report this case before emitting the opcode.</comment>
<file context>
@@ -733,6 +743,9 @@ impl Opcode {
(LoadGlobalCallable, Operand::U16U16(..)) => 1,
+ // `BuildClass(name_const, member_count)` pops `2 * member_count`
+ // key/value pairs and pushes the class object.
+ (BuildClass, Operand::U16U16(_, member_count)) => 1 - 2 * member_count.cast_signed(),
// === Jumps: fall-through effect (what the tracker absorbs after the bytes are written).
</file context>
| let mut namespace = Dict::new(); | ||
| let mut iter = items.into_iter(); | ||
| while let (Some(key), Some(value)) = (iter.next(), iter.next()) { | ||
| namespace.set(key, value, self)?; |
Contributor
There was a problem hiding this comment.
P2: Drop replaced class-member values when duplicate names overwrite earlier entries. Ignoring Dict::set's return leaks the old value's heap ownership/refcount for duplicate methods or class variables.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/bytecode/vm/collections.rs, line 71:
<comment>Drop replaced class-member values when duplicate names overwrite earlier entries. Ignoring Dict::set's return leaks the old value's heap ownership/refcount for duplicate methods or class variables.</comment>
<file context>
@@ -46,6 +48,34 @@ impl<T: ResourceTracker> VM<'_, T> {
+ let mut namespace = Dict::new();
+ let mut iter = items.into_iter();
+ while let (Some(key), Some(value)) = (iter.next(), iter.next()) {
+ namespace.set(key, value, self)?;
+ }
+
</file context>
Suggested change
| namespace.set(key, value, self)?; | |
| if let Some(old_value) = namespace.set(key, value, self)? { | |
| old_value.drop_with_heap(self); | |
| } |
| instance: Value::Ref(self_id), | ||
| func: member, | ||
| }; | ||
| let id = vm.heap.allocate(HeapData::BoundMethod(bound))?; |
Contributor
There was a problem hiding this comment.
P2: Bound-method attribute access leaks references when heap allocation fails. Handle the allocation error by dropping the owned BoundMethod values with the heap, or avoid taking extra refs until allocation succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/instance.rs, line 255:
<comment>Bound-method attribute access leaks references when heap allocation fails. Handle the allocation error by dropping the owned BoundMethod values with the heap, or avoid taking extra refs until allocation succeeds.</comment>
<file context>
@@ -0,0 +1,373 @@
+ instance: Value::Ref(self_id),
+ func: member,
+ };
+ let id = vm.heap.allocate(HeapData::BoundMethod(bound))?;
+ Ok(CallResult::Value(Value::Ref(id)))
+ } else {
</file context>
| old_value.drop_with_heap(vm); | ||
| Ok(()) | ||
| } | ||
| HeapReadOutput::Instance(mut instance) => { |
Contributor
There was a problem hiding this comment.
P3: Update py_set_attr docs to include user-defined instances; current comment is now false and will mislead callers about supported setattr targets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/value.rs, line 1815:
<comment>Update py_set_attr docs to include user-defined instances; current comment is now false and will mislead callers about supported setattr targets.</comment>
<file context>
@@ -1796,6 +1812,15 @@ impl Value {
old_value.drop_with_heap(vm);
Ok(())
}
+ HeapReadOutput::Instance(mut instance) => {
+ let name_value = match name {
+ EitherStr::Interned(string_id) => Self::InternString(*string_id),
</file context>
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Results 📊❌ Patch coverage is 0.12%. Project has 39124 uncovered lines. Files with missing lines (17)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 49.87% 49.70% -0.17%
==========================================
Files 304 308 +4
Lines 76360 77789 +1429
Branches 162849 165638 +2789
==========================================
+ Hits 38085 38665 +580
- Misses 38275 39124 +849
- Partials 3054 3086 +32Generated by Codecov Action |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Support `class Foo: ...` with instance methods, `__init__`, `__repr__`/`__str__`, and literal class variables. Methods are ordinary functions whose first parameter is `self`, so defaults/keyword args come for free. - Parse: new `Node::ClassDef`; reject inheritance/metaclasses, class/method decorators, and non-literal class bodies at parse time. - Prepare: class name binds in the enclosing scope; methods are prepared with `register_name=false` so class scope is skipped for free-var resolution while still capturing enclosing-function locals (matching CPython). - Compile: extract `emit_make_function`; new `BuildClass` opcode (appended). - Heap: new `Class`, `Instance`, `BoundMethod` types wired into HeapData, HeapReadOutput, GC walkers, `Type`, and PyTrait dispatch. - Instantiation: `Foo(...)` allocates the instance, runs `__init__(self, ...)` as a real (suspendable) frame flagged `is_initializer`; `ReturnValue` discards the `None` and leaves the instance. The flag round-trips through frame serialization so a suspended `__init__` resumes correctly. - repr/str dispatch to user `__repr__`/`__str__` via `evaluate_function`, intercepted at the `Value` level (where the heap id is available). - `type(obj)` returns the class object; `type(obj) is Foo` and `isinstance` work. Identity-only equality, always-truthy instances, no inheritance/other dunders yet; divergences documented in limitations/classes.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
samuelcolvin
force-pushed
the
impl-classes
branch
from
259bf57 to
7f9c698
Compare
Model the class body as a synthetic zero-arg function (like CPython's class-body code object): it runs the class statements top-to-bottom in its own scope, assembles the namespace, and returns the Class. This is confined to parse -> prepare -> compile; the heap types, BuildClass opcode, instantiation and builtins are unchanged. - Class variables may now be arbitrary expressions and reference earlier class variables (the literal-only restriction is removed). - Methods skip the class scope for free-var resolution: a bare member name resolves to a global/NameError, never a sibling member. - Class-body values may suspend on external/OS calls (real frame). - The same-name collision (an enclosing local and a class member sharing a name, captured by a method) is rejected with a clean NotImplementedError rather than miscompiling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty/src/prepare.rs">
<violation number="1" location="crates/monty/src/prepare.rs:2211">
P3: Move or rewrite the stale `FunctionScopeInfo` doc comment; it now attaches to `FinalizedScope` and misdocuments the closure-slot helper struct.</violation>
</file>
<file name="crates/monty/src/parse.rs">
<violation number="1" location="crates/monty/src/parse.rs:729">
P2: Class-var RHS named expressions can create class-body locals that are not recorded in `members`, so those bindings disappear from the class namespace. Reject named expressions in class RHS or add their targets to namespace assembly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| )); | ||
| }; | ||
| let ident = self.identifier(id, *name_range); | ||
| let object = self.parse_expression(*value)?; |
Contributor
There was a problem hiding this comment.
P2: Class-var RHS named expressions can create class-body locals that are not recorded in members, so those bindings disappear from the class namespace. Reject named expressions in class RHS or add their targets to namespace assembly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/parse.rs, line 729:
<comment>Class-var RHS named expressions can create class-body locals that are not recorded in `members`, so those bindings disappear from the class namespace. Reject named expressions in class RHS or add their targets to namespace assembly.</comment>
<file context>
@@ -708,14 +721,16 @@ impl<'a> Parser<'a> {
};
let ident = self.identifier(id, *name_range);
- class_vars.push((ident, self.parse_class_var_value(*value)?));
+ let object = self.parse_expression(*value)?;
+ members.push(ident);
+ body.push(Node::Assign { target: ident, object });
</file context>
| /// each captured name against the parent. These are exactly the fields the | ||
| /// compiler needs to emit `MakeFunction`/`MakeClosure` and install cells at | ||
| /// call time; see [`crate::function::Function`] for their meaning. | ||
| struct FinalizedScope { |
Contributor
There was a problem hiding this comment.
P3: Move or rewrite the stale FunctionScopeInfo doc comment; it now attaches to FinalizedScope and misdocuments the closure-slot helper struct.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/prepare.rs, line 2211:
<comment>Move or rewrite the stale `FunctionScopeInfo` doc comment; it now attaches to `FinalizedScope` and misdocuments the closure-slot helper struct.</comment>
<file context>
@@ -2016,6 +2201,20 @@ impl<'i, 'g> Prepare<'i, 'g> {
+/// each captured name against the parent. These are exactly the fields the
+/// compiler needs to emit `MakeFunction`/`MakeClosure` and install cells at
+/// call time; see [`crate::function::Function`] for their meaning.
+struct FinalizedScope {
+ free_var_enclosing_slots: Vec<NamespaceId>,
+ free_var_slots: Vec<NamespaceId>,
</file context>
Cover the bare-name NameError (a method referencing a class member by bare name), missing-attribute AttributeError, and an exception raised in __init__ with TRACEBACK tests so frames, line numbers and caret markers are verified against CPython. To keep byte-for-byte parity, the traceback harness now strips CPython's "Did you mean: '...'?" spelling suggestion, which Monty does not emit (its "Did you forget to import '...'" hint, produced by both, is kept). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
davidhewitt
reviewed
Jun 29, 2026
Comment on lines
+364
to
+371
| /// Whether this frame is a class `__init__` running for `Foo(...)`. | ||
| /// | ||
| /// When `true`, the `ReturnValue` handler discards the frame's return value | ||
| /// (`__init__` returns `None`) and leaves the instance — pushed onto the | ||
| /// caller's operand stack before this frame was created — as the result of the | ||
| /// construction. Threaded through serialization (`SerializedFrame`) so a | ||
| /// suspended initializer resumes correctly. | ||
| is_initializer: bool, |
Collaborator
There was a problem hiding this comment.
This seems like a suspicious special case; the way that this works in CPython is that __call__ for type objects is an outer layer which handles creating a new instance (e.g. via __new__ then calling __init__ on the new object)
Comment on lines
+523
to
+532
| /// Pop `2 * member_count` key/value pairs, build a class object, push it. | ||
| /// Operands: u16 const index of the class name (an interned string) + u16 | ||
| /// member count. | ||
| /// | ||
| /// The compiler emits, for a `class Foo: ...`, the `(name, value)` pairs for | ||
| /// each method and class variable, then this op pops them, builds the class | ||
| /// namespace dict, and wraps it in a [`HeapData::Class`](crate::heap::HeapData). | ||
| /// Stack: `[..., k1, v1, ..., kN, vN] -> [..., class]`. | ||
| /// Appended at the end to preserve the serialized byte values of all older opcodes. | ||
| BuildClass, |
Collaborator
There was a problem hiding this comment.
I don't think this needs to be a new opcode; if we implemented the multi-arg form to type() which dynamically creates new types, we could make the compiler emit codegen which calls that.
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.
Support
class Foo: ...with instance methods,__init__,__repr__/__str__, and literal class variables. Methods are ordinary functions whose first parameter isself, so defaults/keyword args come for free.Node::ClassDef; reject inheritance/metaclasses, class/method decorators, and non-literal class bodies at parse time.register_name=falseso class scope is skipped for free-var resolution while still capturing enclosing-function locals (matching CPython).emit_make_function; newBuildClassopcode (appended).Class,Instance,BoundMethodtypes wired into HeapData, HeapReadOutput, GC walkers,Type, and PyTrait dispatch.Foo(...)allocates the instance, runs__init__(self, ...)as a real (suspendable) frame flaggedis_initializer;ReturnValuediscards theNoneand leaves the instance. The flag round-trips through frame serialization so a suspended__init__resumes correctly.__repr__/__str__viaevaluate_function, intercepted at theValuelevel (where the heap id is available).type(obj)returns the class object;type(obj) is Fooandisinstancework.Identity-only equality, always-truthy instances, no inheritance/other dunders yet; divergences documented in limitations/classes.md.
Summary by cubic
Adds user-defined classes with a real, CPython-like class-body scope. Class variables can be any expressions (and reference earlier ones), class bodies and
__init__run as suspendable frames, and instances work withtype()/isinstance().Node::ClassDef; reject inheritance/metaclasses and class/method decorators; compile the class body as a synthetic zero-arg function that runs top-to-bottom.NotImplementedError.BuildClassopcode assembles(name, value)pairs into a class; calling a class allocates an instance and runs__init__as a real, suspendable frame (is_initializer, serialized for resume); bound methods passself.type(x)for instances returns the class object;isinstancesupports user classes; addedClass/Instance/BoundMethodandType::Instance;repr/strdispatch to user dunders.class__basic.py,class__scope.py,class__body_external.py,class__init_external.py,class__repr.py, plus TRACEBACK testsclass__name_error.py,class__attribute_error.py,class__init_raises.py,class__method_raises.py; updatedparse_errors.rs(simple classes compile; inheritance/decorators rejected),limitations/classes.mdandlimitations/language.md; traceback harness now strips CPython’s “Did you mean: '…'?” suffix for byte-for-byte parity.Written for commit c802ca9. Summary will update on new commits.