Skip to content

Conversation

@williampMSFT
Copy link
Contributor

@williampMSFT williampMSFT commented Dec 3, 2025

This change removes most panic paths from the Datetime type. Doing this required a breaking interface change to how months are expressed. Additionally, it adds some tests that exercise every date for the next couple hundred years to catch any date-specific bugs.

There are still a couple expects() in the chrono conversion path because we know about some invariants that the compiler can't really know about, but those are all behind a feature flag. I don't see a path to removing these without either using some sort of unsafe code or making an operation that is infallible have a fallible API. Compatibility with the chrono lib is behind a feature flag, though, so panic-sensitive clients can opt to not enable the chrono feature.

@williampMSFT williampMSFT force-pushed the user/williamp/no-panic branch 2 times, most recently from 760f153 to 98c4a6c Compare December 9, 2025 18:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the Datetime type to eliminate most panic paths by introducing a type-safe Month enum and removing array indexing operations. The changes improve code safety for embedded systems where panics can be catastrophic, while maintaining strict validation through comprehensive testing that exercises every date from 1970 to 2500.

Key changes:

  • Introduced Month enum with type-safe month representation (1-12) using num_enum for conversions
  • Replaced panic-prone array indexing with const methods on the Month enum
  • Added clippy lints to warn against panic-inducing operations (expect_used, unwrap_used, panic, etc.)

Reviewed changes

Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
supply-chain/imports.lock Added rustversion audit entries for dependency chain verification
supply-chain/audits.toml Added audits for num_enum, num_enum_derive, and rustversion dependencies
embedded-mcu-hal/Cargo.toml Added num_enum dependency and clippy lints to prevent panics
Cargo.lock Updated with num_enum and rustversion dependency entries
embedded-mcu-hal/src/time/mod.rs Exported the new Month enum publicly
embedded-mcu-hal/src/time/datetime.rs Replaced u8 month with Month enum, removed array-based month logic, updated all tests to use Month enum, added comprehensive date roundtrip test
embedded-mcu-hal/src/lib.rs Whitespace formatting changes only

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
RobertZ2011
RobertZ2011 previously approved these changes Dec 10, 2025
@williampMSFT williampMSFT enabled auto-merge (squash) December 10, 2025 17:46
@williampMSFT williampMSFT self-assigned this Dec 10, 2025
@jeffglaum jeffglaum moved this to In review in Embedded Controller Dec 11, 2025
@williampMSFT williampMSFT force-pushed the user/williamp/no-panic branch from 4ce21df to 87c7289 Compare December 11, 2025 18:37
@williampMSFT williampMSFT marked this pull request as ready for review December 11, 2025 18:39
asasine
asasine previously approved these changes Dec 12, 2025
…o stable is_multiple_of to satisfy both nightly clippy and stable clippy simultaneously
Copy link
Contributor

@felipebalbi felipebalbi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just line endings on one file again.

@williampMSFT williampMSFT merged commit 1903e77 into OpenDevicePartnership:main Dec 15, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Dec 15, 2025
@williampMSFT williampMSFT deleted the user/williamp/no-panic branch December 15, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants