WIP: check that required_consts really contains all required consts

[RalfJung]

To this end, assert that evaluating const operands never fails (as they all should have been checked as part of required_consts).

I am going to try to find the right spot for codegen to add a similar check.

However, this also currently does not work -- const-eval triggers the new assertion. I am not sure how that is possible. (My previous theory made no sense.)

Cc @oli-obk

[rust-highfive]

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

[oli-obk]

However, this also currently does not work -- const-eval triggers the new assertion. I am not sure how that is possible. (My previous theory made no sense.)

Maybe it's const-prop again?

EDIT: oh you're checking that

[RalfJung]

@oli-obk is it possible that for const items (not function bodies), required_consts is not filled properly?

No it's not ConstProp. The PR already contains a special constprop hack as usual (this is getting tedious^^), but the remaining failures have a stack trace like

   0: std::panicking::begin_panic
             at ./library/std/src/panicking.rs:497
   1: rustc_errors::HandlerInner::bug
             at ./src/librustc_errors/lib.rs:915
   2: rustc_errors::Handler::bug
             at ./src/librustc_errors/lib.rs:665
   3: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}}
             at ./src/librustc_middle/util/bug.rs:33
   4: rustc_middle::ty::context::tls::with_opt::{{closure}}
             at ./src/librustc_middle/ty/context.rs:1792
   5: rustc_middle::ty::context::tls::with_context_opt
             at ./src/librustc_middle/ty/context.rs:1744
   6: rustc_middle::ty::context::tls::with_opt
             at ./src/librustc_middle/ty/context.rs:1792
   7: rustc_middle::util::bug::opt_span_bug_fmt
             at ./src/librustc_middle/util/bug.rs:29
   8: rustc_middle::util::bug::bug_fmt
             at ./src/librustc_middle/util/bug.rs:14
   9: rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_operand::{{closure}}
             at ./src/librustc_mir/interpret/operand.rs:526
  10: core::result::Result<T,E>::map_err
             at ./library/core/src/result.rs:612
  11: rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_operand
             at ./src/librustc_mir/interpret/operand.rs:521
  12: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_rvalue_into_place
             at ./src/librustc_mir/interpret/step.rs:153
  13: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::statement
             at ./src/librustc_mir/interpret/step.rs:89
  14: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::step
             at ./src/librustc_mir/interpret/step.rs:65
  15: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::run
             at ./src/librustc_mir/interpret/step.rs:34
  16: rustc_mir::const_eval::eval_queries::eval_body_using_ecx
             at ./src/librustc_mir/const_eval/eval_queries.rs:57
  17: rustc_mir::const_eval::eval_queries::const_eval_raw_provider::{{closure}}
             at ./src/librustc_mir/const_eval/eval_queries.rs:313
  18: core::result::Result<T,E>::and_then
             at ./library/core/src/result.rs:729
  19: rustc_mir::const_eval::eval_queries::const_eval_raw_provider
             at ./src/librustc_mir/const_eval/eval_queries.rs:313

Query stack:

query stack during panic:
#0 [const_eval_raw] const-evaluating `main`
#1 [const_eval_validated] const-evaluating + checking `main`
#2 [const_eval_validated] const-evaluating + checking `main`
#3 [normalize_generic_arg_after_erasing_regions] normalizing `main::promoted[2]`
#4 [optimized_mir] optimizing MIR for `main`
#5 [collect_and_partition_mono_items] collect_and_partition_mono_items

[oli-obk]

Ah. I just remembered. We do probe const eval really early and this may cause TooGeneric errors. See

rust/src/librustc_mir/const_eval/eval_queries.rs

Line 234 in 4745cbe

// try again with reveal all as requested

[RalfJung]

But it's not a TooGeneric:

error: internal compiler error: src/librustc_mir/interpret/operand.rs:526:25: const eval should have failed already when evaluating required_consts, so this error is unexpected: InterpErrorInfo { kind: referenced constant has errors, backtrace: None }

If anything, the referenced constant failed with TooGeneruic? But it doesn't look like that, the lints it printed so far are

warning: any use of this value will cause an error
  --> /home/r/src/rust/rustc/library/core/src/hint.rs:52:14
   |
LL |     unsafe { intrinsics::unreachable() }
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^
   |              |
   |              entering unreachable code
   |              inside `std::hint::unreachable_unchecked` at /home/r/src/rust/rustc/library/core/src/hint.rs:52:14
   |              inside `foo` at /home/r/src/rust/rustc/src/test/ui/consts/const_unsafe_unreachable_ub.rs:9:18
   |              inside `BAR` at /home/r/src/rust/rustc/src/test/ui/consts/const_unsafe_unreachable_ub.rs:14:28
   | 
  ::: /home/r/src/rust/rustc/src/test/ui/consts/const_unsafe_unreachable_ub.rs:14:1
   |
LL | const BAR: bool = unsafe { foo(false) };
   | ----------------------------------------
   |
note: the lint level is defined here
  --> /home/r/src/rust/rustc/src/test/ui/consts/const_unsafe_unreachable_ub.rs:13:8
   |
LL | #[warn(const_err)]
   |        ^^^^^^^^^

Also why does the query stack say it is "const-evaluating main"??

[RalfJung]

Oh, doing this during codegen will be hard because it doesn't actually stop compiling a function when encountering a bad constant!

rust/src/librustc_codegen_ssa/mir/mod.rs

Line 199 in 4745cbe

ErrorHandled::Reported(ErrorReported) | ErrorHandled::Linted => {}

That seems rather strange to me? There's no meaningful codegen in this case, we should just leave the function body empty or so... but we don't even delay_span_bug! or so.

[oli-obk]

#1 [const_eval_validated] const-evaluating + checking `main`
#2 [const_eval_validated] const-evaluating + checking `main`

does hint at this happening. I think we should start collecting some numbers on whether this scheme really is useful or not, but that's a different story.

Anyway, back to this PR, idk how this is happening, we should be inserting all constants that we couldn't evaluate, so while there could be more constants added to MIR later (I don't know of this), seems unlikely, we already handle inlining correctly, I think the likely case is that there are constants that fail to eval early that don't end up in the list. This may be true for constants in types, maybe we're not looking at repeat lengths?

[oli-obk]

Jup, we're not checking repeat lengths:

rust/src/librustc_mir/transform/required_consts.rs

Line 16 in 4745cbe

fn visit_constant(&mut self, constant: &Constant<'tcx>, _: Location) {

[RalfJung]

Also main is not a constant so that query stack just makes no sense. main cannot be const-eval'd.

[oli-obk]

Also main is not a constant so that query stack just makes no sense. main cannot be const-eval'd.

it's main::promoted[2], there's just bad printing of the const eval queries

[oli-obk]

you can fix the query stack printing by adjusting

rust/src/librustc_middle/query/mod.rs

Line 682 in 3cfc7fe

tcx.def_path_str(key.value.instance.def.def_id())

to just print the GlobalId I think.

[RalfJung]

I got a span for the const that fails to evaluate:

  --> /home/r/src/rust/rustc/src/test/ui/consts/const_unsafe_unreachable_ub.rs:17:14
   |
LL |   assert_eq!(BAR, true);
   |              ^^^

Doesn't look like an array length to me?

[oli-obk]

omg, promoteds are created after required_consts. We need to change promotion to also fill in required_consts

[oli-obk]

cc @spastorino ooops

[RalfJung]

Ah, found a real bug then. :)

Can one of you take over fixing that bug? That sounds like it is beyond my comfort zone.

[oli-obk]

jup. I checked out your PR locally, feel free to close this PR, I'll open a new one that includes your commit

[RalfJung]

I'll add another commit that does a similar check for codegen -- or do you prefer to land that separately?

[RalfJung]

Pushed the codegen change. It leads to errors like

error: internal compiler error: /home/r/src/rust/rustc/src/librustc_middle/macros.rs:13:35: erroneous constant not captured by required_consts
  --> /home/r/src/rust/rustc/src/test/ui/consts/promoted_div_by_zero.rs:8:13
   |
LL |     let x = &(1 / (1 - 1));
   |             ^^^^^^^^^^^^^^

[RalfJung]

All right, closing as @oli-obk is taking over.