Use rounding in float to Duration conversion methods #96051

newpavlov · 2022-04-14T14:28:43Z

rust-highfive · 2022-04-14T14:28:46Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

a stabilization of a library feature
introducing new or changes existing unstable library APIs
changes to public documentation in ways that create new stability guarantees

rust-highfive · 2022-04-14T14:28:47Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

newpavlov · 2022-04-14T16:25:44Z

r? @nagisa

library/core/src/time.rs

Stargateur · 2022-04-15T04:00:55Z

there is really no tool for rounding float ? we are force to do it at hand in Duration ?

nagisa · 2022-04-15T10:10:51Z

there is really no tool for rounding float ? we are force to do it at hand in Duration ?

Truncating and/or correctly rounded conversions to/from some formats such as 32-bit or 64-bit integers are provided, usually by hardware nowadays. Duration, however, is special in that it is a 96-bit integer with a specific format (the 32-bit nanosecond counter is expected to be in a half-open range 0..1E9) so implementing it ourselves is most likely the only reasonable approach.

newpavlov · 2022-04-15T10:41:49Z

Well, another theoretical alternative is to migrate Duration from the somewhat weird 96-bit type which does not use the 32-bit half fully and to use simple 128-bit nanosecond counter. It would require additional conversions when we talk to most OSes, but it would significantly simplify other parts.

Stargateur · 2022-04-15T10:52:14Z

@newpavlov to be clear I never say we should do this, I just wondering if there was a tool to manipulate float to avoid doing such complex operation in the code of Duration. (I find none)

nagisa · 2022-04-24T16:54:50Z

Nominating for a T-libs decision on:

a) Do we want rounding after all?
b) What kind of tie resolution should be used if a.

joshtriplett · 2022-04-27T18:31:27Z

@nagisa

Reviewed in the @rust-lang/libs meeting.

a) Yes, we want rounding.
b) Round-to-even would be preferable, assuming it isn't massively less performance to implement (e.g. the difference between hardware-assisted and not).

nagisa · 2022-05-01T16:51:07Z

I don't believe there are any significant performance concerns here since we aren't able to use any hardware acceleration for the conversion either way, and I would be surprised if any ever showed up. The only concern with different tie resolution models I believe is just that one of them is just significantly more nasty to implement and understand.

newpavlov · 2022-05-27T01:03:57Z

@bors r+

bors · 2022-05-27T01:03:58Z

📌 Commit 06af3a6 has been approved by newpavlov

newpavlov · 2022-05-27T01:31:56Z

@bors r-

inquisitivecrystal · 2022-05-27T03:05:05Z

@newpavlov: when approving, remember to do r=nagisa,joshtriplett instead of r+.

newpavlov · 2022-05-27T03:06:40Z

@bors r=nagisa,joshtriplett

bors · 2022-05-27T03:06:42Z

📌 Commit 6495963 has been approved by nagisa,joshtriplett

…piler-errors Rollup of 3 pull requests Successful merges: - rust-lang#96051 (Use rounding in float to Duration conversion methods) - rust-lang#97066 (rustdoc: Remove `ItemFragment(Kind)`) - rust-lang#97436 (Update `triagebot.toml` for macos ping group) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2022-05-27T06:07:58Z

⌛ Testing commit 6495963 with merge 9a42c65...

Re-enable tests that were disabled due to floating-point arithmetic changes in the rust standard library. The behavior of floating-point arithmetic went back to rounding vs truncating in rust-lang/rust#96051, so it's safe to enable these tests again. A new bug has been filed to track the work to avoid doing floating-point arithmetic. The complication is that `StepRng` is behaving in unexpected ways and preventing migration of the existing unit tests. Fixed: 92923 Bug: 102199 Change-Id: I3372193a8d59f1bc0d816e9921056bb85fb290d4 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/643581 Reviewed-by: Jake Fried <friedj@google.com> Reviewed-by: Ghanan Gowripalan <ghanan@google.com> Commit-Queue: Tony Gong <gongt@google.com>

- Rust version 1.63 changed how they convert float to Durations. Previously they truncated, now they round - https://github.com/rust-lang/rust/releases/tag/1.63.0 - rust-lang/rust#96051 - 'Rounding is now used when converting a float to a Duration. The converted duration can differ slightly from what it was.'

* Updated dependencies Cargo and npm * Fixed clippy and format warnings * Fixed tests that are failing in Rust 1.63 - Rust version 1.63 changed how they convert float to Durations. Previously they truncated, now they round - https://github.com/rust-lang/rust/releases/tag/1.63.0 - rust-lang/rust#96051 - 'Rounding is now used when converting a float to a Duration. The converted duration can differ slightly from what it was.' * Fixed the names of some variables to match what they are

