Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for Duration::try_from_secs_{f32, f64} #83400

Closed
3 tasks done
mbartlett21 opened this issue Mar 23, 2021 · 10 comments
Closed
3 tasks done

Tracking Issue for Duration::try_from_secs_{f32, f64} #83400

mbartlett21 opened this issue Mar 23, 2021 · 10 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mbartlett21
Copy link
Contributor

mbartlett21 commented Mar 23, 2021

Feature gate: #![feature(duration_checked_float)]

This is a tracking issue for checked methods to construct a duration from a floating-point value of seconds without panicking.

Public API

// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, FromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, FromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct FromFloatSecsError { ... }

impl core::fmt::Display for FromFloatSecsError { ... }
// std::error

impl std::error::Error for FromFloatSecsError { ... }

Steps / History

Unresolved Questions

@mbartlett21 mbartlett21 added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 23, 2021
@mbartlett21 mbartlett21 changed the title Tracking Issue for Duration::checked_from_secs_{f32, f64} Tracking Issue for Duration::try_from_secs_{f32, f64} Mar 23, 2021
@workingjubilee
Copy link
Member

This needs more testing to vet its behavior in the face of various interesting floating point values.
In particular, subnormal values and values close to the limit of f32 and f64's upper range.

@mbartlett21
Copy link
Contributor Author

This needs more testing to vet its behavior in the face of various interesting floating point values.
In particular, subnormal values and values close to the limit of f32 and f64's upper range.

This just currently checks what the othee panicking methods for the same thing do.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 15, 2021
Add functions `Duration::try_from_secs_{f32, f64}`

These functions allow constructing a Duration from a floating point value that could be out of range without panicking.

Tracking issue: rust-lang#83400
@newpavlov
Copy link
Contributor

The new code in #90247 should handle all interesting values perfectly.

@nvzqz
Copy link
Contributor

nvzqz commented May 13, 2022

What is left to stabilize this? It is important to me that time conversions I'm using don't panic.

@mbartlett21
Copy link
Contributor Author

mbartlett21 commented May 16, 2022

There is still an unresolved question:

What should the error type be called?

Once that is resolved, we'll need a stabilisation PR and then, once that gets approved, it will be a month or two until it appears in stable

EDIT: Note: #96051 changes the rounding mode to round-to-nearest instead of truncate. (note on the note: That pr is also changing the stabilised methods from_secs_*, but it would be nice to have it correct when it is first stabilised)

@arniu
Copy link

arniu commented Aug 15, 2022

Why not just impl TryFrom<f32 | f64> for Duration?

@mbartlett21
Copy link
Contributor Author

Because it isn't clear from TryFrom that it is meant to be seconds. It could be milliseconds or hours or days or some other reasonable measurement.
This is the same reason why Duration doesn't impl From<u128> or something. It could likewise be a lot of other units.

@lopopolo
Copy link
Contributor

lopopolo commented Sep 25, 2022

I came across this unstable API when debugging a panic in a time crate I maintain:

I will prepare a stabilization PR shortly and hope we can resolve the open question on error type naming in the context of an FCP. I hope that's an ok way to approach this.

@lopopolo
Copy link
Contributor

I have pushed a stabilization PR and stabilization report here: #102271.

The stabilization PR renames the error type to TryFromFloatSecs to indicate the fallibility of the method like Vec::try_reserve and TryReserveError.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 14, 2022
…ro, r=thomcc

Fix `Duration::{try_,}from_secs_f{32,64}(-0.0)`

Make `Duration::{try_,}from_secs_f{32,64}(-0.0)` return `Duration::ZERO` (as they did before rust-lang#90247) instead of erroring/panicking.

I'll update this PR to remove the `#![feature(duration_checked_float)]` if rust-lang#102271 is merged before this PR.

Tracking issue for `try_from_secs_f{32,64}`: rust-lang#83400
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Oct 24, 2022
…on-try-from-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang#83400.

### Implementation History

- rust-lang#82179
- rust-lang#90247
- rust-lang#96051
- Changed error type to `FromFloatSecsError` in rust-lang#90247
- rust-lang#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue rust-lang#72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

`@rustbot` label +T-libs-api -T-libs

cc `@mbartlett21`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 24, 2022
…on-try-from-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang#83400.

### Implementation History

- rust-lang#82179
- rust-lang#90247
- rust-lang#96051
- Changed error type to `FromFloatSecsError` in rust-lang#90247
- rust-lang#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue rust-lang#72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

``@rustbot`` label +T-libs-api -T-libs

cc ``@mbartlett21``
alexheretic added a commit to alexheretic/ab-av1 that referenced this issue Dec 2, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
…om-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang/rust#83400.

### Implementation History

- rust-lang/rust#82179
- rust-lang/rust#90247
- rust-lang/rust#96051
- Changed error type to `FromFloatSecsError` in rust-lang/rust#90247
- rust-lang/rust#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue #72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

`@rustbot` label +T-libs-api -T-libs

cc `@mbartlett21`
@Dylan-DPC
Copy link
Member

This is stabilised already in #102271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants