Newtype SigalNum by ShaharNaveh · Pull Request #8096 · RustPython/RustPython

ShaharNaveh

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: adf9e996-f085-4257-bba4-65dd8e6417a7

📥 Commits

Reviewing files that changed from the base of the PR and between a7a355a and bbf319b.

📒 Files selected for processing (2)

crates/vm/src/stdlib/_signal.rs
crates/vm/src/stdlib/_thread.rs

🚧 Files skipped from review as they are similar to previous changes (2)

crates/vm/src/stdlib/_thread.rs
crates/vm/src/stdlib/_signal.rs

📝 Walkthrough

Walkthrough

Introduces a SignalNum validated newtype replacing raw i32 signal numbers across the VM signal layer, stdlib _signal module, and _thread module. SignalHandlers is refactored to index by SignalNum via a new SignalHandlersInner type. Host env safety docs are clarified and is_valid_signal is removed.

Changes

SignalNum wrapper and signal API refactor

Layer / File(s)	Summary
host_env safety doc updates and `is_valid_signal` removal `crates/host_env/src/signal.rs`	Expands `# Safety` docs on `probe_handler` and `install_handler` to clarify caller requirements for valid signal numbers and ABI-accepted handlers, extends `pidfd_send_signal` to Android, and removes the Windows-only `is_valid_signal(signalnum: i32) -> bool` helper.
`SignalNum` type definition, conversions, and core VM signal handling `crates/vm/src/signal.rs`	Defines the `SignalNum` newtype with `VALID_RANGE`, `SIGINT`, unchecked constructor, and conversion impls (`From`, `TryFrom`, `TryFromObject`, `Display`); refactors `SignalHandlers` to wrap `SignalHandlersInner` with `Index`/`IndexMut` keyed by `SignalNum`; updates module imports, `trigger_signals` dispatch loop, and `set_interrupt_ex` to accept and use `SignalNum`.
stdlib `_signal`: imports, constants, and initialization with `SignalNum` `crates/vm/src/stdlib/_signal.rs`	Updates imports to bring `SignalNum` into scope, removes `SIGNUM_RANGE` constant, adds assertions on `SignalNum::VALID_RANGE`, rewrites `init_signal_handlers` to iterate `SignalNum::VALID_RANGE` and install SIGINT via `SignalNum::SIGINT`, and updates itimer error construction to use new error API.
stdlib `_signal`: `signal`/`getsignal` and error handling `crates/vm/src/stdlib/_signal.rs`	Reworks `signal` and `getsignal` Python-exposed entry points to accept `SignalNum`, updating handler-table indexing and host installation; simplifies return types (`getsignal` no longer returns `PyResult`); wraps `pause` in `vm.allow_threads`; refines `set_wakeup_fd` error message.
stdlib `_signal`: platform-specific and utility functions `crates/vm/src/stdlib/_signal.rs`	Updates `pidfd_send_signal`, `siginterrupt`, `raise_signal`, and `strsignal` to accept `SignalNum`; updates `sigset_to_pyset` and `pthread_sigmask` range filtering to use `SignalNum::VALID_RANGE` with corrected bounds formatting.
`_thread::interrupt_main` updated to `SignalNum` `crates/vm/src/stdlib/_thread.rs`	Adds conditional import of `SignalNum`, refactors `TIMEOUT_MAX_IN_MICROSECONDS` to use runtime `cfg!(...)` expression, changes `interrupt_main` to accept `OptionalArg<SignalNum>` defaulting to `SignalNum::SIGINT`, and calls `set_interrupt_ex(sig)` without passing the VM.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

RustPython/RustPython#8089: Introduces and uses the SignalHandlers newtype in crates/vm/src/signal.rs, directly overlapping with this PR's SignalHandlersInner refactor.
RustPython/RustPython#7886: Both PRs modify pidfd_send_signal signal-handling and error propagation in crates/host_env/src/signal.rs.
RustPython/RustPython#8076: Modifies signal-number range/iteration and signal API behavior in crates/vm/src/stdlib/_signal.rs, the same file this PR updates with SignalNum::VALID_RANGE.

Suggested reviewers

coolreader18
youknowone

🐇 A number once bare as i32,
Now wrapped in SignalNum, safe to the core.
No raw ints shall pass through the gate,
The handler installs in a type-checked state.
Hop hop! The signals align just right. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title references the main change (introducing a SignalNum newtype), but contains a typo ('Sigal' instead of 'Signal') that makes it unclear and less professional.	Correct the typo to 'Newtype SignalNum' for clarity and accuracy.

✅ Passed checks (4 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%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}