Stream: t-compiler/const-eval

Topic: Const-checking not being run on #66884?


ecstatic-morse (Dec 04 2019 at 17:57, on Zulip):

#66884 changes a bunch of std functions to const, but that PR appears to pass CI even with operations that should be illegal in a const context.

The reason we're not at DEFCON 2 right now is that I can't reproduce this on the playground. I'm gonna try to reproduce this in-core. I'm wondering if the const versions in the PR are getting cfg-ed out?

ecstatic-morse (Dec 04 2019 at 17:57, on Zulip):

cc @oli lemme know if you have found anything else

lqd (Dec 04 2019 at 18:40, on Zulip):

does the #[rustc_const_unstable(feature = "const_option_match")] somehow interact with cfgs ? is this const fn unwrap_or_else cfg-ed out when const_option_match is not enabled ?

ecstatic-morse (Dec 04 2019 at 18:45, on Zulip):

My understanding is that the string is just for documentation. I'll check to see if that's correct.

ecstatic-morse (Dec 04 2019 at 18:56, on Zulip):

Well and also to enable the feature downstream, but it should never compile out the function

ecstatic-morse (Dec 04 2019 at 19:06, on Zulip):

@oli So apparently min_const_fn is not enforced for rustc_const_unstable functions, but qualify_min_const_fn is AFAICT the only place where restrictions on generic parameters are checked.

    /// Returns `true` if this function must conform to `min_const_fn`
    pub fn is_min_const_fn(self, def_id: DefId) -> bool {
        // Bail out if the signature doesn't contain `const`
        if !self.is_const_fn_raw(def_id) {
            return false;
        }
        if let Abi::RustIntrinsic = self.fn_sig(def_id).abi() {
            return self.is_intrinsic_min_const_fn(def_id);
        }

        if self.features().staged_api {
            // in order for a libstd function to be considered min_const_fn
            // it needs to be stable and have no `rustc_const_unstable` attribute
            match self.lookup_stability(def_id) {
                // stable functions with unstable const fn aren't `min_const_fn`
                Some(&attr::Stability { const_stability: Some(_), .. }) => false,
                // unstable functions don't need to conform
                Some(&attr::Stability { ref level, .. }) if level.is_unstable() => false,
                // everything else needs to conform, because it would be callable from
                // other `min_const_fn` functions
                _ => true,
            }
        } else {
            // users enabling the `const_fn` feature gate can do what they want
            !self.features().const_fn
        }
    }
ecstatic-morse (Dec 04 2019 at 19:08, on Zulip):

I think that there's two concepts here: "is callable as a min const fn in stable code" and "requires the min const fn checks", but this function is responsible for both of them.

ecstatic-morse (Dec 04 2019 at 19:14, on Zulip):

Ah, there's an is_unstable_const_fn that's used for the first during const-checking. I don't really have all the background here, but I think we should be running the min const fn checks on rustc_const_unstable functions?

oli (Dec 04 2019 at 19:46, on Zulip):

ah, regular qualif used to have a call check, too afaik

oli (Dec 04 2019 at 19:46, on Zulip):

and that should fail for generic parameters

oli (Dec 04 2019 at 19:46, on Zulip):

having parameters with the const_fn feature gate has always been possible

oli (Dec 04 2019 at 19:47, on Zulip):

doing anything with them but accessing assoc constants has not

ecstatic-morse (Dec 04 2019 at 22:30, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0f28739e52e2d74bdebaac4696cf87c8

ecstatic-morse (Dec 04 2019 at 22:31, on Zulip):

I'm at a loss.

ecstatic-morse (Dec 04 2019 at 22:33, on Zulip):

Probably need to reduce this top-down rather than bottom up

lqd (Dec 04 2019 at 22:39, on Zulip):

@ecstatic-morse are you looking to repro on the playground ?

lqd (Dec 04 2019 at 22:46, on Zulip):

if so, something like https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=29afd185e26a0d92d8f05073cd596c9e ?

oli (Dec 05 2019 at 09:29, on Zulip):

We have a staged API check in the tcx.is_const_fn function, maybe I screwed that up?

lqd (Dec 05 2019 at 10:22, on Zulip):

so it's apparently not the staged api check

lqd (Dec 05 2019 at 10:24, on Zulip):

IIUC, in the gist I posted, unwrap_or_else is not considered a "const fn" nor "min const fn" because the rustc_const_unstable feature mentioned is not enabled

lqd (Dec 05 2019 at 10:25, on Zulip):

it's trivially a "const_fn_raw", so it'll be considered a "const fn" if the feature is enabled: https://github.com/rust-lang/rust/blob/7fa046534e944193cc47b795b9396a7fcf411d9f/src/librustc/ty/constness.rs#L18-L21

lqd (Dec 05 2019 at 10:26, on Zulip):

this is not the case in the gist or the PR (or at least the specific linked commit)

lqd (Dec 05 2019 at 10:27, on Zulip):

if you enable it: you'll get the expected "calls in constant functions are limited to constant functions" - playground

lqd (Dec 05 2019 at 10:29, on Zulip):

(and it's not considered a "min const fn" because it has a const stability attribute, https://github.com/rust-lang/rust/blob/7fa046534e944193cc47b795b9396a7fcf411d9f/src/librustc/ty/constness.rs#L90)

lqd (Dec 05 2019 at 10:32, on Zulip):

I don't know if that helps ? maybe I should look at the check_consts validation / min const fn qualification

lqd (Dec 05 2019 at 10:52, on Zulip):

(we probably need more debug!s in both of those :)

lqd (Dec 05 2019 at 11:32, on Zulip):

(and since I was mentioning mir_const_qualif: it will not run the checks because the function is not considered a const fn, so it'll bail out here https://github.com/rust-lang/rust/blob/7fa046534e944193cc47b795b9396a7fcf411d9f/src/librustc_mir/transform/mod.rs#L192-L194)

oli (Dec 05 2019 at 12:19, on Zulip):

Oh the last part is the problem. Good find. https://github.com/rust-lang/rust/blob/7fa046534e944193cc47b795b9396a7fcf411d9f/src/librustc_mir/transform/check_consts/mod.rs#L80 should check min const fn

oli (Dec 05 2019 at 12:20, on Zulip):

Eh const fn raw

oli (Dec 05 2019 at 12:20, on Zulip):

Instead of const fn

lqd (Dec 05 2019 at 12:24, on Zulip):

I'll get a PR ready

lqd (Dec 05 2019 at 12:28, on Zulip):

I was wondering whether just testing over ui/consts would be enough

lqd (Dec 05 2019 at 12:28, on Zulip):

but I'm not sure it'll bootstrap with the fix huhu

lqd (Dec 05 2019 at 12:29, on Zulip):

(nbd probably, probably just enabling more feature gates)

lqd (Dec 05 2019 at 12:59, on Zulip):

@oli for eg https://github.com/rust-lang/rust/blob/master/src/libcore/ptr/mod.rs#L1308-L1318 should I enable the const_ptr_offset_from feature only conditionally wrt bootstrap ? (I think I need to enable the feature, regardless of how and bootstrapping, it'll otherwise fail to compile with a "constant function contains unimplemented expression type")

lqd (Dec 05 2019 at 13:06, on Zulip):

(in my testing I did #![cfg_attr(not(bootstrap) it but I'm sure if that's correct or matters ?)

lqd (Dec 05 2019 at 14:17, on Zulip):

the lack of the trait bounds other than Sized on const fn parameters are unstable error we'd expect somewhere makes me still a bit uneasy, but here it is https://github.com/rust-lang/rust/pull/67055

oli (Dec 05 2019 at 15:49, on Zulip):

You need to enable it without a cfg. The function itself has no cfgs, so if you only enable it on bootstrap, stage 1 will fail

oli (Dec 05 2019 at 15:50, on Zulip):

Oh :face_palm:you're right

oli (Dec 05 2019 at 15:50, on Zulip):

Bootstrap doesn't need the feature gate

oli (Dec 05 2019 at 15:51, on Zulip):

Because the bootstrap compiler also has the problem

lqd (Dec 05 2019 at 15:56, on Zulip):

yeah it will not bootstrap otherwise (I now realize)

lqd (Dec 05 2019 at 15:56, on Zulip):

I'll adjust the comment, thanks for the quick review @oli :)

lqd (Dec 05 2019 at 16:08, on Zulip):

@oli how about this ?

// Note: this is deliberately checking for `is_const_fn_raw`, as the `is_const_fn`
// checks take into account the `rustc_const_unstable` attribute combined with enabled
// feature gates. Otherwise, const qualification would _not check_ whether this
// function body follows the `const fn` rules, as an unstable `const fn` would
// be considered "not const". More details are available in issue #67053.
oli (Dec 05 2019 at 16:23, on Zulip):

@lqd perfect

lqd (Dec 05 2019 at 16:24, on Zulip):

thanks

lqd (Dec 05 2019 at 16:24, on Zulip):

I'll push and r=you then

oli (Dec 05 2019 at 16:24, on Zulip):

Yes

lqd (Dec 05 2019 at 16:47, on Zulip):

done :)

lqd (Dec 05 2019 at 16:47, on Zulip):

now if you'll allow me: pasted image

centril (Dec 05 2019 at 16:51, on Zulip):

I think that there's two concepts here: "is callable as a min const fn in stable code" and "requires the min const fn checks", but this function is responsible for both of them.

@ecstatic-morse yea; we should separate them I think; see also https://github.com/rust-lang/rust/issues/64285 (feel free to assign yourself and steal)

Christian Poveda (Dec 05 2019 at 16:52, on Zulip):

now if you'll allow me: pasted image

damn now I have to change my nickname...

lqd (Dec 05 2019 at 16:54, on Zulip):

we're going to need bigger memes, of course you'll always be welcome christian :p

ecstatic-morse (Dec 05 2019 at 18:42, on Zulip):

Nice work @lqd!!

lqd (Dec 05 2019 at 18:44, on Zulip):

thank you :)

ecstatic-morse (Dec 05 2019 at 18:45, on Zulip):

Sorry. I went offline right after you sent your message with the repro.

lqd (Dec 05 2019 at 18:45, on Zulip):

(it wasn't hard and oli told me the fix)

lqd (Dec 05 2019 at 18:46, on Zulip):

oh no worries at all

Last update: Jan 28 2020 at 00:35UTC