Stream: t-compiler/const-eval

Topic: calling non-const-fn


RalfJ (Nov 16 2018 at 15:29, on Zulip):

Oh god what have we done... find_mir in CTFE fails to actually check if the function is const, and if I fix that I get two test failures...

RalfJ (Nov 16 2018 at 15:29, on Zulip):
failures:
    [ui] ui/consts/const-call.rs
    [ui] ui/consts/const-eval/strlen.rs

still investigating the details

oli (Nov 16 2018 at 15:38, on Zulip):

I remember doing that change

oli (Nov 16 2018 at 15:39, on Zulip):

the test failures are just changing diagnostics I think

RalfJ (Nov 16 2018 at 15:50, on Zulip):

I remember doing that change

doing what? I remember introducing hook_fn

RalfJ (Nov 16 2018 at 15:51, on Zulip):

ah but then you removed the NonConst error? oO

RalfJ (Nov 16 2018 at 15:52, on Zulip):

I thought I had stated on multiple occasions that we should really error in this case, because otherwise we expose all sorts of stuff in the miri engine that we don't want exposed^^

oli (Nov 16 2018 at 15:54, on Zulip):

we fixed those ;)

oli (Nov 16 2018 at 15:54, on Zulip):

that was mainly miri running on mir that has typeck errors

RalfJ (Nov 16 2018 at 15:56, on Zulip):

the miri engine still can do way more than was have stabilized

RalfJ (Nov 16 2018 at 15:57, on Zulip):

and without this check, the moment we allow calling const fn() pointers, the entire engine becomes stabilized without any const qualification protecting us

RalfJ (Nov 16 2018 at 15:57, on Zulip):

I do not think we want that

RalfJ (Nov 16 2018 at 16:03, on Zulip):

hm, it considers len() non-const even though there is #![feature(const_str_len, const_str_as_bytes)]. any idea what might be causing this @Oli ?

oli (Nov 16 2018 at 16:03, on Zulip):

Yea, that's why I removed it, our const check is broken

oli (Nov 16 2018 at 16:04, on Zulip):

cross crate evaluation with stability is really weird

oli (Nov 16 2018 at 16:04, on Zulip):

I mean, we already proved that something is const fn in the other crate

oli (Nov 16 2018 at 16:04, on Zulip):

we could do is_const_fn_raw

oli (Nov 16 2018 at 16:05, on Zulip):

but then again, with function pointers you could escape the whole stability thing

RalfJ (Nov 16 2018 at 16:06, on Zulip):

I mean, we already proved that something is const fn in the other crate

no we didn't. someone could have transmuted a function ptr.

RalfJ (Nov 16 2018 at 16:07, on Zulip):

we just have to do the same check as const qualification, don't we?

RalfJ (Nov 16 2018 at 16:07, on Zulip):

that somehow takes into account whether we have the necessary features enabled

RalfJ (Nov 16 2018 at 16:07, on Zulip):

I'd have thought that's what is_const_fn is for?

RalfJ (Nov 16 2018 at 16:08, on Zulip):

I mean, we already proved that something is const fn in the other crate

uh wait... this is not checking the body, is it? We want to check the signature, potentially with rustc_const_unstable

RalfJ (Nov 16 2018 at 16:08, on Zulip):

that is not something the other crate can have checked, so I do not understand your statement

RalfJ (Nov 16 2018 at 16:09, on Zulip):

we assume that if the function is marked const in the other crate, the body got checked; that is fair. now we have to check if it is indeed marked as such.

oli (Nov 16 2018 at 16:27, on Zulip):

If we don't care about stability, is_const_fn_raw is what you want. It just checks for the keyword

oli (Nov 16 2018 at 16:28, on Zulip):

is_const _fn also checks for stability things, but can't do so cross crate

oli (Nov 16 2018 at 16:29, on Zulip):

Const qualification is a static check

oli (Nov 16 2018 at 16:29, on Zulip):

You want a dynamic check, which isn't possible

oli (Nov 16 2018 at 16:29, on Zulip):

At least I don't see how to do that

oli (Nov 16 2018 at 16:30, on Zulip):

Or we could, but the code for stability is fragile enough

oli (Nov 16 2018 at 16:30, on Zulip):

I can look into it again if you want

oli (Nov 16 2018 at 16:32, on Zulip):

It's the same problem that macros have

oli (Nov 16 2018 at 16:33, on Zulip):

Except that you can't transmute macros

oli (Nov 16 2018 at 16:34, on Zulip):

We don't guarantee stability of transmuting things in horrible ways already

oli (Nov 16 2018 at 16:34, on Zulip):

Why should we do so here?

RalfJ (Nov 16 2018 at 16:59, on Zulip):

You want a dynamic check, which isn't possible

I want a check whether the current crate would consider that fn const

RalfJ (Nov 16 2018 at 16:59, on Zulip):

or, well, I thought I would. but of course execution goes into other crates, and if libstd calls some things that only it may consider const... I see what you mean

RalfJ (Nov 16 2018 at 17:00, on Zulip):

I still do not understand why len fails thought, because it should be const everywhere (current crate has the feature enabled)

RalfJ (Nov 17 2018 at 09:26, on Zulip):

@Oli do you have an explanation for why these test cases change in #56007? We shouldn't even start executing that code, right?

RalfJ (Nov 17 2018 at 09:28, on Zulip):

Currently we have things like https://play.rust-lang.org/?version=beta&mode=debug&edition=2015&gist=ae04a67a3aee6e747b924223b3b37867 which are really scary IMO

oli (Nov 17 2018 at 11:50, on Zulip):

The problem is that we have no way of knowing whether const qualif succeeded. We should make that query return a Result

oli (Nov 17 2018 at 11:50, on Zulip):

And then make const eval abort with AlreadyReported

RalfJ (Nov 17 2018 at 11:59, on Zulip):

wait, you are saying the error comes from inside the is_const_fn_raw?

RalfJ (Nov 17 2018 at 11:59, on Zulip):

I thought we are doing const qualification first, and then later we start executing this code. We shouldn't even do the latter if the former failed.

oli (Nov 17 2018 at 12:09, on Zulip):

we don't want to abort the entire compilation because of one failed item

oli (Nov 17 2018 at 12:09, on Zulip):

we just need to teach const eval to also run the const qualif query

RalfJ (Nov 17 2018 at 12:12, on Zulip):

okay. no idea how that'd work, I will leave the gluing to you :D

oli (Nov 17 2018 at 12:25, on Zulip):

@eddyb would it make sense to bubble up errors in e.g. mir_validated (by making it return a Result<&Steal<Mir>, ErrorReported>)? https://github.com/rust-lang/rust/blob/87a3c1ee7016bbfb782f2fd8adc75b46687ef929/src/librustc_mir/transform/mod.rs#L224 to prevent erroneous Mir from being processed downstream?

nagisa (Nov 17 2018 at 13:39, on Zulip):

What the query returns when MIR for given DefId does not exist?

nagisa (Nov 17 2018 at 13:39, on Zulip):

whatever it returns there is already a perfectly fine way to prevent downstream from processing data further

oli (Nov 17 2018 at 14:00, on Zulip):

I don't think bug! makes much sense: https://github.com/rust-lang/rust/blob/9649c1f70fddd01843024932df97fb5a2b10bfe8/src/librustc_mir/build/mod.rs#L82

nagisa (Nov 17 2018 at 14:02, on Zulip):

well yeah, so queries don’t have a built-in method for reporting impossiblity to resolve the query, then?

oli (Nov 17 2018 at 14:02, on Zulip):

nope

nagisa (Nov 17 2018 at 14:02, on Zulip):

which means that our end goal will end up making us making all queries return some sort of a Result anyway.

nagisa (Nov 17 2018 at 14:02, on Zulip):

:slight_smile:

oli (Nov 17 2018 at 14:03, on Zulip):

@mw would it make sense in general to make queries support failing?

nagisa (Nov 17 2018 at 14:06, on Zulip):

Not having queries require returning a Result is strictly more general… although with Result<T, !> that argument is somewhat moot as well.

eddyb (Nov 17 2018 at 14:47, on Zulip):

@Oli I don't know how I feel about that. how erroneous are we talking?

oli (Nov 17 2018 at 14:47, on Zulip):

like failed const qualification

oli (Nov 17 2018 at 14:47, on Zulip):

but I can imagine other things

eddyb (Nov 17 2018 at 14:48, on Zulip):

what are you thinking of preventing?

oli (Nov 17 2018 at 14:48, on Zulip):

concretely: const eval trying to evaluate non-const MIR

eddyb (Nov 17 2018 at 14:48, on Zulip):

I'd rather have a flag on the Mir than just throw it away entirely

oli (Nov 17 2018 at 14:48, on Zulip):

Result<Mir, ErrorMir> where the latter is a struct containing an error and the erroneous MIR?

eddyb (Nov 17 2018 at 14:49, on Zulip):

I don't like that tbh

