Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 12, 2025

Summary by CodeRabbit

  • New Features

    • Enhanced signal handling module with improved cross-platform support for Windows and Unix systems.
  • Bug Fixes

    • Added validation for year values in time formatting to prevent errors on Windows, AIX, and Solaris when using year-based format specifiers.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Walkthrough

The PR extends the signal handling module with three new cross-platform utility functions (raise_signal, strsignal, valid_signals) featuring platform-specific implementations, modifies set_wakeup_fd to return i64 with Windows validation, and adds year validation for time formatting on Windows.

Changes

Cohort / File(s) Summary
Signal handling API expansion
crates/vm/src/stdlib/signal.rs
Changed set_wakeup_fd return type from PyResult<WakeupFdRaw> to PyResult<i64> with platform-specific handling and Windows runtime validation. Added three new public functions: raise_signal (validates and raises signals with platform restrictions), strsignal (returns signal name via libc on Unix, custom mapping on Windows), and valid_signals (returns PySet of valid signal numbers with platform-specific contents).
Time formatting validation
crates/vm/src/stdlib/time.rs
Added runtime guard in strftime to enforce year >= 1900 when using %y format on Windows, AIX, or Solaris, returning ValueError if violated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • signal.rs: New public API surface with three functions requiring careful review of platform-specific implementations (Unix vs. Windows variants), return type change implications for set_wakeup_fd, and validation logic differences.
  • time.rs: Simple validation guard with clear intent.
  • Areas needing attention:
    • Platform-specific behavior correctness in raise_signal, strsignal, and valid_signals
    • Impact of set_wakeup_fd return type change on callers and type safety
    • Windows fd validation logic in set_wakeup_fd
    • Completeness of signal mappings in Windows strsignal implementation

Suggested reviewers

  • ShaharNaveh

Poem

🐰 Signals now sing with cross-platform cheer,
Valid signals listed, crystal clear,
Wakeup file descriptors dance to i64,
Time guards the year—what could we ask for more?
—Rabbit the Reviewer, hopping through the code 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'impl {raise_,str}signal' accurately reflects the main changes, which introduce three new signal-handling functions: raise_signal, strsignal, and valid_signals.
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

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.

@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin signal

@youknowone youknowone force-pushed the signal branch 2 times, most recently from 8cab405 to b11c53b Compare December 12, 2025 07:09
@youknowone youknowone marked this pull request as ready for review December 12, 2025 07:58
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/signal.rs (1)

230-311: Windows: use safe i64 conversion for returned previous wakeup fd to prevent overflow

On Windows, old_fd is a usize (from AtomicUsize.swap()), and the direct cast Ok(old_fd as i64) at line 304 can silently overflow or truncate if the socket value exceeds i64::MAX. Use i64::try_from() with proper error handling instead.

         #[cfg(windows)]
         {
             if old_fd == INVALID_WAKEUP {
                 Ok(-1)
             } else {
-                Ok(old_fd as i64)
+                i64::try_from(old_fd)
+                    .map_err(|_| vm.new_value_error("previous wakeup fd does not fit in i64"))
             }
         }
🧹 Nitpick comments (2)
crates/vm/src/stdlib/time.rs (1)

354-381: Fix platform-specific error message + avoid false positives for literal %%y

  • The guard is compiled for Windows/AIX/Solaris (Line 365), but the message says “on Windows” (Line 367).
  • contains("%y") (Line 366) will also trip on literal %%y (should be treated as a literal %y, not a directive).
-        // On Windows/AIX/Solaris, %y format with year < 1900 is not supported
+        // On Windows/AIX/Solaris, %y with year < 1900 is not supported
         #[cfg(any(windows, target_os = "aix", target_os = "solaris"))]
-        if instant.year() < 1900 && format.as_str().contains("%y") {
-            let msg = "format %y requires year >= 1900 on Windows";
+        if instant.year() < 1900 && has_strftime_directive(format.as_str(), 'y') {
+            let msg = "format %y requires year >= 1900 on this platform";
             return Err(vm.new_value_error(msg.to_owned()));
         }
+
+        #[cfg(any(windows, target_os = "aix", target_os = "solaris"))]
+        fn has_strftime_directive(fmt: &str, directive: char) -> bool {
+            // minimal scan to ignore escaped percent (%%)
+            let mut it = fmt.chars().peekable();
+            while let Some(c) = it.next() {
+                if c != '%' {
+                    continue;
+                }
+                match it.next() {
+                    Some('%') | None => continue, // literal '%'
+                    Some(d) if d == directive => return true,
+                    Some(_) => continue,
+                }
+            }
+            false
+        }
crates/vm/src/stdlib/signal.rs (1)

325-358: raise_signal: prefer errno/last_os_error-based exception on failure (and dedupe Windows valid-signal list)

libc::raise() failure currently returns new_os_error("raise_signal failed ...") (Line 351) but doesn’t include the platform error detail. Also, the Windows allowed-signal list is duplicated with valid_signals.

         let res = unsafe { libc::raise(signalnum) };
         if res != 0 {
-            return Err(vm.new_os_error(format!("raise_signal failed for signal {}", signalnum)));
+            return Err(std::io::Error::last_os_error().into_pyexception(vm));
         }
📜 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 4bec0ad and 3e01e22.

⛔ Files ignored due to path filters (3)
  • Lib/test/test_regrtest.py is excluded by !Lib/**
  • Lib/test/test_signal.py is excluded by !Lib/**
  • Lib/test/test_strftime.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/vm/src/stdlib/signal.rs (4 hunks)
  • crates/vm/src/stdlib/time.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/stdlib/time.rs
  • crates/vm/src/stdlib/signal.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/signal.rs (3)
crates/common/src/crt_fd.rs (1)
  • try_borrow_raw (261-267)
crates/common/src/fileutils.rs (4)
  • fstat (11-21)
  • fstat (95-159)
  • std (316-324)
  • std (334-334)
crates/vm/src/vm/mod.rs (1)
  • check_signals (778-787)
🔇 Additional comments (2)
crates/vm/src/stdlib/signal.rs (2)

395-434: valid_signals: LGTM (platform-specific set contents match intent)

The platform split (Unix: 1..NSIG skipping SIGKILL/SIGSTOP; Windows: fixed subset; others: empty) is clear and matches the stated goals.


360-394: Review comment is incorrect; RustPython implementation already matches CPython behavior

CPython's signal.strsignal() raises ValueError for out-of-range/invalid signal numbers, not returns None. The function returns None only for valid signal numbers that lack a platform-specific description. RustPython's current implementation is correct and compatible with CPython; the suggested changes would introduce incompatibility.

Likely an incorrect or invalid review comment.

@youknowone youknowone merged commit df3a0b2 into RustPython:main Dec 12, 2025
13 checks passed
@youknowone youknowone deleted the signal branch December 12, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant