Implement the min_const_fn feature gate

[oli-obk]

cc @RalfJung @eddyb

r? @Centril

implements the feature gate for #53555

I added a hack so the const_fn feature gate also enables the min_const_fn feature gate. This ensures that nightly users of const_fn don't have to touch their code at all.

The min_const_fn checks are run first, and if they succeeded, the const_fn checks are run additionally to ensure we didn't miss anything.

[Centril]

We will need to ensure that the standard library has not enabled #![feature(const_fn)]; this includes libstd, liballoc, libcore.

[RalfJung]

Well, we will want it for NonZero::new_unchecked, right?

[Centril]

Hmm... I was hoping on using the stabilization of min_const_fn but exclusion of const_fn from libstd so that standard library functions could be const-ified en masse instead of piecemeal. This kinda relies on not having #![feature(const_fn)] enabled so that accidental things don't happen.

Could we handle new_unchecked through some other internal means instead temporarily?

EDIT: like rustc_const_stable or something...

[oli-obk]

I'd say:

• const fn without #[rustc_const_unstable] is stable if it passes min_const_fn
• const fn not passing min_const_fn can be forced stable with #[rustc_const_fn_force_stable]

This way the active feature gates have no influence on stability, but we're checking the right thing and can't accidentally do anything wrong.

[Centril]

@oli-obk this sounds good to me :)

[RalfJung]

What is const_raw_ptr_deref?

Can't we somehow check unsafety, instead of trying to get an exhaustive list of unsafe behavior?

[Centril]

If it aint possible to detect this in MIR perhaps do it in HIR instead?

EDIT: Add a test with an empty unsafe { ... } block in it

[RalfJung]

AFAIK MIR has some unsafety information; we do some unsafety checks in MIR.

[oli-obk]

done

[RalfJung]

Done as in, the reference to const_raw_ptr_deref is gone? If not, please mention the filename where that function is defined...^^

[oli-obk]

As in there's a test. And const_raw_ptr_deref is a feature gate.

[RalfJung]

Oh, I see. That's not clear.^^

But the feature is ruled out independent of whether that gate is set...?

[oli-obk]

well... it's additionally prevented in min_const_fn due to its unsafety. The same doesn't hold for casting *const T to usize. I'll add some more sanity checks. This will annoy the heck out of nightly users since they are now getting a lot of messages twice in different wording (already happening with a bunch of things in this PR)

[RalfJung]

That's a consequence of doing multiple analyses, right?

I do not think that is the right approach... even if we have separate analyses, we should only call one.

[RalfJung]

Is it really better to have all that code duplicated into a second analysis, than changing the existing analysis to qualify every const fn as "minimal, normal, invalid"?

[oli-obk]

Is it really better to have all that code duplicated into a second analysis, than changing the existing analysis to qualify every const fn as "minimal, normal, invalid"?

The existing analysis is supposed to be split up into three separate analyses. Me adding a fourth one to be split out in the future would not help the process.

[RalfJung]

Three?!? I can see that promotion is separate, but other than that everything else should be just "const checking", which should (eventually) be the same for const and const fn. What am I missing?

[oli-obk]

What am I missing?

The checks for Drop and Freeze/UnsafeCell to make sure statics don't contain Freeze and constants don't contain Drop values are two further passes.

[oli-obk]

fun side effect: the two cases below which are future compat lints are hard errors inside const fns

[Centril]

Some more tests... :)

[Centril]

I specifically do want a test here that ensures that even without access, const fn no_inner_fn_ptr2(x: Hide) { } is banned.

[oli-obk]

That requires a lot of code... I'll impl it, but I don't think it's a good addition to rustc

[Centril]

So you already have the erroring tests:

const fn no_dyn_trait(_x: &dyn std::fmt::Debug) {}
const fn no_fn_ptrs(_x: fn()) {}

This is good, because it leaves open the possibility that this means &dyn const std::fmt::Debug or const fn(); If we didn't reject this, then users could pass in values where you don't have a const implementation of std::fmt::Debug, and so we wouldn't be able to change this in the future. However, as for situations where you have:

struct Hide<'a>(&'a dyn Debug);
const fn foo(_x: Hide) {}

... this would only prevent a situation in the future where we decided to split up our nominal type universe into one for the const restriction and one for impure... This seems like a very unlikely future... as such... I think the tests you have currently are sufficient.

So you don't need to implement it.

[Centril]

Should be fine to merge once the tests pass. :)
Thanks for doing this!

[oli-obk]

@Centril this is ready now I think

[oli-obk]

So... while I could actually implement a check that validates the MIR of constants in min_const_fn terms, this would be a major undertaking, requiring a new query and very hacky inspection of the MIR of constants at weird intervals.

Can we just decide to treat const items as boundaries for stability? Meaning a constant may access unstable constants, even if said constant and const fns is used in a min_const_fn. There's basically no forward compat issue, as we can replace any unstable const fn by a macro and get pretty much the exact same thing. And using an unstable or even internal macro to compute a stable constant's value seems uncontroversial.

Alternatively I can rewrite the entire min_const_fn checker and fiddle it into the const_qualif pass. I don't trust myself to implement that blacklisting scheme correctly though.

[RalfJung]

I am perfectly fine with treating const as boundary for all sorts of analysis -- IMHO we should never inspect the code of another const, only ever its type+value. And for the value, we should be able to rely on it being const-safe for the given type.

[Centril]

@oli-obk I'm fine with this; I guess T-libs will just be a bit more careful with respect to const items :)

[Centril]

@bors r=Centril,varkor

[bors]

📌 Commit 2839f4f has been approved by Centril,varkor

[bors]

⌛ Testing commit 2839f4f with merge fea32f1...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: Centril,varkor
Pushing fea32f1 to master...

[jethrogb]

I had this code in a PR I was working on, how should I write this now?

impl<T> UnsafeList<T> {
    pub const fn new() -> Self {
        unsafe {
            UnsafeList {
                head_tail: NonNull::new_unchecked(1 as _),
                head_tail_entry: None
            }
        }
    }
}

[jethrogb]

Minimal repro on playground:

#![feature(staged_api, const_fn)]
use std::ptr::NonNull;

pub struct UnsafeList<T>(NonNull<T>);

impl<T> UnsafeList<T> {
    pub const fn new() -> Self {
        unsafe {
            UnsafeList(NonNull::new_unchecked(1 as _))
        }
    }
}

The only way to get around this is to add an unstable attribute.

[Centril]

@jethrogb Is this for a PR to the standard library or the compiler, or is this for code outside of that?
If it is the latter, it is simply unstable and you won't be able to do it.

[jethrogb]

PR for std

[Centril]

@jethrogb if UnsafeList::new is marked as #[rustc_const_unstable(feature="<name>")] then you can use powers that const_fn gives you, but that will also make the constness unstable. We will not permit any new APIs in the standard library to define stable const fns that do not pass min_const_fn.

[jethrogb]

@Centril that is not sufficient. You also need to add an unstable attribute where this wasn't needed before, even if you explicitly set the const_fn feature, see my minimal repro.

[Centril]

@jethrogb #[stable] + #[rustc_const_unstable] work together: https://play.rust-lang.org/?gist=fc05cbb77603a742574a3d85e54ecbb5&version=nightly&mode=debug&edition=2015