eddyb (Nov 17 2018 at 14:49, on Zulip):

I guess you don't want to report duplicate errors, even if miri might not ICE

oli (Nov 17 2018 at 14:50, on Zulip):

yea, there's places we'd rather ICE about than report an error, because they make no sense

oli (Nov 17 2018 at 14:51, on Zulip):

I'll not do this for now and just make mir_const_qualif also return a flag saying that stuff broke

eddyb (Nov 17 2018 at 14:51, on Zulip):

what we're missing, really, is a way to avoid error cascades

eddyb (Nov 17 2018 at 14:52, on Zulip):

there's no reason we couldn't const-eval MIR that didn't pass the static checks

eddyb (Nov 17 2018 at 14:52, on Zulip):

"keep going at any cost (as long as someone reports an error)" is my preferred philosophy

eddyb (Nov 17 2018 at 14:52, on Zulip):

this could mean replacing span_bug!s in miri with delay_span_bug

oli (Nov 17 2018 at 14:54, on Zulip):

we are actually evaluating non-const-fns if const qualification fails

oli (Nov 17 2018 at 14:54, on Zulip):

are you sure we want to go there?

eddyb (Nov 17 2018 at 14:54, on Zulip):

yupp!

eddyb (Nov 17 2018 at 14:54, on Zulip):

as long as someone reported an error, why not?

oli (Nov 17 2018 at 14:55, on Zulip):

well... we can't prevent the error cascade in that case

RalfJ (Nov 17 2018 at 14:57, on Zulip):

I don't think we want to -- miri should be allowed to make some basic assumptions about the MIR, and bug! otherwise

eddyb (Nov 17 2018 at 14:58, on Zulip):

we have delay_span_bug for a reason

RalfJ (Nov 17 2018 at 14:58, on Zulip):

(though it cannot bug! for calling non-const-fns because of transmute, but there might be other cases)

RalfJ (Nov 17 2018 at 14:58, on Zulip):

that doesn't help, it doesn't abort computation

RalfJ (Nov 17 2018 at 14:58, on Zulip):

miri then still has to do something

RalfJ (Nov 17 2018 at 14:59, on Zulip):

I also dont see the point, TBH. we'd certainly want to suppress any errors that show up in such a computation, so why even perform it? just a waste of time.

oli (Nov 17 2018 at 15:00, on Zulip):

ignore the delay_span_bug thing for now... I'm talking about running into code like dereferencing raw pointers, which now can suddenly happen and spit out a bunch of diagnostics that will likely not be helpful to the user

eddyb (Nov 17 2018 at 15:00, on Zulip):

"because it could succeed" was part of my reasoning

eddyb (Nov 17 2018 at 15:00, on Zulip):

but, sure, as long as it doesn't stop the compilation, or non-miri things from using the MIR, I'm probably fine with most solutions

RalfJ (Nov 17 2018 at 15:00, on Zulip):

"because it could succeed" was part of my reasoning

and then what?

RalfJ (Nov 17 2018 at 15:01, on Zulip):

we abort compilation anyway. the user won't even know it succeeded.

eddyb (Nov 17 2018 at 15:01, on Zulip):

the computation could be an array length

oli (Nov 17 2018 at 15:02, on Zulip):

which would wrong very often, leading to downstream type mismatches

RalfJ (Nov 17 2018 at 15:02, on Zulip):

I dont think we should do anything based on an array length that calls a non-const-fn...

oli (Nov 17 2018 at 15:02, on Zulip):

(which are just noise in my book, we used to have them with old ctfe)

eddyb (Nov 17 2018 at 15:02, on Zulip):

but I guess if nothing bails out completely when const-eval errors, it's probably fine

RalfJ (Nov 17 2018 at 15:02, on Zulip):

Basically https://github.com/rust-lang/rust/pull/56007 is trying to make sure we do not accidentally expose "stuff" on stable

oli (Nov 17 2018 at 15:02, on Zulip):

yea, we'd just get an TyKind::Err instead of a TyKind::Array in that case

RalfJ (Nov 17 2018 at 15:02, on Zulip):

but in doing so we get annoying duplicate errors

oli (Nov 17 2018 at 15:04, on Zulip):

I have ideas, I'll cook up a PR and we can see if everyone likes it

RalfJ (Nov 17 2018 at 15:05, on Zulip):

okay, I am curious what you are cooking :)

eddyb (Nov 17 2018 at 15:07, on Zulip):

yeah, ping me on it :D

Last update: Nov 15 2019 at 20:10UTC