Move std::time::Instant to core - #159421
Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
e29c5ae to
679f70c
Compare
This comment has been minimized.
This comment has been minimized.
679f70c to
e7de16b
Compare
This comment has been minimized.
This comment has been minimized.
849aedc to
678fff5
Compare
This comment has been minimized.
This comment has been minimized.
| /// | ||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| #[rustc_has_incoherent_inherent_impls] | ||
| #[repr(transparent)] |
There was a problem hiding this comment.
| #[repr(transparent)] | |
| #[repr(transparent)] | |
| #[rustc_pub_transparent] |
and missing docs about this. In any case I think having to transmute is a hack.
There was a problem hiding this comment.
Didn't know about that attribute, added!
It's definitely hacky. My rationale is this is something unsafe and to be discouraged. Having an unsafe function to do the conversion would make it more readily visible (e.g., in docs.rs and code suggestions).
I would also like to note that public transparency can be removed from this PR and still provide utility. Crates like Winit include Instant in their public API but only the user ever supplies the value. Having Instant in libcore would allow such crates to avoid feature gating.
|
How would you handle mixing of web-time with std::time::Instant::now() on a future web target where std::time::Instant::now() works? web-time and std::time::Instant::now() may disagree about both the epoch and the clock source in that case. |
Leave `now()` and `elapsed()` as incoherent methods in `std`. The new `Instant` is now always a transparent wrapper around `Duration`, which allows 3rd party libraries to provide `Instant` functionality on `no_std` platforms. This also allows `no_std` libraries to refer to `Instant` without a feature gate.
678fff5 to
82ba332
Compare
Co-Authored-By: bjorn3 <17426603+bjorn3@users.noreply.github.com>
|
For reference, the internal representation of |
Definitely messy. Even without the hypothetical target, I think it'd be likely that crates like
Was unaware of this PR! I checked through the open issues for anything like this but I clearly should've checked the PRs too. Regardless of the specific internal representation, having the type declared in @rustbot blocked |
|
There are two unrelated things here. Exposing the internal structure of an |
|
☔ The latest upstream changes (presumably #161338) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
ACP: None (happy to create one if requested!)
Tracking Issue: None
Feature Gate:
core_time_instantBackground
Instantis currently provided bystdas it has a platform specific internal representation. However, this representation is currently unavailable to users. That is, there is no method to convert to an from a system-specific value. As such, users do not actually benefit from this platform dependent representation. Further, since the type is defined instdand entirely opaque, libraries such asweb-timemust recreate theInstantAPI to act as a polyfill. Even thoughwasm32-unknown-unknownhas access tostd, itsnow()implementation is a panic-stub, so this library is virtually required when writing Rust for the browser.This reliance on a 3rd party polyfill is frustrating for end users. As an example,
wasm32v1-noneis currently incompatible with the polyfill due to its lack ofno_stdsupport.Solution
Instead, I propose standardising on
Durationas the canonical internal representation of anInstant. As is already the case, the meaning of the internal value is undefined. Users do not have safe access to this value and are provided no guarantees around its reference point.With a standard representation,
Instantcan then be moved tocore::time, allowingno_stdlibraries access to the type. Creating a value of typeInstantis still restricted tostdin safe Rust. This is done by leavingnow()andelapsed()instdas incoherent methods.To allow 3rd party libraries such as
web-timeto be substantially simplified, I am also proposingInstantbe arepr(transparent)type, which will allow usingtransmuteto convert between aDurationandInstantwithout causing UB. This will allow polyfills forInstantto simply providenow()andelapsed()as trait extension methods on thecoretype. In the future, these could be brought intocoreas well using EII.Platform Specific Changes
Instantwould consider anything within anepsilon()to be equivalent. Instead of checking against epsilon at comparison, I round the internalDurationto the next multiple of epsilon at creation. I believe this should give the same effect in mitigating measurement noise.Instantnow has an layout equivalent toDuration. All other platforms were already a wrapper aroundDuration.Notes
Instantthrough transmutation would likely resolve Add explicit or zero constructor forstd::time::Instant#40910, and be substantially helpful for libraries such asmock_instant. This isn't the reason for my effort, but it is worth noting.