…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`

…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``

version 1.60 of the rust stdlib included a change to improve the accuracy of calculations in `Duration::try_from_secs_f32/64` (see [1]), which had a minor breaking change of slightly altering the calculated results. version 1.63 further tweaked this (see [2]). this commit updates tests to match the new calculated values from 1.63+, and thus fixes failure running tests in newer stable rust releases and helps us to fix CI failure. this is done at the expense of breaking the ability to run the tests with older rust versions, which seems reasonably acceptable at this time. for the record, the new values used here are those calculated using version 1.66. it has been suggested that rather than compare float values directly, it may be better for this crate to compare via the `float_eq` crate instead. this would essentially mean switching to a less accurate comparison. since some time has passed now since i originally encountered this problem, i think that just updating the values compared against is the best move at this time, though i am not necessarily against moving to use of `float_eq` at some point in future if this causes significant problems for users of older rust versions, or if we encounter further such issues from rust stdlib changes. note that in the original issue (see [3]) there were four functions whose doc-tests were failing, yet only three have needed to be modified here. the lack of needing to fix the fourth function's tests is presumably as a result of the changes made in rust v1.63. (the original issue was noticed with a v1.60 nightly). [1]: rust-lang/rust#90247 [2]: rust-lang/rust#96051 [3]: #48

…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`

rust-highfive assigned kennytm Apr 14, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 14, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2022

rust-highfive assigned nagisa and unassigned kennytm Apr 14, 2022

newpavlov force-pushed the duration_rounding branch from cb476c8 to 2521fcf Compare April 14, 2022 16:57

Use rounding in float to Duration conversion methods

65c75b2

newpavlov force-pushed the duration_rounding branch from 92a8201 to 65c75b2 Compare April 14, 2022 17:44

newpavlov mentioned this pull request Apr 14, 2022

Regression in rounding of Duration::from_secs_f64 in 1.60.0 #96045

Closed

nagisa reviewed Apr 14, 2022

View reviewed changes

library/core/src/time.rs Show resolved Hide resolved

library/core/src/time.rs Outdated Show resolved Hide resolved

mbartlett21 mentioned this pull request May 16, 2022

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

Closed

3 tasks

implement tie to even

c2d8445

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 27, 2022

newpavlov added 2 commits May 27, 2022 04:59

fix nanos overflow for f64

38609cd

fmt

6495963

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2022

compiler-errors mentioned this pull request May 27, 2022

Rollup of 3 pull requests #97444

Merged

bors merged commit e3813e4 into rust-lang:master May 27, 2022

rustbot added this to the 1.63.0 milestone May 27, 2022

newpavlov deleted the duration_rounding branch May 27, 2022 08:34

compiler-errors mentioned this pull request Aug 13, 2022

std::time::Duration::from_secs_f64 behavior changed in 1.63.0 #100480

Closed

tkmcmaster mentioned this pull request Sep 7, 2022

Pewpew updates20220906 FamilySearch/pewpew#98

Merged

lopopolo mentioned this pull request Sep 25, 2022

Stabilize duration_checked_float #102271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rounding in float to Duration conversion methods #96051

Use rounding in float to Duration conversion methods #96051

newpavlov commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

newpavlov commented Apr 14, 2022

Stargateur commented Apr 15, 2022

nagisa commented Apr 15, 2022 •

edited

Loading

newpavlov commented Apr 15, 2022

Stargateur commented Apr 15, 2022 •

edited

Loading

nagisa commented Apr 24, 2022

joshtriplett commented Apr 27, 2022

nagisa commented May 1, 2022

newpavlov commented May 27, 2022

bors commented May 27, 2022

newpavlov commented May 27, 2022

inquisitivecrystal commented May 27, 2022

newpavlov commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

Use rounding in float to Duration conversion methods #96051

Use rounding in float to Duration conversion methods #96051

Conversation

newpavlov commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

rust-highfive commented Apr 14, 2022

newpavlov commented Apr 14, 2022

Stargateur commented Apr 15, 2022

nagisa commented Apr 15, 2022 • edited Loading

newpavlov commented Apr 15, 2022

Stargateur commented Apr 15, 2022 • edited Loading

nagisa commented Apr 24, 2022

joshtriplett commented Apr 27, 2022

nagisa commented May 1, 2022

newpavlov commented May 27, 2022

bors commented May 27, 2022

newpavlov commented May 27, 2022

inquisitivecrystal commented May 27, 2022

newpavlov commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

nagisa commented Apr 15, 2022 •

edited

Loading

Stargateur commented Apr 15, 2022 •

edited

Loading