Fix test_runpy by youknowone · Pull Request #6409 · RustPython/RustPython

youknowone

Walkthrough

The pull request upgrades exit code handling from 8-bit to 32-bit across the codebase. A new Windows-aware utility function exit_code() is added to handle platform-specific exit code semantics. Interpreter and VM methods are updated to return u32, and call sites are refactored to use the new utility function for exit code conversion.

Changes

Cohort / File(s)	Summary
Core exit code utility `crates/common/src/os.rs`	Added public `exit_code(code: u32) -> ExitCode` function with Windows-specific handling: truncates to `u8` and calls `std::process::exit()` if code exceeds `u8::MAX`; otherwise returns `ExitCode::from()` on all platforms.
Interpreter and VM exit code types `crates/vm/src/vm/interpreter.rs`, `crates/vm/src/vm/mod.rs`	Updated `Interpreter::run()` and `Interpreter::finalize()` return types from `u8` to `u32`. Updated `handle_exit_exception()` return type from `u8` to `u32`, changed exit code conversion to `to_u32()`, updated signal calculation to use `u32` arithmetic, and added Windows-specific branch returning `STATUS_CONTROL_C_EXIT (0xC000013A)` for keyboard interrupt.
Call site migrations `examples/generator.rs`, `examples/package_embed.rs`, `src/lib.rs`	Replaced `ExitCode::from(...)` with `vm::common::os::exit_code(...)` for exit code construction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Windows-specific exit code semantics in crates/vm/src/vm/mod.rs: verify the STATUS_CONTROL_C_EXIT constant and keyboard interrupt handling align with CPython behavior
Type propagation across interpreter/VM layers: confirm all u32 conversions are intentional and no silent truncation occurs
Exit code truncation logic in crates/common/src/os.rs: ensure the fallback for codes exceeding u8::MAX correctly handles all platforms

Possibly related PRs

#6355: Introduces waitstatus_to_exitcode Windows variant and overlaps on exit code handling upgrades to support larger exit codes with platform-specific semantics.

Suggested reviewers

ShaharNaveh

Poem

🐰 A rabbit hops through code so fine,
Exit codes now reach past eight in line,
With Windows wisdom and u32 grace,
We handle signals at a larger place! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix test_runpy' is vague and generic, using non-descriptive terms that don't convey the actual scope of changes made to the codebase.	Provide a more specific title that describes the actual changes, such as 'Extend exit code handling to support u32' or 'Implement platform-specific exit code semantics' to better reflect the substantial refactoring across multiple modules.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ff1fba and 73a86c1.

⛔ Files ignored due to path filters (2)

Lib/test/test_runpy.py is excluded by !Lib/**
Lib/test/test_signal.py is excluded by !Lib/**

📒 Files selected for processing (6)

crates/common/src/os.rs (1 hunks)
crates/vm/src/vm/interpreter.rs (2 hunks)
crates/vm/src/vm/mod.rs (2 hunks)
examples/generator.rs (1 hunks)
examples/package_embed.rs (1 hunks)
src/lib.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/vm/src/vm/interpreter.rs
src/lib.rs
examples/generator.rs
crates/vm/src/vm/mod.rs
examples/package_embed.rs
crates/common/src/os.rs

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features

📚 Learning: 2025-11-29T12:17:28.606Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations

Applied to files:

src/lib.rs

📚 Learning: 2025-12-09T08:46:58.660Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

Applied to files:

crates/vm/src/vm/mod.rs

🧬 Code graph analysis (4)

src/lib.rs (1)

crates/common/src/os.rs (1)

exit_code (10-22)

examples/generator.rs (1)

crates/common/src/os.rs (1)

exit_code (10-22)

crates/vm/src/vm/mod.rs (2)

crates/vm/src/exceptions.rs (2)

args (568-570)

args (574-574)

crates/vm/src/builtins/int.rs (1)

as_bigint (265-267)

examples/package_embed.rs (1)

crates/common/src/os.rs (1)

exit_code (10-22)

🔇 Additional comments (8)

src/lib.rs (1)

114-114: LGTM!

The switch to the centralized exit_code utility correctly handles the widened u32 exit code from interp.run() with proper platform-specific semantics.

examples/generator.rs (1)

49-49: LGTM!

Consistent use of the centralized exit_code utility for proper platform-specific exit code handling.

examples/package_embed.rs (1)

29-29: LGTM!

Consistent application of the exit_code utility across all examples.

crates/vm/src/vm/interpreter.rs (2)

99-105: LGTM!

The return type widening to u32 correctly propagates the full exit code range needed for Windows-specific codes like STATUS_CONTROL_C_EXIT.

116-135: LGTM!

The finalize method correctly returns the widened u32 exit code from handle_exit_exception.
crates/vm/src/vm/mod.rs (2)
844-852: LGTM!

The return type change to u32 and corresponding to_u32() call correctly handle the wider exit code range. The fallback to 0 for out-of-range values matches Python's behavior.

886-896: Correct platform-specific KeyboardInterrupt handling.

The Windows-specific STATUS_CONTROL_C_EXIT (0xC000013A) matches CPython's behavior. The Unix path correctly computes signal + 128 with proper u32 arithmetic.
#!/bin/bash
# Verify STATUS_CONTROL_C_EXIT constant is used correctly
# Check if there are any other references to this constant in the codebase
rg -n "STATUS_CONTROL_C_EXIT|0xC000013A" --type rust
crates/common/src/os.rs (1)

10-22: Reasonable workaround for Windows exit code semantics.

The side-effect-based design (calling std::process::exit() directly for large codes) is acknowledged via the FIXME comment. The cast code as i32 correctly wraps large u32 values like STATUS_CONTROL_C_EXIT (0xC000013A) to their signed representation, which Windows expects. ExitCode::from_raw() remains unstable in Rust 1.79 and later, so this workaround is still necessary.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}