update test_time and implement more time module #7073
Merged
+732
−182
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThe changes extend the Unix timestamp and time structure handling in the time module with platform-specific code paths and new helper functions. Function signatures for gmtime, localtime, and ctime now accept optional nullable time values. StructTimeData gains new constructors (new_inner, new_utc, new_local) for platform-specific instantiation, and strftime/asctime logic is refactored to branch on Unix versus non-Unix implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
youknowone
force-pushed
the
time
branch
3 times, most recently
from
933912a to
f1e686e
Compare
youknowone
changed the title
youknowone
commented
Feb 11, 2026
|
|
||
| class TestAsctime4dyear(_TestAsctimeYear, _Test4dYear, unittest.TestCase): | ||
| pass | ||
| # TODO: RUSTPYTHON; chrono on Windows cannot handle month=0/day=0 in struct_time |
Member
Author
There was a problem hiding this comment.
Incorrect workaround handling
- Replace chrono-based gmtime/localtime/mktime/strftime/asctime/ctime with direct libc calls (gmtime_r, localtime_r, mktime, strftime) on unix, keeping chrono as fallback for non-unix platforms - Add checked_tm_from_struct_time for struct_time field validation - Accept None in gmtime/localtime/ctime (OptionalArg<Option<>>) - Fix StructTimeData field order: tm_zone before tm_gmtoff - Fix PyStructTime slot_new to accept optional dict argument
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/time.rs`:
- Line 580: The error message uses the wrong function name: update the call to
checked_tm_from_struct_time in the ctime implementation so the third argument is
"ctime" (not "asctime"), ensuring checked_tm_from_struct_time(...) produces
correct error text for the ctime function; look for the call site in the ctime
function and replace the literal function-name argument accordingly.
- Around line 670-675: The current match on t.naive_or_local(vm) in the strftime
path swallows struct_time conversion errors and returns the raw format string
(using fmt_lossy), which hides user errors; instead propagate the conversion
error so callers get a proper exception like CPython does. Replace the Err(_) =>
return Ok(vm.ctx.new_str(...)) branch in the t.naive_or_local(vm) match with
propagation of the underlying error (or create and return an appropriate VM
error/Value) so that invalid struct_time inputs to the strftime implementation
raise rather than silently fall back.
- Around line 515-520: The non-Unix branch for localtime currently passes a
hardcoded 0 for tm_isdst (see secs.naive_or_local and
StructTimeData::new_local), which incorrectly claims DST is off; change this to
derive the DST flag from the chrono datetime if possible (compute whether the
local datetime is in DST and pass that value into StructTimeData::new_local as
tm_isdst) or, if accurate DST detection is not feasible, pass -1 to indicate
unknown DST instead of 0 so non-Unix platforms don't falsely report "not DST".
- Around line 836-841: new_local currently calls
chrono::Local.from_local_datetime(&tm).unwrap(), which can panic for DST-gap
times; change it to handle the MappedLocalTime result like the non-Unix mktime
path by using .single().ok_or_else(...) (or otherwise map the none case to a
PyErr) and propagate the error (or change new_local to return PyResult<Self>) so
you don't unwrap; update the callsite or signature accordingly and then pass the
resultant offset_seconds and tz_abbr into Self::new_inner as before.
🧹 Nitpick comments (2)
crates/vm/src/stdlib/time.rs (2)
947-964:i64→f64round-trip inpyobj_to_time_tcan lose precision.For the
Either::B(int)branch, the value is cast tof64(line 958), range-checked, then cast totime_t(line 963). On 64-bit platforms wheretime_tisi64, thef64round-trip silently loses precision for values with magnitude > 2^53. Consider handling the integer path directly:Proposed fix
pub(super) fn pyobj_to_time_t( value: Either<f64, i64>, vm: &VirtualMachine, ) -> PyResult<time_t> { - let secs = match value { + match value { Either::A(float) => { if !float.is_finite() { return Err(vm.new_value_error("Invalid value for timestamp")); } - float.floor() + let secs = float.floor(); + if secs < time_t::MIN as f64 || secs > time_t::MAX as f64 { + return Err( + vm.new_overflow_error("timestamp out of range for platform time_t"), + ); + } + Ok(secs as time_t) } - Either::B(int) => int as f64, - }; - if secs < time_t::MIN as f64 || secs > time_t::MAX as f64 { - return Err(vm.new_overflow_error("timestamp out of range for platform time_t")); + Either::B(int) => { + let ts: time_t = int + .try_into() + .map_err(|_| vm.new_overflow_error("timestamp out of range for platform time_t"))?; + Ok(ts) + } } - Ok(secs as time_t) }
1019-1078: Duplicatedlibc::tmconstruction betweenunix_mktimeandchecked_tm_from_struct_time.Both functions extract the same fields from
StructTimeDatainto alibc::tmwith nearly identical code (clone + try_into_value + error mapping). Consider extracting a shared helper that builds the baselibc::tm, then let each caller apply its own adjustments (validation, sentinel values, zone handling). 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."
Comment on lines
+515
to
+520
| #[cfg(not(unix))] | ||
| let instant = secs.naive_or_local(vm)?; | ||
| // TODO: isdst flag must be valid value here | ||
| // https://docs.python.org/3/library/time.html#time.localtime | ||
| Ok(StructTimeData::new_local(vm, instant, -1)) | ||
| #[cfg(not(unix))] | ||
| { | ||
| Ok(StructTimeData::new_local(vm, instant, 0)) | ||
| } |
Contributor
There was a problem hiding this comment.
isdst is hardcoded to 0 in non-Unix localtime, ignoring DST.
CPython's localtime sets tm_isdst based on whether DST is in effect. Passing 0 unconditionally means non-Unix platforms will never report DST. You can derive it from chrono:
Proposed fix sketch
#[cfg(not(unix))]
- {
- Ok(StructTimeData::new_local(vm, instant, 0))
- }
+ {
+ let local_dt = chrono::Local
+ .from_local_datetime(&instant)
+ .single()
+ .ok_or_else(|| vm.new_overflow_error("localtime argument out of range"))?;
+ // chrono doesn't directly expose DST, but offset difference can indicate it
+ let isdst = -1; // unknown; or compute from offset vs standard offset
+ Ok(StructTimeData::new_local(vm, instant, isdst))
+ }At minimum, pass -1 (unknown) instead of 0 (definitely not DST) to be honest about the limitation.
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/time.rs` around lines 515 - 520, The non-Unix branch for
localtime currently passes a hardcoded 0 for tm_isdst (see secs.naive_or_local
and StructTimeData::new_local), which incorrectly claims DST is off; change this
to derive the DST flag from the chrono datetime if possible (compute whether the
local datetime is in DST and pass that value into StructTimeData::new_local as
tm_isdst) or, if accurate DST detection is not feasible, pass -1 to indicate
unknown DST instead of 0 so non-Unix platforms don't falsely report "not DST".
| OptionalArg::Present(None) | OptionalArg::Missing => current_time_t(), | ||
| }; | ||
| let local = localtime_from_timestamp(ts, vm)?; | ||
| let tm = checked_tm_from_struct_time(&local, vm, "asctime")?.tm; |
Contributor
There was a problem hiding this comment.
Wrong function name in error message: "asctime" should be "ctime".
checked_tm_from_struct_time uses the third argument to construct error messages (e.g. "{func_name}(): illegal time tuple argument"). This call site is inside ctime, not asctime.
Proposed fix
- let tm = checked_tm_from_struct_time(&local, vm, "asctime")?.tm;
+ let tm = checked_tm_from_struct_time(&local, vm, "ctime")?.tm;📝 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.
Suggested change
| let tm = checked_tm_from_struct_time(&local, vm, "asctime")?.tm; | |
| let tm = checked_tm_from_struct_time(&local, vm, "ctime")?.tm; |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/time.rs` at line 580, The error message uses the wrong
function name: update the call to checked_tm_from_struct_time in the ctime
implementation so the third argument is "ctime" (not "asctime"), ensuring
checked_tm_from_struct_time(...) produces correct error text for the ctime
function; look for the call site in the ctime function and replace the literal
function-name argument accordingly.
Comment on lines
+670
to
+675
| // (e.g. month=0), return the format string as-is, matching | ||
| // the fallback behavior for unsupported chrono formats. | ||
| let instant = match t.naive_or_local(vm) { | ||
| Ok(dt) => dt, | ||
| Err(_) => return Ok(vm.ctx.new_str(fmt_lossy.into_owned()).into()), | ||
| }; |
Contributor
There was a problem hiding this comment.
Non-Unix strftime silently swallows invalid struct_time errors.
CPython raises ValueError/TypeError for invalid struct_time arguments in strftime. Catching the error here and returning the raw format string is incorrect—it hides real user errors and deviates from CPython semantics.
Proposed fix: propagate the error
- // If the struct_time can't be represented as NaiveDateTime
- // (e.g. month=0), return the format string as-is, matching
- // the fallback behavior for unsupported chrono formats.
- let instant = match t.naive_or_local(vm) {
- Ok(dt) => dt,
- Err(_) => return Ok(vm.ctx.new_str(fmt_lossy.into_owned()).into()),
- };
+ let instant = t.naive_or_local(vm)?;📝 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.
Suggested change
| // (e.g. month=0), return the format string as-is, matching | |
| // the fallback behavior for unsupported chrono formats. | |
| let instant = match t.naive_or_local(vm) { | |
| Ok(dt) => dt, | |
| Err(_) => return Ok(vm.ctx.new_str(fmt_lossy.into_owned()).into()), | |
| }; | |
| let instant = t.naive_or_local(vm)?; |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/time.rs` around lines 670 - 675, The current match on
t.naive_or_local(vm) in the strftime path swallows struct_time conversion errors
and returns the raw format string (using fmt_lossy), which hides user errors;
instead propagate the conversion error so callers get a proper exception like
CPython does. Replace the Err(_) => return Ok(vm.ctx.new_str(...)) branch in the
t.naive_or_local(vm) match with propagation of the underlying error (or create
and return an appropriate VM error/Value) so that invalid struct_time inputs to
the strftime implementation raise rather than silently fall back.
Comment on lines
836
to
841
| fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { | ||
| let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); | ||
| let offset_seconds = | ||
| local_time.offset().local_minus_utc() + if isdst == 1 { 3600 } else { 0 }; | ||
| let offset_seconds = local_time.offset().local_minus_utc(); | ||
| let tz_abbr = local_time.format("%Z").to_string(); | ||
| Self::new_inner(vm, tm, isdst, offset_seconds, &tz_abbr) | ||
| } |
Contributor
There was a problem hiding this comment.
unwrap() on from_local_datetime can panic during DST gaps.
from_local_datetime returns MappedLocalTime which is None for times that fall in a DST gap (e.g. 2:30 AM on spring-forward day). Compare with the mktime non-Unix path (line 536) which correctly uses .single().ok_or_else(...).
Proposed fix
fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self {
- let local_time = chrono::Local.from_local_datetime(&tm).unwrap();
+ let local_time = chrono::Local
+ .from_local_datetime(&tm)
+ .single()
+ .unwrap_or_else(|| chrono::Local.from_local_datetime(&tm).latest().unwrap());
let offset_seconds = local_time.offset().local_minus_utc();Alternatively, make new_local return PyResult<Self> and propagate the error.
📝 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.
Suggested change
| fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { | |
| let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); | |
| let offset_seconds = | |
| local_time.offset().local_minus_utc() + if isdst == 1 { 3600 } else { 0 }; | |
| let offset_seconds = local_time.offset().local_minus_utc(); | |
| let tz_abbr = local_time.format("%Z").to_string(); | |
| Self::new_inner(vm, tm, isdst, offset_seconds, &tz_abbr) | |
| } | |
| fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { | |
| let local_time = chrono::Local | |
| .from_local_datetime(&tm) | |
| .single() | |
| .unwrap_or_else(|| chrono::Local.from_local_datetime(&tm).latest().unwrap()); | |
| let offset_seconds = local_time.offset().local_minus_utc(); | |
| let tz_abbr = local_time.format("%Z").to_string(); | |
| Self::new_inner(vm, tm, isdst, offset_seconds, &tz_abbr) | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/time.rs` around lines 836 - 841, new_local currently
calls chrono::Local.from_local_datetime(&tm).unwrap(), which can panic for
DST-gap times; change it to handle the MappedLocalTime result like the non-Unix
mktime path by using .single().ok_or_else(...) (or otherwise map the none case
to a PyErr) and propagate the error (or change new_local to return
PyResult<Self>) so you don't unwrap; update the callsite or signature
accordingly and then pass the resultant offset_seconds and tz_abbr into
Self::new_inner as before.
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/test/support dependencies:
dependent tests: (no tests depend on support) [ ] test: cpython/Lib/test/test_time.py (TODO: 24) dependencies: dependent tests: (133 tests)
Legend:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor