Replace our fragile safety scheme around erroneous constants

[oli-obk]

Discussion: #67134 (comment)

The situation is as follows:

1. Sometimes code relies on a failing constant for safety (see Usage of errorneous constant can be omitted on nightly and beta #67083)
2. Those failing constants may have a ZST type (e.g. ())
3. A MIR optimization may see the constant and optimize it out (this happened: [mir-opt] Handle return place in ConstProp and improve SimplifyLocals pass #66216)
4. The constant not showing up in MIR can cause it to never get evaluated, thus never reporting its error
5. The runtime code relying on safety by having a hard error during const eval is now being executed even though it's unsound

The proposed solution is to gather all unevaluated constants in a MIR right after mir building and put them in a Vec<(DefId, SubstsRef<'tcx>, Promoted)> (or maybe a HashSet?). This should be placed on the mir::Body and inlining should also carry it down to the function that its being inlined into (and adjust substitutions on the SubstsRef). In the end, instead of having the collector go through the MIR to find constants to evaluate, we go through the vector and evaluate all of them.

This will require more memory and some additional evaluation time, but it would be significantly less fragile than the current setup which relies on optimizations not removing even guaranteed dead code containing constants.

cc @RalfJung @wesleywiser

[RalfJung]

Also Cc @Centril -- I am curious about your position on "compilation failure" being a side-effect of CTFE that we want to guarantee is being preserved. On the one hand this seems useful for some things, on the other hand it makes CTFE somewhat impure.

Relevant discussions:

• Ensure that we get a hard error on generic ZST constants if their bod… #67134 (comment)
• Ensure that we get a hard error on generic ZST constants if their bod… #67134 (comment)

[Centril]

@RalfJung I think I'm lacking some context; could you elaborate re. the various positions one could take and their trade-offs?

[oli-obk]

Essentially the question boils down to "is it ok for the following program to never emit a hard error, but at best emit a dead code warning or a const_err lint"

const FOO: usize = 0 - 1;
fn main() {
    if false {
        println!("{}", FOO);
    }
}

Everything else is just a more complex instance of the shown example. Although some of these situations may in fact be relied upon by users for safety:

const FOO: ! = panic!();
fn main() {
    let _ = FOO;
    unsafe {
        *(5 as *mut i32) = 49;
    }
}

In the above code it can be expected that the unsound code is in fact dead code (as the compiler tells you: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a2e0b258329500d44609bd82afd8ed07) and it even is unreachable, but only because the let _ = FOO; essentially translates to the equivalent of std::intrinsics::unreachable(). So the program exhibits UB because the code compiled successfully, when it should not.

[RalfJung]

Hm, that uninhabited type example is interesting, I was actually thinking of inhabited ZSTs so far... there we don't even have much of a choice. We have to abort compilation. (Well we could generate a [safe] trap but that would be very surprising.)

Things are less clear in situations like this:

const FOO: ! = panic!();
fn main() {
    if false {
        let _ = FOO;
    }
}

One could argue that we shouldn't error about bad constants in dead code. Is there a way people could rely on compilation passing when their CTFE error is only used in dead code?

[pvdrz]

What's the reason behind having bad constants inside dead code? The only situation I can imagine is some platform specific constant evaluation that would succeed in one platform and fail in another. But even then you could be better using conditional compilation instead.

[Centril]

@oli-obk @RalfJung So I think I understand the question now, but some elaboration re. "impure" and what drawbacks checking constants in dead code would be good. It sounds like the drawback would be potentially regressing perf and memory usage (but the evaluation of the constants used in live code should be memoized (?) so we would mostly only pay for the ones in dead code?

My position for now is that:

• Optimizations like const-prop et al. should not affect what code compiles under #![allow(const_err)] to maintain a basic "as-if" rule for defining the language. Thus the notion of "dead code" should be the pre-optimization notion. E.g. if according to NLL if false $block has $block live, then it is also live according to language rules. However, if { return; } $block would have $block dead according to NLL and so that legitimizes the idea that we do not error on bad constants in $block.

• Using #[deny(const_err)] for soundness of library code is dubious as it is a lint.

[osa1]

(Sorry if I'm misunderstanding the question here)

From a user's point of view I think control-flow based analyses are confusing. I think in this code

const FOO: ! = panic!();
fn main() {
    let _ = FOO;
    unsafe {
        *(5 as *mut i32) = 49;
    }
}

We should abort compilation. Same in this example

const FOO: ! = panic!();
fn main() {
    if false {
        let _ = FOO;
    }
}

If you ever worked with a langauge with control-flow based static checking (e.g. RPython) it's really confusing for a user for several reasons:

• You get different compile-time feedback from the compiler based on how good its control-flow analysis is, e.g. a compiler may be able to figure out if false { ... } will never be taken and allow compile-time errors inside the branch, but you change it to if f() { ... } where fn f() -> bool { false } and suddenly your code doesn't compile. It's even worse if the analysis is done post-optimization as that makes your program compile or not depending on the optimizer as well.

• During development sometimes you start implementing a function but do not call it yet. Until you actually call that function you don't get any feedback from the compiler for that function which is really annoying. E.g. in the example above if main() were actually f() and were not called by main() just yet you wouldn't get an abort in compile time until you call it from main. That's really bad IMHO.

I'm very new at rustc development but I think compile-time evaluation is done in MIR, which can be optimized. If this is done post-optimizations then we can also add an item here about getting different compile-time feedback depending on optimizations, which is also bad (GHC also has such problems, though they're really minor stuff and most users are probably not even aware of these).

[pnkfelix]

T-compiler triage: leaving unprioritized until we see feedback from T-lang on what we should do here.

[Centril]

We discussed this briefly in the language team meeting this Thursday. In this meeting, only @Centril, @nikomatsakis, and @ecstatic-morse attended.

Our basic conclusions were that having one consistent standard for "dead" / "live" as a matter of language definition (unaffected by compiler optimizations and lints like const_err) would be good. As NLL already has one standard, using that one would be sensible. In other words, if false { BAD_CONST } would still be a hard error whereas if {return} { BAD_CONST } would not be (as NLL would consider BAD_CONST dead). We also felt that relying on const_err for safety was not a good idea.

We briefly touched upon implementation strategy and wondered whether e.g. collecting things during the NLL analysis would be a good idea. This is of course left open for the compiler team to decide. :)