Make const_err a future-incompatibility lint and eventually a hard error

[RalfJung]

Currently, when Miri raises an error while evaluating a const, that does not lead to a hard compiler error (sess.struct_err or so). Instead, the const_err lint is emitted (and there are a bunch of exceptions I keep forgetting... promoteds and static each get their own treatment). Then later consumers of consts have to remember to struct_err when they query for the value of the const (and they do that with inconsistent messages and some might even forget to do it), usually without any actual details so if you allow(const_err) the error makes no sense... in a word, it's a mess (and so is the code backing this).

Can we clean that up? I think we should try. IMO the long-term goal should be to just make errors when evaluating a const hard errors, so we can have that error-reporting code once in librustc_mir/const_eval and not worry about it everywhere else in the compiler. Right now, it's a deny-by-default lint, for backwards compatibility.

To make this slightly more complicated, we also evaluate consts that the program does not actually use, so we can tell library crate authors when their consts are wrong. Inside the const-eval engine, these look like normal const-eval queries, so if we switch to a hard error, that would be a hard error, too. We have to decide what to do about that.

Option 1: treat unused consts like used consts whenever we can

The easiest way forward would be to say that an eventual hard error for CTFE failures in unused consts is fine. Then we can just make the current const_err lint reporting into a future-incompatibility lint (maybe clean it up a little, and clean up the way that we error when we truly need the const value). Later, we can experiment with making it a hard error, and at that point everything would become simple and uniform and beautiful. (Promoteds would still only lint as the user did not ask for const-evaluation, but at least behavior wouldn't depend on context any more: const/static always hard-error [which currently const only does for some callers], and promoteds never do. Okay maybe it's not beautiful but it is better.)

Note that this matches what we already do for static. However, unlike static, const can exist in generic contexts (as associated consts), and those we cannot evaluate and thus not lint or error for. So, "top-level" const would behave differently from associated consts in terms of error reporting. (That is already the case, but currently the difference is a deny-by-default lint that can be allow'ed away.) There is no technical need to hard-error on CTFE failures in unused consts, but then one could say the same about unused statics and we do already hard-error there (and I don't remember anyone ever complaining about that). So my personal preference would be to hard-error for failing consts as often as we can.

Option 2: keep just linting for unused consts

Alternatively, maybe we don't want to hard-error when a const that is not used fails to evaluate (consistently with how we cannot produce a hard error when an assoc const fails to evaluate as we cannot even check that). Arguably we should then downgrade such messages to warn-by-default.

In this case we should probably find a way for the caller to inform the const-eval engine whether they truly need the result of the query or not (almost always they do need it, except for that lint), and then use that to control our lint level... but how can we do that without duplicating errors or work? Currently it is deduplicated because the query only runs once, and it always emits a lint, and then the caller potentially additionally emits a hard error but pointing at the span where the const actually gets used. But this is bad, it means that with allow(const_err) none of the relevant error details are shown even when we do get a hard error.

Backwards compatibility concerns

A potential concern here might be that if we make CTFE errors hard errors, we have to be careful with making the Miri engine raise more CTFE errors (like, improved UB checking). But that is already the case: when raise more CTFE errors, the consts affected by it go from having a value to not having one, and cannot be used any more. So I don't think the proposal here creates any new backcompat issues.

Cc @rust-lang/wg-const-eval @rust-lang/lang

[joshtriplett]

On Sat, May 02, 2020 at 05:07:18AM -0700, Ralf Jung wrote: To make this slightly more complicated, we also [evaluate consts that the program does not actually use](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L1161-L1188), so we can tell library crate authors when their consts are wrong. Inside the const-eval engine, these look like normal const-eval queries, so if we switch to a hard error, that would be a hard error, too. We have to decide what to do about that. ### Option 1: treat unused consts like used consts whenever we can The easiest way forward would be to say that an eventual hard error for CTFE failures in unused consts is fine. Then we can just make the current `const_err` lint reporting into a future-incompatibility lint (maybe clean it up a little, and clean up the way that we error when we truly need the const value). Later, we can experiment with making it a hard error, and at that point everything would become simple and uniform and beautiful.

This seems completely reasonable to me. Unused constants are also potentially used for things like compile-time assertions, for which treating them exactly like used consts seems like the right behavior. We should always evaluate them, even if they won't get included in the final binary. So yes, I think we should have a single hard-error behavior for all constants.

[oli-obk]

I think we should have a single hard-error
behavior for all constants.

Just for the record: I think so, too.

We've had the deny warning around since one of my first contributions to rustc, and it not being a hard error has frequently required us to add more complexity to code that tries to work with constants just in case the user used #[warn(const_err)] or even #[allow(const_err)].

[Shnatsel]

For reference, here are all occurrences of const_err on crates.io, found with ripgrep: https://gist.github.com/Shnatsel/ff0bc7da32a0129fe5fe7211fac7d45c

[RalfJung]

@Shnatsel thanks!
That's actually not that many (131 lines, and a bunch of these are deny(const_err) or comments or function names).

[RalfJung]

Here's an example of an ICE caused by being able to make const_err not hard errors, that would be quite annoying to fix I think (other than what this issue proposes):

#![feature(const_transmute)]
#![warn(const_err)]

use std::mem;

const X: &i32 = unsafe {
    let x = 0;
    mem::transmute(&x)
};

This is just an example of the kind of problem we have because one part of the compiler cannot rely on other parts actually properly reporting good hard errors in certain situation. The code where we currently call delay_span_bug could emit a hard error instead, but that error is not as good as the const_err as it has less information, and we would get both errors in the default setting of deny(const_err).

[RalfJung]

Ah I think that ICE will actually be fixed by #71665... or at least behavior will change (likely to a duplicate error as discussed here).

Right now, interning fails on that constant, raises a Miri error, and that Miri error is treated as const_err. With my PR, interning can never fail with a Miri error (it might report diagnostic errors but the operation will always complete) -- and then in the next step we'll get to validation, and validation errors are always hard errors.

So this particular ICE can probably actually be fixed... but thinking this through is quite complicated each time.^^

[nikomatsakis]

I am 👍 with making it a future-incompatibility lint, if it's not already, and I would be in favor of moving to a hard error. I'm not sure of the timeline on those two steps, but I imagine that doing a crater run with a hard error would be a good way to augment the data that @Shnatsel gathered in terms of assessing the impact (the ripgrep suggests "not much", right?).

[nikomatsakis]

I wonder if an RFC is an appropriate idea here.

[golddranks]

The momentum here seems to be that the const_err lint is going to go the way of the dodo. It makes me think whether we should get rid of the "note: #[deny(const_err)] on by default" note on any const item eval failure? If we discourage the use of this lint, we shouldn't advertise it by default – the users may get the wrong picture there. I sure did.

I'm considering removing the note a part of #72660 for now. If anybody disagrees, please tell me.

[RalfJung]

That note is AFAIK printed by the general lint code in rustc.

But maybe it goes away when we declare const_err a future compat lint?

[RalfJung]

As part of this we should probably also clean up our span handling for const errors... currently there are up to 3 spans which the party reporting the error is specifying, and it is entirely unclear what they all mean:

rust/src/librustc_mir/const_eval/error.rs

Line 71 in 441fd22

span: Option<Span>,

rust/src/librustc_mir/const_eval/error.rs

Line 96 in 441fd22

tcx: TyCtxtAt<'tcx>,

rust/src/librustc_mir/const_eval/error.rs

Line 99 in 441fd22

span: Option<Span>,

[lcnr]

Taken from rust-lang/const-eval#53 (comment) as it is more relevant to this issue:

If we convert const_err to a hard error, we stabilize when we evaluate associated consts:

struct Foo<T>(T);

impl<T> Foo<T> {
    const ASSOC:usize = 4 / std::mem::size_of::<T>();

    fn dummy1(&self) {}

    fn dummy2(&self) {
        if false {
            Self::ASSOC;
        }
    }

    fn dummy3(&self) {
        Self::ASSOC;
    }
}

fn main() {
    // Which of these lines should try to evaluate `Foo::<()>::ASSOC` and cause `const_err`.
    let x: Foo<()> = Foo(()); // does not rn
    x.dummy1(); // does not rn
    x.dummy2(); // does rn (I fairly strongly oppose changing this to a hard error)
    x.dummy3(); // does rn
}

[RalfJung]

x.dummy2(); // does rn (I fairly strongly oppose changing this to a hard error)

I think a hard error is the only sensible choice. Leaving aside associated consts, we recently fixed the following to be a hard error:

const ERR: i32 = 1/0;

fn foo() { if false { let _val = ERR; } }

This was needed to resolve #67083, which was considered a critical issue. By the same argument, we have to also make dummy2 a hard error.

[RalfJung]

@lcnr your dummy2 and dummy3 btw are already hard errors now -- they fail to compile even with allow(const_err).

So nobody is proposing to change dummy2 to a hard error, it already is. allow(const_err) just makes the error message terrible, which is hard to fix without making const_err a hard error itself (so we do not need to rely on the later "erroneous constant used"). The only behavior that is proposed to be changed here is what happens with entirely unused consts (syntactically unused, even in dead code) that fail to evaluate.

[lcnr]

true 🤔 I was thinking of 4 / Self::ASSOC here... mb (with assoc being std::mem::size_of::<Self>())