Make `Heap::inc_ref` only require immutable heap borrow (#193) by Butch78 · Pull Request #4 · Butch78/monty

Butch78 · 2026-03-09T22:06:05Z

No description provided.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Updated math module's create_module and call functions to use the new VM parameter pattern from upstream, while preserving the math module additions from our branch. https://claude.ai/code/session_01JfGrtZj2hMoSk1tDu1AypD

Copilot

Pull request overview

Refactors VM/heap APIs to reduce mutable borrowing requirements, while expanding runtime “pause points” (name lookup, function calls, OS calls) and adding initial re/math module plumbing.

Changes:

Updated many builtins/modules to take &mut VM instead of separate heap/interns/print parameters, aligning with new call/result flow (CallResult).
Added new execution pause semantics (e.g., name lookup snapshots) and updated Python/JS bindings + tests accordingly.
Introduced re module support (incl. exception type mapping) plus new opcodes/utilities and additional stdlib/typeshed entries.

Reviewed changes

Copilot reviewed 111 out of 241 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
crates/monty/src/parse.rs	Adds AST support for augmented subscript assignment node.
crates/monty/src/os.rs	Updates docs to reference `CallResult::OsCall`.
crates/monty/src/namespace.rs	Removes namespace storage impl; keeps `NamespaceId` docs/definition.
crates/monty/src/modules/typing.rs	Switches module creation API to receive `&mut VM`.
crates/monty/src/modules/sys.rs	Switches module creation API to receive `&mut VM`.
crates/monty/src/modules/pathlib.rs	Switches module creation API to receive `&mut VM`.
crates/monty/src/modules/os.rs	Migrates OS module to `CallResult` and `&mut VM` module creation.
crates/monty/src/modules/mod.rs	Adds `math`/`re` modules and updates module dispatch to `VM` + `CallResult`.
crates/monty/src/modules/asyncio.rs	Migrates asyncio module call results from `AttrCallResult` to `CallResult`.
crates/monty/src/lib.rs	Splits heap internals into new modules and updates public re-exports.
crates/monty/src/io.rs	Changes `PrintWriter::Collect` to borrow a buffer; adds `reborrow()`.
crates/monty/src/intern.rs	Adds many `StaticStrings` for `math`/`re`; replaces external-function id lookup with string-id reverse lookup helper.
crates/monty/src/fstring.rs	Removes `DepthGuard` from string formatting path; updates `py_str` usage.
crates/monty/src/expressions.rs	Introduces `Node::SubscriptOpAssign` AST node.
crates/monty/src/exception_private.rs	Adds `re.PatternError` support; migrates exception calling and getattr handling to new VM/call-result flow.
crates/monty/src/bytecode/vm/scheduler.rs	Updates task frame serialization and cancellation/cleanup logic for new locals model.
crates/monty/src/bytecode/vm/format.rs	Removes `DepthGuard`, adds pre-checks for huge format widths, updates dropping APIs.
crates/monty/src/bytecode/vm/exceptions.rs	Fixes handler unwind depth to include inline locals; updates drop calls.
crates/monty/src/bytecode/vm/compare.rs	Refactors comparison operations to new `py_eq`/`py_cmp` signatures and immediate cloning approach.
crates/monty/src/bytecode/vm/binary.rs	Adds dict-view + set operator special handling and refactors binary op dispatch.
crates/monty/src/bytecode/vm/attr.rs	Switches getattr to pass `EitherStr` to `py_getattr`.
crates/monty/src/bytecode/op.rs	Adds new opcodes (`LoadCallable`, `Dup2`, `DeleteGlobal`) and stabilizes serialized opcode tests.
crates/monty/src/bytecode/mod.rs	Re-exports `CallResult` for internal use.
crates/monty/src/bytecode/builder.rs	Adds emitters for new callable-load opcodes.
crates/monty/src/builtins/zip.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/type_.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/sum.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/sorted.rs	Migrates builtin to accept `&mut VM`, refactors arg parsing to use `VM`.
crates/monty/src/builtins/round.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/reversed.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/repr.rs	Migrates builtin to accept `&mut VM`, removes `DepthGuard`.
crates/monty/src/builtins/print.rs	Migrates builtin printing to use `vm.print_writer` and new `py_str` API.
crates/monty/src/builtins/pow.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/ord.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/oct.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/next.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/mod.rs	Enables `filter`/`getattr` builtins and migrates all builtins to `&mut VM` call shape.
crates/monty/src/builtins/min_max.rs	Migrates builtin to accept `&mut VM` and removes `DepthGuard` usage.
crates/monty/src/builtins/map.rs	Updates iterator creation to use `VM`.
crates/monty/src/builtins/len.rs	Migrates builtin to accept `&mut VM` and uses `py_len(vm)`.
crates/monty/src/builtins/isinstance.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/id.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/hex.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/hash.rs	Migrates builtin to accept `&mut VM` and updated hashing API.
crates/monty/src/builtins/getattr.rs	Adds new `getattr()` builtin implementation.
crates/monty/src/builtins/filter.rs	Adds new `filter()` builtin implementation.
crates/monty/src/builtins/enumerate.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/divmod.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/chr.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/bin.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/any.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/all.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/builtins/abs.rs	Migrates builtin to accept `&mut VM`.
crates/monty/src/asyncio.rs	Updates coroutine locals model/documentation and removes stored frame cells.
crates/monty/src/args.rs	Generalizes `DropWithHeap` to `ContainsHeap` for VM-centric dropping.
crates/monty/benches/main.rs	Updates `MontyRun::new` signature usage.
crates/monty/Cargo.toml	Adds `fancy-regex` dependency for `re` module work.
crates/monty-typeshed/update.py	Adds `re.pyi` and `math.pyi` to minimal typeshed, updates VERSIONS content.
crates/monty-type-checking/tests/good_types.py	Adds `re` typing assertions and imports `Any`.
crates/monty-python/tests/test_type_check.py	Removes `external_functions` constructor arg usage.
crates/monty-python/tests/test_threading.py	Updates snapshot type names and removes `external_functions` constructor arg usage.
crates/monty-python/tests/test_serialize.py	Updates snapshot type names and removes `external_functions` constructor arg usage.
crates/monty-python/tests/test_re.py	Adds runtime tests for new `re` module support and resume/persistence behavior.
crates/monty-python/tests/test_external.py	Updates external-call error semantics and constructor args.
crates/monty-python/tests/test_dataclasses.py	Updates snapshot type name and adds tests for private-field handling.
crates/monty-python/tests/test_basic.py	Updates `__repr__` expectations after constructor API change.
crates/monty-python/tests/test_async.py	Updates async snapshot type names and missing-function error semantics.
crates/monty-python/src/lib.rs	Exposes new snapshot classes from Python bindings.
crates/monty-python/src/external.rs	Switches to new `ExtFunctionResult` and “not found” handling.
crates/monty-python/src/exceptions.rs	Adds conversion path for `re.PatternError`/`re.error`.
crates/monty-python/src/dataclass.rs	Skips private dataclass fields during serialization.
crates/monty-python/src/convert.rs	Converts callable Python objects to Monty function objects; improves type error messages.
crates/monty-python/python/pydantic_monty/init.py	Updates exported snapshot types and adds name-lookup/method-call handling for async runner.
crates/monty-python/example.py	Updates usage to remove constructor `external_functions`.
crates/monty-python/README.md	Updates docs to new snapshot names and constructor signature.
crates/monty-js/wrapper.ts	Adds `MontyNameLookup` wrapper and updates async runner + print callback support.
crates/monty-js/src/lib.rs	Exports new name-lookup types and updates docs example.
crates/monty-js/src/convert.rs	Converts JS functions to Monty function objects; renders function objects as strings on output.
crates/monty-js/smoke-test/test.ts	Updates smoke tests for removed constructor external-functions declaration.
crates/monty-js/test/type_check.spec.ts	Updates type-check constructor options.
crates/monty-js/test/start.spec.ts	Adds name-lookup tests and updates existing start/resume tests.
crates/monty-js/test/serialize.spec.ts	Adds name-lookup dump/load tests and updates snapshot serialization tests.
crates/monty-js/test/print.spec.ts	Updates print tests for removed constructor external-functions declaration.
crates/monty-js/test/external.spec.ts	Updates external-call error semantics and removed constructor declaration.
crates/monty-js/test/exceptions.spec.ts	Adds OS-call error tests for JS bindings.
crates/monty-js/test/basic.spec.ts	Updates constructor expectations and repr tests after API changes.
crates/monty-js/test/async.spec.ts	Updates async runner tests for new name-lookup semantics and adds print callback tests.
crates/monty-js/README.md	Updates docs for removed constructor external-functions declaration.
crates/monty-cli/src/main.rs	Adds configurable resource limits and updates runner API usage and pause-point handling.
crates/fuzz/fuzz_targets/tokens_input_panic.rs	Updates `MontyRun::new` and `run` signatures.
crates/fuzz/fuzz_targets/string_input_panic.rs	Updates `MontyRun::new` and `run` signatures.
README.md	Updates docs/examples for new snapshot types and removed constructor external-functions.
CLAUDE.md	Updates macro examples and heap/refcount guidance.
.github/workflows/ci.yml	Removes `.cargo/config.toml` usage in CI and tweaks docker command formatting.
.claude/settings.json	Allows `mkdir` shell command in Claude tooling settings.

Files not reviewed (1)

crates/monty-js/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-09T22:09:21Z

-                    if k_needs_drop {
-                        k_value.drop_with_heap(this.heap);
-                    }
+                    let is_equal = v.py_eq(k_value, this.heap, this.interns)?;


py_eq is called with k_value by value here, but elsewhere (compare_eq) it’s called with a borrowed RHS. If py_eq expects &Value (which the surrounding code strongly suggests), this should pass &k_value to avoid a type mismatch / unintended move. Concretely: change the call to borrow k_value in the equality check.

Suggested change

let is_equal = v.py_eq(k_value, this.heap, this.interns)?;

let is_equal = v.py_eq(&k_value, this.heap, this.interns)?;

Copilot · 2026-03-09T22:09:22Z

+        Err(e) => {
+            if let Some(d) = default {
+                Ok(d.clone_with_heap(heap))
+            } else {
+                Err(e)
+            }
+        }


With a default, this returns the default for any error from py_getattr, not just “attribute missing”. In CPython, getattr(obj, name, default) only returns default when an AttributeError occurs; other exceptions raised during attribute access must propagate. Suggestion: only fall back to default when e is RunError::Exc and the exception type is AttributeError; otherwise return Err(e).

Copilot · 2026-03-09T22:09:22Z

+/// getattr(obj, 'y', None)       # Get obj.y or None if not found
+/// getattr(module, 'function')   # Get module.function
+/// ```
+pub fn builtin_getattr(vm: &mut VM<'_, '_, impl ResourceTracker>, args: ArgValues) -> RunResult<Value> {


This adds a new builtin with non-trivial error semantics (default handling, rejecting non-CallResult::Value). There doesn’t appear to be a corresponding runtime test added alongside other Python binding tests in this diff. Adding tests covering: missing attribute with default, missing attribute without default, and propagation of non-AttributeError exceptions would help lock in the intended behavior.

Copilot · 2026-03-09T22:09:22Z

+/// filter(lambda x: x > 0, [-1, 0, 1, 2])  # [1, 2]
+/// filter(None, [0, 1, False, True, ''])   # [1, True]
+/// ```
+pub fn builtin_filter(vm: &mut VM<'_, '_, impl ResourceTracker>, args: ArgValues) -> RunResult<Value> {


This introduces a new filter() builtin, but this diff doesn’t add any direct runtime tests for it (unlike the new re tests). Consider adding tests for filter(None, ...) truthiness behavior and for predicate-call behavior (including predicate raising exceptions) to ensure correct VM pause/resume and refcount-dropping behavior.

Copilot · 2026-03-09T22:09:23Z

    'dataclasses.pyi',
    # used by dataclasses
    'enum.pyi',
+    # the re std lib module is not mostly implemented


The sentence reads ungrammatically; “not mostly implemented” is confusing. Consider changing it to “not fully implemented” (or “only partially implemented”) to clarify intent.

Suggested change

# the re std lib module is not mostly implemented

# the re std lib module is only partially implemented

Copilot · 2026-03-09T22:09:23Z

+# DO NOT EDIT THIS FILE DIRECTLY
+# instead edit crates/monty-typeshed/update.py
+# this file should match the modules
+# which monty's minimimal typeshed includes


Typo in “minimimal”. This should be “minimal”.

Suggested change

# which monty's minimimal typeshed includes

# which monty's minimal typeshed includes

Copilot · 2026-03-09T22:09:23Z

+    pub fn get_string_id_by_name(&self, s: &str) -> Option<StringId> {
+        // Check single ASCII char
+        if s.len() == 1 {
+            return Some(StringId::from_ascii(s.as_bytes()[0]));
+        }
+        // Check static strings
+        if let Ok(ss) = StaticStrings::from_str(s) {
+            return Some(ss.into());
+        }
+        // Check interned strings
+        for (i, interned) in self.strings.iter().enumerate() {
+            if interned == s {
+                return u32::try_from(INTERN_STRING_ID_OFFSET + i).ok().map(StringId);
+            }
+        }
+        None
    }


This reverse lookup is O(n) over all interned strings, which can become a hot path if name resolution happens frequently (e.g., repeated NameLookup resumes). A tangible improvement is to maintain a HashMap<String, StringId> (or a parallel interner index) for reverse lookups, populated during interning, so this becomes O(1) average-case.

Copilot · 2026-03-09T22:09:23Z

+    /// This evaluates the container and index exactly once, then performs the
+    /// inplace operation on the current item before storing the result back.
+    /// Limiting duplicate evaluation is important because index expressions may
+    /// have side effects and CPython only evaluates them once.


The docstring claims both “container and index” are evaluated exactly once, but the node shape (target: Identifier) implies the container is restricted to a plain identifier (no side effects to repeat). If this is intentionally limited, consider clarifying that only the index expression is side-effectful in Monty’s current model (or, if broader support is intended, consider changing target to an ExprLoc/expression form in a follow-up).

Suggested change

/// This evaluates the container and index exactly once, then performs the

/// inplace operation on the current item before storing the result back.

/// Limiting duplicate evaluation is important because index expressions may

/// have side effects and CPython only evaluates them once.

/// The container here is always a plain identifier (e.g., `totals`), while the

/// index is a full expression which may have side effects.

/// The container identifier and index expression are each evaluated exactly once,

/// then the inplace operation is performed on the current item before storing

/// the result back. Avoiding duplicate evaluation is important because the index

/// expression may have side effects, and CPython only evaluates it once.

sathish-t and others added 5 commits March 9, 2026 14:31

Fix os.environ panic in JS bindings (pydantic#201)

a1b083d

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: David Hewitt <mail@davidhewitt.dev>

remove pointer indirection from PrintWriter inside VM (pydantic#258)

c01d6d2

convert more code to take vm instead of heap, interns (pydantic#260)

0bb5305

remove Namespaces; keep locals on the stack (pydantic#259)

508945d

Merge upstream/main and resolve conflicts in math module

8fdd0f7

Updated math module's create_module and call functions to use the new VM parameter pattern from upstream, while preserving the math module additions from our branch. https://claude.ai/code/session_01JfGrtZj2hMoSk1tDu1AypD

Copilot AI review requested due to automatic review settings March 9, 2026 22:06

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Butch78 changed the base branch from main to math-module March 9, 2026 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Heap::inc_ref` only require immutable heap borrow (#193)#4

Make `Heap::inc_ref` only require immutable heap borrow (#193)#4
Butch78 wants to merge 5 commits into
math-modulefrom
claude/pull-upstream-main-PCkzx

Butch78 commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	let is_equal = v.py_eq(k_value, this.heap, this.interns)?;
	let is_equal = v.py_eq(&k_value, this.heap, this.interns)?;

	# the re std lib module is not mostly implemented
	# the re std lib module is only partially implemented

	# which monty's minimimal typeshed includes
	# which monty's minimal typeshed includes

-    /// This evaluates the container and index exactly once, then performs the
-    /// inplace operation on the current item before storing the result back.
-    /// Limiting duplicate evaluation is important because index expressions may
-    /// have side effects and CPython only evaluates them once.
+    /// The container here is always a plain identifier (e.g., `totals`), while the
+    /// index is a full expression which may have side effects.
+    /// The container identifier and index expression are each evaluated exactly once,
+    /// then the inplace operation is performed on the current item before storing
+    /// the result back. Avoiding duplicate evaluation is important because the index
+    /// expression may have side effects, and CPython only evaluates it once.

Conversation

Butch78 commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants