Skip to content

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

Open
Zorbatron wants to merge 1 commit into
rust-lang:mainfrom
Zorbatron:zb/non_panicking_duration_conversion
Open

Implementation of try_from_{nanos_u128,mins,hours,days,weeks}#153683
Zorbatron wants to merge 1 commit into
rust-lang:mainfrom
Zorbatron:zb/non_panicking_duration_conversion

Conversation

@Zorbatron

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

Copy link
Copy Markdown

View all comments

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

rustbot commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

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
1 of 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

Zorbatron commented Apr 15, 2026

Copy link
Copy Markdown
Author

@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

@teor2345 teor2345 left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

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 Resolved
@scottmcm scottmcm added the I-libs-api-nominated [DEPRECATED; DO NOT USE] label Apr 27, 2026
@scottmcm

scottmcm commented Apr 27, 2026

Copy link
Copy Markdown
Member

Nominating the discuss the questions from @teor2345 above.

r? joshtriplett

@rustbot rustbot assigned joshtriplett and unassigned scottmcm Apr 27, 2026
@rustbot

rustbot commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

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

@joshtriplett

joshtriplett commented Apr 28, 2026

Copy link
Copy Markdown
Member

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

@nia-e

nia-e commented Apr 28, 2026

Copy link
Copy Markdown
Member

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 [DEPRECATED; DO NOT USE] label Apr 28, 2026
@Zorbatron

Zorbatron commented Apr 28, 2026

Copy link
Copy Markdown
Author

... 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

the8472 commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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

@Zorbatron

Zorbatron commented Apr 28, 2026

Copy link
Copy Markdown
Author

Or, keep DurationConversionErrorKind but change

pub struct TryFromFloatSecsError {
    kind: TryFromFloatSecsErrorKind,
}

to

pub struct TryFromFloatSecsError {
    kind: DurationConversionErrorKind,
}

@Zorbatron

Zorbatron commented Apr 28, 2026

Copy link
Copy Markdown
Author

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

This comment has been minimized.

Sign in to view
@Zorbatron
Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from 8e02931 to bf42220 Compare August 18, 2026 16:45
@rustbot

This comment has been minimized.

Sign in to view
@Zorbatron

Zorbatron commented Aug 18, 2026

Copy link
Copy Markdown
Author

@teor2345 ping? I kind of forgot about this after addressing your review.

teor2345
teor2345 approved these changes Aug 18, 2026
View reviewed changes

@teor2345 teor2345 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks fine to me, but I'm not a final approver here.

View changes since this review

@Zorbatron

Zorbatron commented Aug 18, 2026

Copy link
Copy Markdown
Author

@joshtriplett is this PR good now?

@Zorbatron

Zorbatron commented Oct 15, 2026

Copy link
Copy Markdown
Author

@rustbot ready

@Zorbatron

Zorbatron commented Oct 19, 2026

Copy link
Copy Markdown
Author

@teor2345 sorry for the ping, but what steps do I need to take to get this merged? This is my first time contributing to the stdlib so I don't know the proper procedures 😟.

@teor2345

teor2345 commented Oct 19, 2026

Copy link
Copy Markdown
Member

A lot of people have just been at EuroRust, so review capacity is a bit lower. But maybe Josh is busy, let's try another libs reviewer.

r? libs

Mark-Simulacrum
Mark-Simulacrum requested changes Oct 24, 2026
View reviewed changes
Comment thread library/core/src/time.rs Resolved
Comment thread library/core/src/time.rs Resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2026
@rustbot

rustbot commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@Zorbatron
Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from bf42220 to d0c0b1d Compare October 24, 2026 23:23
@rustbot

This comment has been minimized.

Sign in to view
@Zorbatron

Zorbatron commented Oct 24, 2026

Copy link
Copy Markdown
Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung

RalfJung commented Oct 25, 2026

Copy link
Copy Markdown
Member

Also if there's anything in the code that you'd have written differently if it weren't for const fn constraints, please add FIXME(const-hack) comments to explain what that would be.

Mark-Simulacrum
Mark-Simulacrum approved these changes Oct 25, 2026
View reviewed changes

@Mark-Simulacrum Mark-Simulacrum left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me with commits squashed

View changes since this review

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@Zorbatron
Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from 1779386 to 46fcd01 Compare November 4, 2026 20:53
@rustbot rustbot added the A-time Area: `std::time` label Nov 4, 2026
@rustbot

rustbot commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

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.

@Zorbatron

Zorbatron commented Nov 5, 2026

Copy link
Copy Markdown
Author

r=me with commits squashed

Does thing mean I need to squash my commits?

@teor2345

teor2345 commented Nov 6, 2026

Copy link
Copy Markdown
Member

Yes, but let's see if this works

@bors squash

@rust-bors

rust-bors Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

🔑 Only the PR author or reviewers can squash commits.

@Zorbatron
initial implementation of try_from_{nanos_u128,mins,hours,days,weeks}
e0ac689 
re-add internal comments on error kind enum

accidentally forgot to set the `issue` on the implementation of `core::error::Error` on `DurationConversionError`

Keep `TryFromFloatSecsError` and have `DurationConversionError` be a type alias to it

I don't think add repr(transparent) was neccesary

Remove const_unstable annotations

Use checked_mul instead

fmt
@Zorbatron
Zorbatron force-pushed the zb/non_panicking_duration_conversion branch from 46fcd01 to e0ac689 Compare November 6, 2026 17:00
@Zorbatron

Zorbatron commented Nov 6, 2026

Copy link
Copy Markdown
Author

Alright, squashed

This branch has not been deployed

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

Reviewers

@RalfJung RalfJung RalfJung left review comments
@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes
@teor2345 teor2345 teor2345 approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

A-time Area: `std::time` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

10 participants

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