de-promote Duration::from_secs #71796
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit 58ae4a9 with merge b2a885c600d942fb829a214e798f7ae205a0594e... |
|
☀️ Try build successful - checks-azure |
|
@nikomatsakis so do you agree this is worthwhile and we should try to crater it? |
|
I do, which is why I did the bors try command. @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
crlf0710
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot p=1 |
|
🚨 Error: it's only possible to edit queued experiments 🆘 If you have any trouble with Crater please ping |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Both regressions are spurious. So, looks like on the part of the ecosystem that crater covers, this will not break anything. @nikomatsakis what is the next step here? |
|
With my release team hat on I'd say the zero regressions from the crater do look quite promising. I would suggest that we land this at the start of a nightly cycle, though, so ~June 4th -- in a few weeks -- just to give it as much time to bake as possible. |
|
r=me but waiting until the start of the cycle seems fine. @rust-lang/lang -- I'm not going to bother nominating this as I don't anticipate controversy. The proposal here is to remove |
Would this still allow |
|
@joshtriplett yes and yes. |
|
No objections then. |
nikomatsakis
added
S-blocked
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
|
⌛ Testing commit 58ae4a9 with merge 23ff1657a24783bddd42e51cfa70902060439a9d... |
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
|
yield |
|
⌛ Testing commit 58ae4a9 with merge 6543d83271ab6a1d8c1553a1ddb183e604a96ebd... |
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
|
@bors retry |
|
⌛ Testing commit 58ae4a9 with merge 42cda8ef3fabb5418ef717b08c7f056188ad4738... |
|
@bors retry rollup=always |
Rollup of 3 pull requests Successful merges: - rust-lang#71796 (de-promote Duration::from_secs) - rust-lang#72508 (Make `PolyTraitRef::self_ty` return `Binder<Ty>`) - rust-lang#72708 (linker: Add a linker rerun hack for gcc versions not supporting -static-pie) Failed merges: r? @ghost
|
@Mark-Simulacrum this was merged, even though it breaks Clippys UI tests: https://dev.azure.com/rust-lang/rust/_build/results?buildId=31276&view=logs&j=e50ab380-190b-535c-8a18-25c988f7577b&t=4a9c8583-1de4-50df-1657-629e71b187fc&l=7899 Admittedly one Clippy UI test is broken in the rustc testsuite anyway (something, something, proc_macro), but I thought with #72674 this shouldn't happen anymore. |
|
Hm yeah I would've expected that to not happen to. I'll try to take a look tomorrow. |
|
Fixed in #73097 |
In #67531, we removed the
rustc_promotableattribute from a bunch ofDurationmethods, but not fromDuration::from_secs. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):INT::min_value,INT::max_valuestd::mem::size_of,std::mem::align_ofRangeInclusive::new(backingx..=y)std::ptr::null,std::ptr::null_mutRawWaker::new,RawWakerVTable::new???Duration::from_secsI feel like the last one stands out a bit here -- the rest are all very core language primitives, and
RawWakerhas a strong motivation for getting a'staticvtable. But a&'static Duration? That seems unlikely. So I propose we no longer promote calls toDuration::from_secs, which is what this PR does.#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.
See this document for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.