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

no longer promote non-pattern const functions #67531

Merged
merged 1 commit into from
Jan 4, 2020

Conversation

RalfJung
Copy link
Member

This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb here: possibly, a sane subset of const fn that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point.

So, this removes the rustc_promotable attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2019
@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Dec 22, 2019

⌛ Trying commit 368ac73 with merge 662bf71...

bors added a commit that referenced this pull request Dec 22, 2019
WIP: no longer promote non-pattern const functions

This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point.

So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable.

r? @oli-obk
@bors
Copy link
Contributor

bors commented Dec 22, 2019

☀️ Try build successful - checks-azure
Build commit: 662bf71 (662bf71cc5bbfe44a109ab5b7205e12ec319754c)

@RalfJung
Copy link
Member Author

@pietroalbini could we get a check-only crater run for this?

@Centril
Copy link
Contributor

Centril commented Dec 22, 2019

@craterbot check

(@RalfJung you should be able to do ^-- as well.)

@craterbot
Copy link
Collaborator

👌 Experiment pr-67531 created and queued.
🤖 Automatically detected try build 662bf71
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2019
@craterbot
Copy link
Collaborator

🚧 Experiment pr-67531 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-67531 is completed!
📊 0 regressed and 0 fixed (81904 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 28, 2019
@oli-obk oli-obk added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 28, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Dec 28, 2019

Noone seems to be using this.

nominating for @rust-lang/libs discussion

This PR breaks hypothetical uses of

let x: &'static Duration = &Duration::from_millis(42);

The above happened because the Duration constructors were one of the very first const fn, created when we didn't know about the unfortunate side effects of permitting const fn calls to get promoted. A detailed explanation of the issues can be found in rust-lang/const-eval#19

The following two snippets will keep working as they would with any user defined const fn. Only the backwards compat hack in libstd will be removed.

let x: &Duration = &Duration::from_millis(42);

and

const X: &Duration = &Duration::from_millis(42);

will keep working as they do now, as promotion inside constants has none of the problematic side effects.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 28, 2019
@Centril
Copy link
Contributor

Centril commented Dec 28, 2019

Adding the language team as well; I think it would be quite desirable from a language POV to nix the promotability of these non-constructor functions.

@nikomatsakis
Copy link
Contributor

We discussed in the 2020-01-02 @rust-lang/lang team meeting and decided to approve this.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 2, 2020

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

@RalfJung RalfJung changed the title WIP: no longer promote non-pattern const functions no longer promote non-pattern const functions Jan 2, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2020

Awesome, thanks :)

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 2, 2020

📌 Commit 368ac73 has been approved by nikomatsakis

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2020
Centril added a commit to Centril/rust that referenced this pull request Jan 3, 2020
…akis

no longer promote non-pattern const functions

This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point.

So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable.

r? @oli-obk
@bors
Copy link
Contributor

bors commented Jan 4, 2020

⌛ Testing commit 368ac73 with merge 1eb8aca1a6dc799978a3c9f82147de7e9f909748...

Centril added a commit to Centril/rust that referenced this pull request Jan 4, 2020
…akis

no longer promote non-pattern const functions

This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point.

So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable.

r? @oli-obk
@Centril
Copy link
Contributor

Centril commented Jan 4, 2020

Rolled up into #67853, @bors retry

@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors added a commit that referenced this pull request Jan 4, 2020
Rollup of 8 pull requests

Successful merges:

 - #66913 (Suggest calling method when first argument is `self`)
 - #67531 (no longer promote non-pattern const functions)
 - #67773 (Add a test for #37333)
 - #67786 (Nix reexports from `rustc_span` in `syntax`)
 - #67789 (Cleanup linkchecker whitelist)
 - #67810 (Implement uncommon_codepoints lint.)
 - #67835 (tweak wording of mismatched delimiter errors)
 - #67845 (Also remove const-hack for abs)

Failed merges:

r? @ghost
@bors bors merged commit 368ac73 into rust-lang:master Jan 4, 2020
@RalfJung RalfJung deleted the tame-promotion branch January 8, 2020 09:44
@eddyb
Copy link
Member

eddyb commented Jan 16, 2020

I wonder if we could add a check, forcing a very trivial body for #[rustc_promotable] functions.

This might be easy, if entire MIR body looks like _0 = Foo { x: _1, y: _2, ... } (it should be using all of the fn arguments as fields, possibly using constants for some other fields).

RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants