Skip to content

Implementation of try_from_{nanos_u128,mins,hours,days,weeks}#153683

Open
Zorbatron wants to merge 5 commits intorust-lang:mainfrom
Zorbatron:zb/non_panicking_duration_conversion
Open

Implementation of try_from_{nanos_u128,mins,hours,days,weeks}#153683
Zorbatron wants to merge 5 commits intorust-lang:mainfrom
Zorbatron:zb/non_panicking_duration_conversion

Conversation

@Zorbatron
Copy link
Copy Markdown

@Zorbatron Zorbatron commented Mar 11, 2026 •
edited by joshtriplett
Loading

Implements the methods discussed in the related ACP.

impl Duration {
    pub const fn try_from_nanos_u128(nanos: u128) -> Result<Duration, DurationConversionError>;
    pub const fn try_from_weeks(weeks: u64) -> Result<Duration, DurationConversionError>;
    pub const fn try_from_days(days: u64) -> Result<Duration, DurationConversionError>;
    pub const fn try_from_hours(hours: u64) -> Result<Duration, DurationConversionError>;
    pub const fn try_from_mins(mins: u64) -> Result<Duration, DurationConversionError>;
}

ACP: rust-lang/libs-team#749
Tracking issue: #153678

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 11, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 11, 2026

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, scottmcm

@Zorbatron Zorbatron mentioned this pull request Mar 11, 2026
Open
5 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zorbatron Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from d8bc8ed to e0982d7 Compare April 12, 2026 16:06
@rustbot

This comment has been minimized.

Sign in to view
@Zorbatron
Copy link
Copy Markdown
Author

Zorbatron commented Apr 15, 2026

@scottmcm, sorry for the ping if I'm missing something for this PR to be reviewed, but it's been over a month. Are you available to look this over?

teor2345
teor2345 reviewed Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@teor2345 teor2345 left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

The code here looks good, I have a question about the stability of the error and display impls. (Probably for the libs-api team to answer.)

View changes since this review

Comment thread library/core/src/error.rs Outdated
#[stable(feature = "duration_checked_float", since = "1.66.0")]
impl Error for crate::time::TryFromFloatSecsError {}
#[unstable(feature = "non_panicking_duration_conversion", issue = "153678")]
impl Error for crate::time::DurationConversionError {}
Copy link
Copy Markdown
Contributor

@teor2345 teor2345 Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think removing the stability of the Error and Display impls from TryFromFloatSecsError is a breaking change?
https://doc.rust-lang.org/std/time/struct.TryFromFloatSecsError.html#impl-Error-for-TryFromFloatSecsError

Maybe it's worth asking if DurationConversionError and its trait impls should be stabilised instead?
rust-lang/libs-team#749 (comment)

Here's the context from the meeting notes approving the ACP:
https://hackmd.io/rGWrzOppS8-ehldMMjWwKQ#new-change-proposal-rusttflibs749-ACP-Add-non-panicking-versions-of-Durationfrom_nanos_u128minshoursdaysweeks

Suddenly not being able to print the return type of the stable try_from_secs_f64() method seems surprising for users. So does having to name it using a stable alias that doesn't appear in the method docs.

@scottmcm scottmcm added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 27, 2026
@scottmcm
Copy link
Copy Markdown
Member

scottmcm commented Apr 27, 2026

Nominating the discuss the questions from @teor2345 above.

r? joshtriplett

@rustbot rustbot assigned joshtriplett and unassigned scottmcm Apr 27, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 27, 2026

joshtriplett is not on the review rotation at the moment.
They may take a while to respond.

@joshtriplett
Copy link
Copy Markdown
Member

joshtriplett commented Apr 28, 2026

Fixed the signature of try_from_nanos_u128 in the OP to match the correct signature in the PR.

@nia-e
Copy link
Copy Markdown
Member

nia-e commented Apr 28, 2026

This was discussed in today's @rust-lang/libs-api meeting; the idea we came up with was to (for now) keep TryFromFloatSecsError and make DurationConversionError an unstable alias to it. We're not 100% on whether unstable aliases behave properly, so if that doesn't work we can FCP an instantly-stable DurationConversionError and make TryFromFloatSecsError an alias to it instead

@nia-e nia-e removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 28, 2026
@Zorbatron
Copy link
Copy Markdown
Author

Zorbatron commented Apr 28, 2026

... keep TryFromFloatSecsError and make DurationConversionError an unstable alias to it.

Just to confirm, this would mean adding the {Nanoseconds,Weeks,Days,Hours,Minutes}Overflow variants to TryFromFloatSecsError instead of DurationConversionErrorKind (for now)?

@the8472
Copy link
Copy Markdown
Member

the8472 commented Apr 28, 2026 •
edited
Loading

The kind is a private type so that change shouldn't change anything user-visible.

@Zorbatron
Copy link
Copy Markdown
Author

Zorbatron commented Apr 28, 2026

Or, keep DurationConversionErrorKind but change

pub struct TryFromFloatSecsError {
    kind: TryFromFloatSecsErrorKind,
}

to

pub struct TryFromFloatSecsError {
    kind: DurationConversionErrorKind,
}

@Zorbatron
Copy link
Copy Markdown
Author

Zorbatron commented Apr 28, 2026

Oop, I didn't see your comment the8472.

@Zorbatron Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from e0982d7 to 9eecbaf Compare April 28, 2026 19:20
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 28, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@teor2345 teor2345 teor2345 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@joshtriplett joshtriplett

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Zorbatron @rustbot @rust-log-analyzer @scottmcm @joshtriplett @nia-e @the8472 @teor2345