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

Remove #[rustc_allow_const_fn_ptr] and add #![feature(const_fn_fn_ptr_basics)] #77170

Merged
merged 6 commits into from
Sep 28, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 24, 2020

rustc_allow_const_fn_ptr was a hack to work around the lack of an escape hatch for the "min const fn" checks in const-stable functions. Now that we have co-opted allow_internal_unstable for this purpose, we no longer need a bespoke attribute.

Now this functionality is gated under const_fn_fn_ptr_basics (how concise!), and #[allow_internal_unstable(const_fn_fn_ptr_basics)] replaces #[rustc_allow_const_fn_ptr]. const_fn_fn_ptr_basics allows function pointer types to appear in the arguments and locals of a const fn as well as function pointer casts to be performed inside a const fn. Both of these were allowed in constants and statics already. Notably, this does not allow users to invoke function pointers in a const context. Presumably, we will use a nicer name for that (const_fn_ptr?).

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2020
@ecstatic-morse ecstatic-morse changed the title Remove rustc_allow_const_fn_ptr and add #![feature(const_fn_fn_ptr_basics)] Remove #[rustc_allow_const_fn_ptr] and add #![feature(const_fn_fn_ptr_basics)] Sep 24, 2020
@jyn514 jyn514 added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 25, 2020
@bors
Copy link
Contributor

bors commented Sep 25, 2020

☔ The latest upstream changes (presumably #77172) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@oli-obk
Copy link
Contributor

oli-obk commented Sep 25, 2020

Oh this is a wondeful fix.

Presumably, we will use a nicer name for that (const_fn_ptr?).

we could add an unsound feature like that if you want it for experimentation (though it would probably be better to just allow it via unleash and not a gate). We'll need to figure out the details around that together with trait objects and method calls in general.

Note that you don't really need to fix the old min_const_fn check here, as that is just used by clippy now. We could in fact remove all feature gating logic from min_const_fn and just move it to clippy. this way it's removed from rustc and we'll just have to figure out whether we want to not just permit trivial const fns (so we'll have to adjust the logic in clippy instead of reusing the rustc logic)

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 25, 2020

we could add an unsound feature like that if you want it for experimentation.

Oh nothing like that. I was trying to explain the reason I chose a verbose name here (const_fn_fn_ptr_basics). const_fn_fn_ptr or const_fn_ptr imply that you can actually do stuff with them besides move them around, and I think we would want to use const_fn_ptr for const fn() -> i32 (or whatever form it takes).

@oli-obk
Copy link
Contributor

oli-obk commented Sep 25, 2020

right. So this PR lgtm, r=me after a rebase

@ecstatic-morse
Copy link
Contributor Author

@bors r=oli-obk rollup

@bors
Copy link
Contributor

bors commented Sep 25, 2020

📌 Commit 6d91b11a29a3d14f71bdef7383aeecc0e0fa1149 has been approved by oli-obk

@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 Sep 25, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75454 (Explicitly document the size guarantees that Option makes.)
 - rust-lang#76631 (Add `x.py setup`)
 - rust-lang#77076 (Add missing code examples on slice iter types)
 - rust-lang#77093 (merge `need_type_info_err(_const)`)
 - rust-lang#77122 (Add `#![feature(const_fn_floating_point_arithmetic)]`)
 - rust-lang#77127 (Update mdBook)
 - rust-lang#77161 (Remove TrustedLen requirement from BuilderMethods::switch)
 - rust-lang#77166 (update Miri)
 - rust-lang#77181 (Add doc alias for pointer primitive)
 - rust-lang#77204 (Remove stray word from `ClosureKind::extends` docs)
 - rust-lang#77207 (Rename `whence` to `span`)
 - rust-lang#77211 (Remove unused #[allow(...)] statements from compiler/)

Failed merges:

 - rust-lang#77170 (Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`)

r? `@ghost`
@bors
Copy link
Contributor

bors commented Sep 26, 2020

☔ The latest upstream changes (presumably #77224) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors 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 Sep 26, 2020
@ecstatic-morse ecstatic-morse force-pushed the const-fn-ptr branch 3 times, most recently from aa2e362 to 7a5ab4b Compare September 26, 2020 21:43
@ecstatic-morse
Copy link
Contributor Author

This will conflict with #77231. Better to let that one go through first.

@ecstatic-morse ecstatic-morse added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Sep 26, 2020
@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2020
@jonas-schievink
Copy link
Contributor

@bors r- failed in #77269

@bors 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 Sep 27, 2020
This was a hack to work around the lack of an escape hatch for the "min
`const fn`" checks in const-stable functions. Now that we have co-opted
`allow_internal_unstable` for this purpose, we no longer need the
bespoke attribute.
@ecstatic-morse
Copy link
Contributor Author

@jonas-schievink Any idea where I can find the logs for the failing rollup? A stage 1 build works and all UI tests pass for me locally on the latest master.

@jonas-schievink
Copy link
Contributor

Logs should be here: https://github.com/rust-lang/rust/runs/1173085918

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 27, 2020

Seems to be working now. I removed the old failing gate test for rustc_allow_const_fn_ptr, since it's not covering anything unique now.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 27, 2020

📌 Commit 807260b has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 28, 2020
Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`

`rustc_allow_const_fn_ptr` was a hack to work around the lack of an escape hatch for the "min `const fn`" checks in const-stable functions. Now that we have co-opted `allow_internal_unstable` for this purpose, we no longer need a bespoke attribute.

Now this functionality is gated under `const_fn_fn_ptr_basics` (how concise!), and `#[allow_internal_unstable(const_fn_fn_ptr_basics)]` replaces `#[rustc_allow_const_fn_ptr]`. `const_fn_fn_ptr_basics` allows function pointer types to appear in the arguments and locals of a `const fn` as well as function pointer casts to be performed inside a `const fn`. Both of these were allowed in constants and statics already. Notably, this does **not** allow users to invoke function pointers in a const context. Presumably, we will use a nicer name for that (`const_fn_ptr`?).

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 28, 2020
Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`

`rustc_allow_const_fn_ptr` was a hack to work around the lack of an escape hatch for the "min `const fn`" checks in const-stable functions. Now that we have co-opted `allow_internal_unstable` for this purpose, we no longer need a bespoke attribute.

Now this functionality is gated under `const_fn_fn_ptr_basics` (how concise!), and `#[allow_internal_unstable(const_fn_fn_ptr_basics)]` replaces `#[rustc_allow_const_fn_ptr]`. `const_fn_fn_ptr_basics` allows function pointer types to appear in the arguments and locals of a `const fn` as well as function pointer casts to be performed inside a `const fn`. Both of these were allowed in constants and statics already. Notably, this does **not** allow users to invoke function pointers in a const context. Presumably, we will use a nicer name for that (`const_fn_ptr`?).

r? @oli-obk
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#76454 (UI to unit test for those using Cell/RefCell/UnsafeCell)
 - rust-lang#76474 (Add option to pass a custom codegen backend from a driver)
 - rust-lang#76711 (diag: improve closure/generic parameter mismatch)
 - rust-lang#77170 (Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`)
 - rust-lang#77194 (Add doc alias for iterator fold)
 - rust-lang#77288 (fix building libstd for Miri on macOS)
 - rust-lang#77295 (Update unstable-book: Fix ABNF in inline assembly docs)

Failed merges:

r? `@ghost`
@bors bors merged commit 85a59d4 into rust-lang:master Sep 28, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 28, 2020
@ecstatic-morse ecstatic-morse deleted the const-fn-ptr branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants