Skip to content

fix StructTimeData::new#6330

Merged
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:fix-time
Dec 5, 2025
Merged

fix StructTimeData::new#6330
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:fix-time

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 5, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Refactor
    • Reorganized time handling system to improve internal code organization and maintainability through streamlined constructor patterns and optimized time value processing.

✏️ Tip: You can customize this high-level summary in your review settings.

@youknowone youknowone marked this pull request as ready for review December 5, 2025 13:45
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 5, 2025 •
edited
Loading

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.

@youknowone youknowone merged commit 89bcae7 into RustPython:main Dec 5, 2025
13 checks passed
@youknowone youknowone deleted the fix-time branch December 5, 2025 23:30
This was referenced Feb 11, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone