crates/stdlib/src/_opcode.rs (3)

309-331: Drop the unused Result<(), ()> return type.

The ? on Line 324 operates inside the interp.enter closure (which returns PyResult<String>), and any Err is already converted to a panic by .unwrap() on Line 326. The outer function always returns Ok(()), so Result<(), ()> and the final Ok(()) are dead surface area.

Proposed simplification

-    fn dis_output(source: &str) -> Result<(), ()> {
+    fn dis_output(source: &str) {
         let builder = vm::Interpreter::builder(Default::default());
         let stdlib_defs = crate::stdlib_module_defs(&builder.ctx);
         let interp = builder.add_native_modules(&stdlib_defs).build();
 
         let output = interp
             .enter(|vm| {
                 let pycode = vm
-                    .compile(&source, Mode::Exec, "?".into())
-                    .map_err(|err| vm.new_syntax_error(&err, Some(&source)))
+                    .compile(source, Mode::Exec, "?".into())
+                    .map_err(|err| vm.new_syntax_error(&err, Some(source)))
                     .unwrap();
 
                 let dis_module = vm.import("dis", 0).unwrap();
                 let dis = dis_module.get_attr("dis", vm).unwrap();
 
                 dis.call((pycode,), vm)?.try_into_value::<String>(vm)
             })
             .unwrap();
 
         insta::assert_snapshot!(insta::internals::AutoName, output, source);
-
-        Ok(())
     }

(Also drops the unnecessary &source reborrow since source is already &str.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/_opcode.rs` around lines 309 - 331, The function dis_output
currently returns Result<(), ()> with a final Ok(()) and reborrows source;
simplify it by changing the signature of dis_output to return () (unit), remove
the final Ok(()) return, and stop reborrowing source when calling vm.compile
(use source instead of &source); keep the existing interp.enter closure and its
internal error handling as-is (the closure still returns PyResult<String> and
unwraps).

---

310-312: Avoid rebuilding a fresh Interpreter per test case.

The dis_output function has 8 #[test_case] invocations, each spinning up a new Interpreter, initializing the stdlib, and re-importing dis. This multiplies test runtime and can noticeably slow cargo test. Consider sharing an interpreter across cases via OnceLock/LazyLock:

Sketch

use std::sync::OnceLock;

fn shared_interp() -> &'static vm::Interpreter {
    static INTERP: OnceLock<vm::Interpreter> = OnceLock::new();
    INTERP.get_or_init(|| {
        let builder = vm::Interpreter::builder(Default::default());
        let stdlib_defs = crate::stdlib_module_defs(&builder.ctx);
        builder.add_native_modules(&stdlib_defs).build()
    })
}

Then call shared_interp().enter(|vm| { … }) from dis_output. Since VirtualMachine contains RefCell and UnsafeCell, verify thread-safety for parallel test execution; if not Sync, either use serial_test or pin tests to single-threaded execution.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/_opcode.rs` around lines 310 - 312, The tests rebuild an
Interpreter for every dis_output #[test_case], slowing test runs; create and
reuse a shared static interpreter (e.g., a shared_interp function backed by
std::sync::OnceLock or LazyLock that initializes vm::Interpreter via
vm::Interpreter::builder and crate::stdlib_module_defs once) and call
shared_interp().enter(|vm| { … }) inside dis_output instead of rebuilding;
verify whether the VM/VirtualMachine types are Sync/Send—if not, either run
those tests serially (use serial_test or #[test] with single-threaded execution)
or restrict the static to single-threaded use to avoid data races.

---

328-328: Remove unstable insta::internals::AutoName and use simplified public API.

The internals module is not part of insta's stable surface. With test_case descriptions driving the snapshot filename suffix, auto-naming works correctly using the simplified syntax:

-        insta::assert_snapshot!(insta::internals::AutoName, output, source);
+        insta::assert_snapshot!(output, source);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/_opcode.rs` at line 328, Replace the unstable internals
usage in the snapshot assertion: remove the first argument
`insta::internals::AutoName` from the `insta::assert_snapshot!` invocation so it
uses the public simplified API (i.e., call `insta::assert_snapshot!(output,
source)`), leaving the auto-naming behavior driven by the test_case description
intact.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/stdlib/src/_opcode.rs (1)

314-328: ⚠️ Potential issue | 🔴 Critical

Runtime panic: dis.dis() returns None, not a string — all test cases will fail.

dis.dis(code) (like CPython) prints to stdout and returns None. The try_into_value::<String>(vm) on Line 324 will therefore error, and the subsequent .unwrap() on Line 326 will panic for every parametrized case.

To capture the disassembly, redirect sys.stdout to an io.StringIO before the call and read getvalue() afterwards, or invoke a helper that returns a string (e.g., build the output via dis.Bytecode(code).dis() or similar).

Additionally, insta::internals::AutoName on Line 328 is a non-public API; prefer the public naming surface (e.g., passing the name explicitly or using assert_snapshot! without an explicit name so insta derives it from the function/test-case).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/_opcode.rs` around lines 314 - 328, The test currently
assumes dis.dis(code) returns a string and calls try_into_value::<String>(vm),
which panics because dis.dis prints to stdout and returns None; modify the
closure passed to interp.enter to capture the disassembly by redirecting
sys.stdout to an io.StringIO (or call a helper like dis.Bytecode(code).dis()
that returns a string), then read string_io.getvalue() and return that String
instead of trying to convert None; also replace the non-public
insta::internals::AutoName usage in the insta::assert_snapshot! invocation with
the public API (omit the explicit internal name so insta derives it or pass an
explicit name) so snapshots use supported naming.