[5895] added code to raise error in sleep if invalid value #5907
Conversation
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant time_sleep
participant VirtualMachine
Caller->>time_sleep: sleep(secs: PyObjectRef, vm)
time_sleep->>time_sleep: Convert secs to f64 or i64
alt Conversion fails
time_sleep->>VirtualMachine: Raise TypeError
else Conversion succeeds
time_sleep->>time_sleep: Validate value (finite, non-negative)
alt Validation fails
time_sleep->>VirtualMachine: Raise ValueError
else Validation passes
time_sleep->>time_sleep: Convert to Duration
time_sleep->>time_sleep: Thread sleep for Duration
alt Unix platform and interrupted
time_sleep->>VirtualMachine: vm.check_signals()
end
time_sleep-->>Caller: Ok(())
end
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! 10000 It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Summary of Changes
Hello @DominicPrince2003, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the robustness of the time.sleep() function by implementing comprehensive input validation. It ensures that the duration argument is a valid, finite, non-negative number, raising specific Python exceptions for invalid inputs. This change improves the reliability and predictability of the sleep function's behavior.
Highlights
- Input Validation: The
sleep()function now performs robust input validation. It ensures that the provided duration is a valid number (float or integer) and falls within an acceptable range (non-negative, finite, and not exceedingu64::MAX). - Error Handling: Appropriate
TypeErrorandValueErrorexceptions are raised whensleep()receives invalid input, aligning its behavior with standard Python error handling for such cases. - Cross-platform Consistency: The new validation logic has been applied to both the generic
sleepimplementation (for non-Unix systems) and the platform-specificsleepimplementation (likely for Unix systems usinglibc::nanosleep), ensuring consistent behavior across different environments.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds input validation to time.sleep() to handle non-numeric, negative, and non-finite values, which is a great improvement.
My review includes a few points:
- There's some code duplication between the
unixandnon-uniximplementations ofsleep. Extracting the validation logic to a helper function would improve maintainability. - The
uniximplementation has a minor indentation issue. - The upper bound check for the sleep duration on
non-unixplatforms might not be accurate for Windows, potentially leading to silent truncation of the sleep time.
Overall, good work on improving the robustness of this function.
vm/src/stdlib/time.rs
Outdated
| // Try to get as float first, if that fails try as integer | ||
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | ||
| float_val | ||
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | ||
| int_val as f64 | ||
| } else { | ||
| return Err(vm.new_type_error("sleep() argument must be a number")); | ||
| }; | ||
| if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 { | ||
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | ||
| } |
There was a problem hiding this comment.
This validation logic is duplicated in the unix implementation of sleep as well (lines 710-720). To improve maintainability and avoid code duplication, consider extracting this logic into a shared helper function. This function could be defined at the module level and be called by both sleep implementations.
For example:
fn get_sleep_secs(secs_obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<f64> {
// Try to get as float first, if that fails try as integer
let secs = if let Ok(float_val) = f64::try_from_object(vm, secs_obj.clone()) {
float_val
} else if let Ok(int_val) = i64::try_from_object(vm, secs_obj) {
int_val as f64
} else {
return Err(vm.new_type_error("sleep() argument must be a number"));
};
if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 <
10000
span class="pl-kos">{
return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
}
Ok(secs)
}There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_time.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/stdlib/time.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/stdlib/time.rs
🧠 Learnings (1)
vm/src/stdlib/time.rs (3)
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
🔇 Additional comments (2)
vm/src/stdlib/time.rs (2)
544-544: LGTM: Import addition is necessary.The
TryFromObjectimport is required for the new sleep function implementation that converts Python objects to numeric types.
95-101: Good implementation of Python object conversion.The two-stage conversion approach (f64 first, then i64) is well-designed as it preserves fractional seconds while still accepting integer inputs. This matches Python's expected behavior.
Also applies to: 711-717
vm/src/stdlib/time.rs
Outdated
| fn sleep(secs: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | ||
| // this is basically std::thread::sleep, but that catches interrupts and we don't want to; | ||
|
|
||
| // Try to get as float first, if that fails try as integer | ||
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | ||
| float_val | ||
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | ||
| int_val as f64 | ||
| } else { | ||
| return Err(vm.new_type_error("sleep() argument must be a number")); | ||
| }; | ||
| if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 { | ||
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | ||
| } | ||
| let dur = Duration::from_secs_f64(secs); |
There was a problem hiding this comment.
Fix potential precision issue with u64::MAX conversion (Unix implementation).
Same precision issue exists in the Unix implementation as in the non-Unix version.
Apply the same fix as suggested for the non-Unix implementation:
- if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 {
- return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
- }
+ if !secs.is_finite() {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }
+ if secs < 0.0 {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }
+ // Check for values that would overflow when converted to Duration
+ if secs > Duration::MAX.as_secs_f64() {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn sleep(secs: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | |
| // this is basically std::thread::sleep, but that catches interrupts and we don't want to; | |
| // Try to get as float first, if that fails try as integer | |
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | |
| float_val | |
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | |
| int_val as f64 | |
| } else { | |
| return Err(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| let dur = Duration::from_secs_f64(secs); | |
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | |
| float_val | |
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | |
| int_val as f64 | |
| } else { | |
| return Err(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite() { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| if secs < 0.0 { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| // Check for values that would overflow when converted to Duration | |
| if secs > Duration::MAX.as_secs_f64() { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| let dur = Duration::from_secs_f64(secs); |
🤖 Prompt for AI Agents
In vm/src/stdlib/time.rs around lines 708 to 721, the code compares secs to
u64::MAX as f64, which can cause precision issues. To fix this, replace the
comparison with a check against u64::MAX as u128 converted to f64 or use a safer
method to ensure the comparison is accurate without precision loss, similar to
the fix applied in the non-Unix implementation.
vm/src/stdlib/time.rs
Outdated
| fn sleep(secs: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | ||
| // Try to get as float first, if that fails try as integer | ||
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | ||
| float_val | ||
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | ||
| int_val as f64 | ||
| } else { | ||
| return Err(vm.new_type_error("sleep() argument must be a number")); | ||
| }; | ||
| if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 { | ||
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | ||
| } | ||
|
|
||
| let duration = Duration::from_secs_f64(secs); | ||
| std::thread::sleep(duration); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Fix potential precision issue with u64::MAX conversion.
The validation logic has a potential issue with floating-point precision. When u64::MAX is converted to f64, it loses precision due to floating-point representation limits, which could allow some values larger than u64::MAX to pass validation.
Consider this alternative approach that handles the precision issue:
- if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 {
- return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
- }
+ if !secs.is_finite() {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }
+ if secs < 0.0 {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }
+ // Check for values that would overflow when converted to Duration
+ if secs > Duration::MAX.as_secs_f64() {
+ return Err(vm.new_value_error("sleep length must be a non-negative finite number"));
+ }This approach uses Duration::MAX.as_secs_f64() which provides a more accurate upper bound for the sleep duration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn sleep(secs: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | |
| // Try to get as float first, if that fails try as integer | |
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | |
| float_val | |
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | |
| int_val as f64 | |
| } else { | |
| return Err(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite() || secs < 0.0 || secs > u64::MAX as f64 { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| let duration = Duration::from_secs_f64(secs); | |
| std::thread::sleep(duration); | |
| Ok(()) | |
| } | |
| fn sleep(secs: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | |
| // Try to get as float first, if that fails try as integer | |
| let secs = if let Ok(float_val) = f64::try_from_object(vm, secs.clone()) { | |
| float_val | |
| } else if let Ok(int_val) = i64::try_from_object(vm, secs) { | |
| int_val as f64 | |
| } else { | |
| return Err(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| // Validate the float before converting to Duration | |
| if !secs.is_finite() { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| if secs < 0.0 { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| // Check for values that would overflow when converted to Duration | |
| if secs > Duration::MAX.as_secs_f64() { | |
| return Err(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| let duration = Duration::from_secs_f64(secs); | |
| std::thread::sleep(duration); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In vm/src/stdlib/time.rs around lines 93 to 109, the current validation compares
the sleep duration against u64::MAX converted to f64, which can cause precision
loss and allow invalid values. To fix this, replace the upper bound check with
Duration::MAX.as_secs_f64() to accurately represent the maximum allowable
duration. Update the condition to ensure secs is less than or equal to this
value, preventing precision-related validation errors.
💡 Verification agent
🧩 Analysis chain
Verify error message consistency with CPython.
Ensure that the error messages match CPython's behavior for better compatibility. The current generic message might not align with Python's standard library.
Also applies to: 708-721
🏁 Script executed:
#!/bin/bash
# Check CPython's error messages for time.sleep() with invalid arguments
python3 -c "
import time
try:
time.sleep(-1)
except ValueError as e:
print('Negative value error:', e)
try:
time.sleep('invalid')
except TypeError as e:
print('Type error:', e)
try:
time.sleep(float('inf'))
except ValueError as e:
print('Infinity error:', e)
try:
time.sleep(float('nan'))
except ValueError as e:
print('NaN error:', e)
"Length of output: 669
🏁 Script executed:
#!/bin/bash
# Enhanced check of CPython’s error messages for time.sleep() with various invalid arguments
python3 - << 'PYCODE'
import time
tests = [
(-1, "negative"),
("invalid","string"),
(float('inf'), "infinite"),
(float('nan'), "nan")
]
for val, label in tests:
try:
time.sleep(val)
except Exception as e:
print(f"{label} -> {type(e).__name__}: {e}")
PYCODELength of output: 264
Match CPython’s time.sleep error messages
The generic checks in sleep() should be split to mirror CPython’s behavior exactly:
- On non-number inputs, raise TypeError like
‘<type>’ object cannot be interpreted as an integer
instead of"sleep() argument must be a number". - For negative values, use ValueError
"sleep length must be non-negative". - For NaN, use ValueError
"Invalid value NaN (not a number)". - For infinities or overflow, use OverflowError
"timestamp too large to convert to C _PyTime_t".
Apply the same adjustments in the secondary implementation at lines 708–721.
Locations to update:
- vm/src/stdlib/time.rs: lines 93–109
- vm/src/stdlib/time.rs: lines 708–721
🤖 Prompt for AI Agents
In vm/src/stdlib/time.rs at lines 93 to 109, update the error handling in the
sleep() function to match CPython's exact error messages and types: raise a
TypeError with the message "'<type>' object cannot be interpreted as an integer"
for non-number inputs, raise a ValueError with "sleep length must be
non-negative" for negative values, raise a ValueError with "Invalid value NaN
(not a number)" for NaN inputs, and raise an OverflowError with "timestamp too
large to convert to C _PyTime_t" for infinities or values exceeding the maximum
allowed. Apply the same changes to the secondary implementation at lines 708 to
721 in the same file.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Thank you for your contribution.
Unfortunately, since #5906 was submitted earlier and addresses the same issue, I decided to merge that patch first.
However, your addition of a test for negative floats is still valuable, as we currently don't have coverage for that case.
I'd still like to merge the test you wrote as an additional improvement to the project.
| self.assertRaises(ValueError, time.sleep, float('nan')) | ||
| self.assertRaises(ValueError, time.sleep, float('inf')) | ||
| self.assertRaises(ValueError, time.sleep, -float('inf')) | ||
| self.assertRaises(ValueError, time.sleep, 1e100) | ||
| time.sleep(0) | ||
| time.sleep(0.000001) | ||
| time.sleep(1e-9) | ||
| time.sleep(0.5) | ||
| time.sleep(1.2) | ||
| time.sleep(2) |
There was a problem hiding this comment.
Since Lib/test/test_time.py is originated from CPython, we usually don't directly edit this file.
We usually add our original test under extra_tests like extra_tests/snippets/stdlib_time.py.
If you also want to contribute to CPython, you can submit a similar patch to CPyhton project: https://github.com/python/cpython
fixes #5895. I have added code to validate the input in the sleep() and raise the required errors. modified the test file and added few more test cases. Please let me know if any further changes are required.
Summary by CodeRabbit
New Features
sleepfunction to accept both integer and floating-point values for seconds, improving flexibility in specifying sleep durations.Bug Fixes