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.

Sign up for GitHub

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

Jump to bottom

Time::checked_add_f64 and Time::checked_sub_f64 panic on too large input #2194

Closed
lopopolo opened this issue Sep 25, 2022 · 2 comments · Fixed by #2416
Closed

Time::checked_add_f64 and Time::checked_sub_f64 panic on too large input #2194

lopopolo opened this issue Sep 25, 2022 · 2 comments · Fixed by #2416
Assignees
@lopopolo
Labels
A-ruby-core Area: Ruby Core types. C-bug Category: This is a bug.

Comments

@lopopolo
Copy link
Member 

use core::time::Duration;

fn main() {
    dbg!(Duration::from_secs_f64(f64::MAX));
}

Output:

Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.58s
     Running `target/debug/playground`
thread 'main' panicked at 'can not convert float seconds to Duration: value is either too big or NaN', /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/time.rs:744:23
note: [run with `RUST_BACKTRACE=1` environment variable to display a backtrace](https://play.rust-lang.org/#)

There is an unstable API, Duration::try_from_secs_{f32, f64} which will allow for a checked conversion. See:

Once this API is stabilized, convert this code to use the fallible conversion:

artichoke/spinoso-time/src/time/tzrs/math.rs

Lines 163 to 167 in 2db5303

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

artichoke/spinoso-time/src/time/tzrs/math.rs

Lines 244 to 248 in 2db5303

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

@lopopolo lopopolo added C-bug Category: This is a bug. S-blocked Status: Marked as blocked ❌ on something else such as other implementation work. A-ruby-core Area: Ruby Core types. labels Sep 25, 2022
This was referenced Sep 25, 2022
Closed
Merged
@lopopolo
Copy link
Member Author

I've put up a stabilization PR for this feature in Rust std here: rust-lang/rust#102271.

JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Oct 24, 2022
Rollup merge of rust-lang#102271 - lopopolo:lopopolo/stabilize-durati…
c1f9d98 
…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
@Dylan-DPC
Rollup merge of rust-lang#102271 - lopopolo:lopopolo/stabilize-durati…
89d2387 
…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``
@lopopolo
Copy link
Member Author

These functions were stabilized and released today in Rust 1.66.0:

@lopopolo lopopolo self-assigned this Dec 16, 2022
@lopopolo lopopolo mentioned this issue Dec 19, 2022
Merged
@lopopolo lopopolo removed the S-blocked Status: Marked as blocked ❌ on something else such as other implementation work. label Dec 19, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Rollup merge of #102271 - lopopolo:lopopolo/stabilize-duration-try-fr…
bc9b441 
…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`
@lopopolo lopopolo mentioned this issue Feb 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lopopolo lopopolo

Labels
A-ruby-core Area: Ruby Core types. C-bug Category: This is a bug.
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix panics on f64 outside i64 range for checked Time float ops artichoke/artichoke
1 participant
@lopopolo