◐ Shell
clean mode source ↗

fix StructTimeData::new by youknowone · Pull Request #6330 · RustPython/RustPython

youknowone

Walkthrough

Refactoring of the StructTimeData constructor in the time module. The single public new method is replaced with three specialized constructors: new_inner for direct construction with explicit parameters, new_utc for UTC times, and new_local for local times. Existing call sites in gmtime and localtime are updated to use the appropriate new constructors.

Changes

Cohort / File(s) Summary
Time module constructor refactoring
crates/vm/src/stdlib/time.rs		 Removed public new(vm, tm, isdst) constructor. Added new_inner(vm, tm, isdst, gmtoff, zone) as the primary implementation, with new_utc(vm, tm) and new_local(vm, tm, isdst) as specialized variants. Updated gmtime and localtime to use new_utc and new_local respectively.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify the removed new method is not called elsewhere in the codebase that isn't shown in this diff
  • Confirm new_utc and new_local correctly delegate parameters to new_inner with appropriate defaults
  • Validate that gmtoff and zone calculations are correct for UTC and local time variants

Poem

🐰 A constructor split in three,
New paths where one used to be,
UTC and local ways,
Time zones dancing through our days,
Cleaner code, we hop with glee!

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 'fix StructTimeData::new' directly addresses the main refactoring in the changeset, which involves restructuring the StructTimeData constructor methods.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% 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
📜 Recent 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 c826f9d and f14469b.

📒 Files selected for processing (1)
  • crates/vm/src/stdlib/time.rs (4 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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (4)
crates/vm/src/stdlib/time.rs (4)

308-315: LGTM!

The refactoring to use new_utc for gmtime is correct. UTC always has isdst=0, gmtoff=0, and zone "UTC".

317-326: LGTM!

The call to new_local with isdst=-1 correctly represents an unknown DST state, as noted by the existing TODO comment.

503-523: LGTM!

The new_inner constructor is clean and correctly initializes all struct fields with the provided parameters.

530-537: Verify DST transition handling against CPython behavior before applying fixes.

The review identifies two concerns in new_local():

  1. .unwrap() on from_local_datetime() may panic during DST gaps
  2. Offset calculation may double-count DST

However, verification cannot proceed without repository access. Critically, Python's time.localtime() is a thin wrapper around the platform C library, which handles DST transitions, ambiguous times, and non-existent times according to platform-specific semantics. RustPython's chrono-based Rust implementation may intentionally or unintentionally diverge from this behavior.

Before applying the proposed diff:

  • Confirm the actual code at lines 530–537
  • Verify whether isdst == 1 is ever passed to new_local() (affects offset double-counting risk)
  • Test DST transition behavior against CPython on the target platform to ensure compatibility
  • Determine if errors should be propagated (returning PyResult) or handled by picking a canonical time (e.g., .earliest())

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.