Introduce TimeoutSeconds utility type for timeout parameters#7271
Introduce TimeoutSeconds utility type for timeout parameters#7271ever0de wants to merge 3 commits intoRustPython:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/function/number.rs (1)
222-225: Extract the numeric value first, then constructTimeoutSecondsonce.Both branches only differ in conversion; building
Selfonce keeps this aligned with project style and avoids duplication.Proposed refactor
impl TryFromObject for TimeoutSeconds { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> { - super::Either::<f64, i64>::try_from_object(vm, obj).map(|e| match e { - super::Either::A(f) => Self { value: f }, - super::Either::B(i) => Self { value: i as f64 }, - }) + super::Either::<f64, i64>::try_from_object(vm, obj).map(|e| { + let value = match e { + super::Either::A(f) => f, + super::Either::B(i) => i as f64, + }; + Self { value } + }) } }As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/function/number.rs` around lines 222 - 225, Extract the numeric value returned by super::Either::<f64, i64>::try_from_object(vm, obj) into a local variable (e.g., let num = match e { ... }) converting i64 to f64 in the match, then construct and return the TimeoutSeconds instance once using that variable (e.g., Self { value: num }) instead of building Self in each match arm; update the closure where Either::try_from_object(...) is mapped so it returns the numeric f64 first and then constructs Self { value: num } for consistent, non-duplicated creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/thread.rs`:
- Around line 1097-1100: The join() code path improperly constructs
Duration::from_secs_f64(secs) with only secs >= 0.0, which allows
infinities/overflows to panic and treats NaN inconsistently; fix it by mirroring
the acquire_lock_impl! validation: check secs.is_finite(), secs >= 0.0, and secs
<= TIMEOUT_MAX before calling Duration::from_secs_f64(secs), returning None or
an appropriate error when validation fails so join() no longer panics on
infinite/overflow inputs (refer to join(), TIMEOUT_MAX, and acquire_lock_impl!
for the exact validation logic to copy).
---
Nitpick comments:
In `@crates/vm/src/function/number.rs`:
- Around line 222-225: Extract the numeric value returned by
super::Either::<f64, i64>::try_from_object(vm, obj) into a local variable (e.g.,
let num = match e { ... }) converting i64 to f64 in the match, then construct
and return the TimeoutSeconds instance once using that variable (e.g., Self {
value: num }) instead of building Self in each match arm; update the closure
where Either::try_from_object(...) is mapped so it returns the numeric f64 first
and then constructs Self { value: num } for consistent, non-duplicated creation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/stdlib/src/_sqlite3.rscrates/vm/src/function/mod.rscrates/vm/src/function/number.rscrates/vm/src/stdlib/thread.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/thread.rs`:
- Around line 1097-1110: The branch treating Sec as non-negative currently uses
`if secs >= 0.0 { ... } else { None }`, which makes NaN fall into the `else` and
become an infinite wait; invert the condition so NaN goes through the validation
path used by acquire_lock_impl!. Replace the `if secs >= 0.0 { ... } else { None
}` pattern with the inverse `if secs < 0.0 { None } else { ... }`, keeping the
inner validation that checks `nanos`, `TIMEOUT_MAX`, `!nanos.is_finite()` and
calls `vm.new_overflow_error(...)` or `Duration::from_secs_f64(secs)` as before.
| Some(t) => { | ||
| let secs = t.to_secs_f64(); | ||
| if secs >= 0.0 { | ||
| let nanos = secs * 1_000_000_000.0; | ||
| if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() { | ||
| return Err(vm.new_overflow_error( | ||
| "timestamp too large to convert to Rust Duration".to_owned(), | ||
| )); | ||
| } | ||
| Some(Duration::from_secs_f64(secs)) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
NaN handling is inconsistent with acquire_lock_impl!.
The condition secs >= 0.0 (line 1099) evaluates to false for NaN, causing NaN inputs to silently become infinite waits (via the else branch). In contrast, acquire_lock_impl! (lines 84–92) routes NaN through the validation path where !nanos.is_finite() correctly raises an OverflowError.
To match acquire_lock_impl! semantics and the intent of the previous fix, invert the condition so NaN reaches the validation logic:
Proposed fix
Some(t) => {
let secs = t.to_secs_f64();
- if secs >= 0.0 {
+ if secs < 0.0 {
+ None
+ } else {
let nanos = secs * 1_000_000_000.0;
if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() {
return Err(vm.new_overflow_error(
"timestamp too large to convert to Rust Duration".to_owned(),
));
}
Some(Duration::from_secs_f64(secs))
- } else {
- None
}
}📝 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.
| Some(t) => { | |
| let secs = t.to_secs_f64(); | |
| if secs >= 0.0 { | |
| let nanos = secs * 1_000_000_000.0; | |
| if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() { | |
| return Err(vm.new_overflow_error( | |
| "timestamp too large to convert to Rust Duration".to_owned(), | |
| )); | |
| } | |
| Some(Duration::from_secs_f64(secs)) | |
| } else { | |
| None | |
| } | |
| } | |
| Some(t) => { | |
| let secs = t.to_secs_f64(); | |
| if secs < 0.0 { | |
| None | |
| } else { | |
| let nanos = secs * 1_000_000_000.0; | |
| if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() { | |
| return Err(vm.new_overflow_error( | |
| "timestamp too large to convert to Rust Duration".to_owned(), | |
| )); | |
| } | |
| Some(Duration::from_secs_f64(secs)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/thread.rs` around lines 1097 - 1110, The branch treating
Sec as non-negative currently uses `if secs >= 0.0 { ... } else { None }`, which
makes NaN fall into the `else` and become an infinite wait; invert the condition
so NaN goes through the validation path used by acquire_lock_impl!. Replace the
`if secs >= 0.0 { ... } else { None }` pattern with the inverse `if secs < 0.0 {
None } else { ... }`, keeping the inner validation that checks `nanos`,
`TIMEOUT_MAX`, `!nanos.is_finite()` and calls `vm.new_overflow_error(...)` or
`Duration::from_secs_f64(secs)` as before.
There was a problem hiding this comment.
This is already addressed — TimeoutSeconds::try_from_object() rejects NaN with ValueError("Invalid value NaN (not a number)") before join() is ever called, so NaN cannot reach this code path.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of TimeoutSeconds in number.rs to see if NaN is rejected
cat crates/vm/src/function/number.rs | grep -A 30 "impl TryFromObject for TimeoutSeconds"
Length of output: 460
@ever0de, you're absolutely right, and I apologize for the incorrect review comment.
The TimeoutSeconds::try_from_object() implementation explicitly validates and rejects NaN values:
if value.is_nan() {
return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned()));
}This means NaN is caught during the conversion from Python object to TimeoutSeconds, so by the time join() receives a TimeoutSeconds parameter, it's guaranteed not to be NaN. The current implementation is correct as-is.
🧠 Learnings used
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7181
File: crates/stdlib/src/csv.rs:20-20
Timestamp: 2026-02-17T07:54:36.200Z
Learning: In RustPython, ensure that modules using Python sequence-like types bring in extension methods by importing the trait (for example, 'use rustpython_vm::sliceable::SliceableSequenceOp;'). Even if the trait name isn't directly referenced in code, its methods (such as len()) become in-scope on types like PyStr. Add this import in modules that rely on those extension methods to avoid compilation errors.
6f8ad6f to
783cca6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/vm/src/stdlib/thread.rs (1)
1094-1106:⚠️ Potential issue | 🟡 Minor
join()still treats NaN timeout as infinite wait.Line 1096 uses
secs >= 0.0; forNaNthis is false, so Line 1104 returnsNone(infinite wait) instead of erroring. This was already flagged previously and appears unresolved.Suggested fix
Some(t) => { let secs = t.to_secs_f64(); - if secs >= 0.0 { - if secs > TIMEOUT_MAX { + if secs < 0.0 { + None + } else { + if !secs.is_finite() || secs > TIMEOUT_MAX { return Err(vm.new_overflow_error( "timeout value is too large".to_owned(), )); } Some(Duration::from_secs_f64(secs)) - } else { - None } }#!/bin/bash set -euo pipefail echo "Verify join timeout branch condition and finite checks" rg -nP 'fn join\(|if\s+secs\s*>=\s*0\.0|if\s+secs\s*<\s*0\.0|!secs\.is_finite|from_secs_f64' crates/vm/src/stdlib/thread.rs -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/thread.rs` around lines 1094 - 1106, The join timeout branch currently treats NaN as an infinite wait because it only checks `secs >= 0.0`; update the logic in the join path that computes `secs` so you first assert the value is finite (e.g., `secs.is_finite()`), return an error for non-finite timeouts (NaN or ±inf) using the VM error constructor, and only then handle finite positive values (compare to TIMEOUT_MAX and call Duration::from_secs_f64) or finite negative values if you intend those to map to infinite waits; change the condition around `secs` where `Duration::from_secs_f64(secs)` is created to use `secs.is_finite()` together with the existing comparisons so NaN no longer falls through to `None`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/vm/src/stdlib/thread.rs`:
- Around line 1094-1106: The join timeout branch currently treats NaN as an
infinite wait because it only checks `secs >= 0.0`; update the logic in the join
path that computes `secs` so you first assert the value is finite (e.g.,
`secs.is_finite()`), return an error for non-finite timeouts (NaN or ±inf) using
the VM error constructor, and only then handle finite positive values (compare
to TIMEOUT_MAX and call Duration::from_secs_f64) or finite negative values if
you intend those to map to infinite waits; change the condition around `secs`
where `Duration::from_secs_f64(secs)` is created to use `secs.is_finite()`
together with the existing comparisons so NaN no longer falls through to `None`.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/vm/src/function/number.rscrates/vm/src/stdlib/thread.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/function/number.rs
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin refactor/introduce-timeout-seconds-type |
|
The changes looks good. Thinking Timeout is not belong to number. where will be the proper place? |
Thanks for checking! I agree it doesn't quite belong in number. How about we put it in a |
Follow-up refactoring from RustPython#7237. Python timeout parameters typically accept both float and int. Several places in the codebase used Either<f64, i64> for this, each repeating the same match-and-convert boilerplate. This extracts that into a TimeoutSeconds type in vm::function::number. Refactored sites: - _sqlite3::ConnectArgs.timeout - _thread::AcquireArgs.timeout - _thread::ThreadHandle::join timeout Either<f64, i64> in time.rs (6 sites) left unchanged: those are timestamp values with per-branch logic (floor, range checks, etc). Either<f64, isize> in select.rs also left unchanged (different type).
19c157e to
58f3904
Compare
f8929ae to
aa5a6f0
Compare
Follow-up refactoring from #7237.
Python timeout parameters typically accept both float and int. Several places in the codebase used Either<f64, i64> for this, each repeating the same match-and-convert boilerplate. This extracts that into a TimeoutSeconds type in vm::function::number.
Refactored sites:
Either<f64, i64> in time.rs (6 sites) left unchanged: those are timestamp values with per-branch logic (floor, range checks, etc). Either<f64, isize> in select.rs also left unchanged (different type).
Summary by CodeRabbit
Bug Fixes
Refactor