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 qualify_min_const_fn and #![feature(const_fn)] #76618

Closed
1 of 2 tasks
ecstatic-morse opened this issue Sep 11, 2020 · 15 comments
Closed
1 of 2 tasks

Remove qualify_min_const_fn and #![feature(const_fn)] #76618

ecstatic-morse opened this issue Sep 11, 2020 · 15 comments
Assignees
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) 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.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 11, 2020

We currently have two separate MIR passes that are responsible for const-checking. One lives in the check_consts directory, and is responsible for the checks that run in all const contexts. The other is qualify_min_const_fn, which performs additional checks within const fn when #![feature(const_fn)] is not enabled. The split was necessary to push const fn feature over the finish line due to technical debt in the const-qualification code, but it resulted in some checks being duplicated between the two modules. Const-qualification has since been rewritten entirely, and it should be possible to unify the two to reduce the maintenance burden and improve diagnostics.

I half-heartedly attempted this in #68940, but eventually abandoned it due to the volume of error message changes and reticence from a former lang-team member. However, I think we should give this another try, since the split has caused some confusion about #![feature(const_fn)] (see #76602) and there was a request from the libs-team to change how stability attributes affect whether the qualify_min_const_fn runs (see #75794).

I did not fully understand when writing #68940 that qualify_min_const_fn is responsible for ensuring that publicly exported, stable const fn in staged_api crates do not rely on unstable features as part of their implementation. check_consts makes no such effort. So far, we've relied on the review process to enforce this, but I think one of the first steps will be to add this functionality to check_consts, enabling it only within const fn at first. After that, we can define new structured errors for each of the checks in qualify_min_const_fn in the check_consts framework and disable the former entirely. At first, all of these checks will be controlled by the #![feature(const_fn)] feature gate, but I think we should assign a new gate to each (floating point arithmetic within const fn, impl Trait, etc.).

@oli-obk, would you be willing to review this work? Is there anything I missed?

TODO:

cc @rust-lang/wg-const-eval

@ecstatic-morse ecstatic-morse added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-const-eval Area: Constant evaluation (MIR interpretation) labels Sep 11, 2020
@ecstatic-morse ecstatic-morse self-assigned this Sep 11, 2020
@RalfJung
Copy link
Member

So far, we've relied on the review process to enforce this

I am confused, haven't we relied on qualify_min_const_fn to enforce this?

@ecstatic-morse
Copy link
Contributor Author

For checks that exist in both qualify_min_const_fn and check_consts, yes. However, not everything in check_consts has an analogue in qualify_min_const_fn. const_precise_live_drops for sure does not, and I believe there are others (MutDeref?).

@RalfJung
Copy link
Member

Ah, so basically when check_consts was extended to accept more things, we sometimes forgot to extend qualify_min_const_fn to make sure it rejects them? That is surprising as I thought qualify_min_const_fn was a tight whitelist and thus wouldn't accept any of these new features.

@josephlr
Copy link
Contributor

josephlr commented Sep 13, 2020

I believe there are others (MutDeref?).

So it looks like stable library functions can't use MutDeref, as qualify_min_const_fn has a specific type-level check that bans any mutable references. However, this only applies in functions not constants, leading to a strange outcome where:

const FOO: Option<&mut i32> = None;

is allowed on stable, but

const fn bar() {
    let x: Option<&mut i32> = None;
}

is not, playground link.

The corresponding check in check_consts bans the operation of taking mutable references, rather than the type itself. These checks not being exactly the same mostly just seem to cause weird compiler errors (with incorrect recommendations).

That is surprising as I thought qualify_min_const_fn was a tight whitelist and thus wouldn't accept any of these new features.

This seems mostly true, an example of where it's not is the type-level check, which checks for some banned types, and then lets all other types pass through, which means if a new const-incompatible type/operation was added, this check would need to be updated.

@RalfJung
Copy link
Member

leading to a strange outcome

Note that we have a few cases where things are allowed in constants but not const fn. Union field access and mem:.transmute are further examples.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 17, 2020

would you be willing to review this work?

yes definively!

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 21, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Sep 22, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Sep 22, 2020
…d-api, r=oli-obk

Use const-checking to forbid use of unstable features in const-stable functions

First step towards rust-lang#76618.

Currently this code isn't ever hit because `qualify_min_const_fn` runs first and catches pretty much everything. One exception is `const_precise_live_drops`, which does not use the newly added code since it runs as part of a separate pass.

Also contains some unrelated refactoring, which is split into separate commits.

r? @oli-obk
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 25, 2020

I tried to remove qualify_min_const_fn in #76850, but ended up merely disabling it in rustc because it would have broken clippy, which would have caused CI to fail. I tried again in #77128, but clippy can only run lints on optimized_mir, which is a) wrong and b) causes some assertions in const-checking to fail.

To fully resolve the first half of this issue, one of a few things needs to happen:

@RalfJung
Copy link
Member

missing_const_for_fn can be disabled entirely. This lint is only in the "nursery", so this should be fine.

That would be my preferred solution for now, TBH -- but I am not a Clippy maintainer so obviously I am picking what would be easiest for rustc itself.

I added resolving this as a checkbox to the OP. I am not sure if there are still parts of const_fn left to be moved out so I couldn't add those to the list.

@RalfJung
Copy link
Member

Cc @rust-lang/clippy for the clippy problem

@ebroto

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Sep 26, 2020

IIUC, after that moving qualify_min_const_fn.rs would not be needed?

Clippy is the only client of qualify_min_const_fn right now. So it should be either moved to Clippy or removed, as rustc itself does not use it.

The new const analysis that rustc is using does not have a "speculative mode" -- if it detects a const-error, it raises a hard error. That is how such analysis usually work in rustc AFAIK, min_const_fn was the odd one out. Clippy exploited this special status to be able to detect if a function could be const without raising hard errors when it is not. A new mechanism to achieve this will have to be introduced, ideally with minimal maintenance overhead on the rustc side.

RalfJung added a commit to RalfJung/rust that referenced this issue Sep 26, 2020
… r=RalfJung,oli-obk

Add `#![feature(const_fn_floating_point_arithmetic)]`

cc rust-lang#76618

This is a template for splitting up `const_fn` into granular feature gates. I think this will make it easier, both for us and for users, to track stabilization of each individual feature. We don't *have* to do this, however. We could also keep stabilizing things out from under `const_fn`.

cc @rust-lang/wg-const-eval
r? @oli-obk
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 26, 2020
… r=RalfJung,oli-obk

Add `#![feature(const_fn_floating_point_arithmetic)]`

cc rust-lang#76618

This is a template for splitting up `const_fn` into granular feature gates. I think this will make it easier, both for us and for users, to track stabilization of each individual feature. We don't *have* to do this, however. We could also keep stabilizing things out from under `const_fn`.

cc @rust-lang/wg-const-eval
r? @oli-obk
@oli-obk
Copy link
Contributor

oli-obk commented Sep 26, 2020

Clippy frequently has custom algorithms to detect whether to lint on something. It's totally fine not to reuse the compiler logic and keep the compiler logic simple and just have a bunch of false negatives in the lint. False negatives in a non-bug-detecting lint are absolutely no problem. So yes, I think we should just move the function to clippy and call it a day. We can then look into improving things on the clippy side without requiring any maintaining on the compiler side

@RalfJung
Copy link
Member

@oli-obk sounds like a good plan to me.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 26, 2020

I'll do that now

@RalfJung
Copy link
Member

@oli-obk @ecstatic-morse what is left to be done here? The checklist in the original issue only has one unchecked item, which says "list needs completing"... ;)

@oli-obk oli-obk closed this as completed Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) 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.
Projects
None yet
Development

No branches or pull requests

5 participants