- Notifications
You must be signed in to change notification settings - Fork1.3k
[5895] added code to raise error in sleep if invalid value#5907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
[5895] added code to raise error in sleep if invalid value#5907
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedJul 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
""" 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 endAssessment 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! 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 withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thetime.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 thesleep 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 ourdocumentation 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 foundhere.
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 theGemini Code Assist IDE Extension.
Footnotes
Review thePrivacy Notices,Generative AI Prohibited Use Policy,Terms of Service, and learn how to configure Gemini Code Assist in GitHubhere. Gemini can make mistakes, so double check it anduse code with caution.↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code Review
This pull request adds input validation totime.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.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This validation logic is duplicated in theunix implementation ofsleep 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 bothsleep implementations.
For example:
fnget_sleep_secs(secs_obj:PyObjectRef,vm:&VirtualMachine) ->PyResult<f64>{// Try to get as float first, if that fails try as integerlet secs =ifletOk(float_val) = f64::try_from_object(vm, secs_obj.clone()){ float_val}elseifletOk(int_val) = i64::try_from_object(vm, secs_obj){ int_valasf64}else{returnErr(vm.new_type_error("sleep() argument must be a number"));};if !secs.is_finite() || secs <0.0 || secs > u64::MAXasf64{returnErr(vm.new_value_error("sleep length must be a non-negative finite number"));}Ok(secs)}
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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: morealPR: RustPython/RustPython#5847File: vm/src/stdlib/stat.rs:547-567Timestamp: 2025-06-27T14:47:28.810ZLearning: 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: morealPR: RustPython/RustPython#5847File: vm/src/stdlib/stat.rs:547-567Timestamp: 2025-06-27T14:47:28.810ZLearning: 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: CRPR: RustPython/RustPython#0File: .github/copilot-instructions.md:0-0Timestamp: 2025-06-30T10:08:48.858ZLearning: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 =ifletOk(float_val) = f64::try_from_object(vm, secs.clone()){ | |
| float_val | |
| }elseifletOk(int_val) = i64::try_from_object(vm, secs){ | |
| int_valasf64 | |
| }else{ | |
| returnErr(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite() || secs <0.0 || secs > u64::MAXasf64{ | |
| returnErr(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| let dur =Duration::from_secs_f64(secs); | |
| let secs =ifletOk(float_val) = f64::try_from_object(vm, secs.clone()){ | |
| float_val | |
| }elseifletOk(int_val) = i64::try_from_object(vm, secs){ | |
| int_valasf64 | |
| }else{ | |
| returnErr(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite(){ | |
| returnErr(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| if secs <0.0{ | |
| returnErr(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(){ | |
| returnErr(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 tou64::MAX as f64, which can cause precision issues. To fix this, replace thecomparison with a check against u64::MAX as u128 converted to f64 or use a safermethod to ensure the comparison is accurate without precision loss, similar tothe 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix potential precision issue with u64::MAX conversion.
The validation logic has a potential issue with floating-point precision. Whenu64::MAX is converted tof64, it loses precision due to floating-point representation limits, which could allow some values larger thanu64::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 usesDuration::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 =ifletOk(float_val) = f64::try_from_object(vm, secs.clone()){ | |
| float_val | |
| }elseifletOk(int_val) = i64::try_from_object(vm, secs){ | |
| int_valasf64 | |
| }else{ | |
| returnErr(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| if !secs.is_finite() || secs <0.0 || secs > u64::MAXasf64{ | |
| returnErr(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 =ifletOk(float_val) = f64::try_from_object(vm, secs.clone()){ | |
| float_val | |
| }elseifletOk(int_val) = i64::try_from_object(vm, secs){ | |
| int_valasf64 | |
| }else{ | |
| returnErr(vm.new_type_error("sleep() argument must be a number")); | |
| }; | |
| // Validate the float before converting to Duration | |
| if !secs.is_finite(){ | |
| returnErr(vm.new_value_error("sleep length must be a non-negative finite number")); | |
| } | |
| if secs <0.0{ | |
| returnErr(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(){ | |
| returnErr(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 comparesthe sleep duration against u64::MAX converted to f64, which can cause precisionloss and allow invalid values. To fix this, replace the upper bound check withDuration::MAX.as_secs_f64() to accurately represent the maximum allowableduration. Update the condition to ensure secs is less than or equal to thisvalue, 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 argumentspython3 -c"import timetry: 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 argumentspython3 -<< 'PYCODE'import timetests = [ (-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}")PYCODE
Length of output: 264
Match CPython’s time.sleep error messages
The generic checks insleep() 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 thesleep() function to match CPython's exact error messages and types: raise aTypeError with the message "'<type>' object cannot be interpreted as an integer"for non-number inputs, raise a ValueError with "sleep length must benon-negative" for negative values, raise a ValueError with "Invalid value NaN(not a number)" for NaN inputs, and raise an OverflowError with "timestamp toolarge to convert to C _PyTime_t" for infinities or values exceeding the maximumallowed. Apply the same changes to the secondary implementation at lines 708 to721 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SinceLib/test/test_time.py is originated from CPython, we usually don't directly edit this file.
We usually add our original test underextra_tests likeextra_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
Uh oh!
There was an error while loading.Please reload this page.
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