Rust: Errors in promoteds stop compilation (via deny-by-default const_err lint) even in dead code

[lcnr]

example by @felix91gr

#![feature(const_generics)]
#![allow(const_err)]

fn foo<const C: u8>() {
  if C > 0 {
    println!("Foo gives {}", 25 / C);
  }
  else {
    println!("Foo gives 0");
  }
}

fn main() {
  foo::<0>();
}

Note that this currently requires allow(const_err) because 25 /C would panic at runtime.
This is however fine as it isn't reachable because of C > 0.

We probably should relax this slightly here, not sure what's the best approach though

[felix91gr]

For what it's worth: it works fine for me in a nightly playground. I'm assuming the current const_err feature is not in nightly yet, right? @lcnr, and that's why it currently builds?

Edit for future people: see the two answers below :)

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

[lcnr]

It works because I added #![allow(const_err)]. Remove that line and you get an error

[felix91gr]

AHH, I thought that flag was to enable the const_err checking :smile:

Here's the playground then, with the error enabled :3 https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=28ae219a9e7d1054cb7009c5bb254686

[RalfJung]

Hm, this is interesting. It feels loosely related to discussions we recently had about constants always having to be well-formed even when unused (Cc https://github.com/rust-lang/rust/issues/67191).

But this time, it also involves promotion -- at least, I think 25/C is being promoted (println! adds an implicit &). Promotion should not lead to hard errors though, so something is odd.

Cc @oli-obk

[oli-obk]

There's no hard error involved, so all is fine from that perspective. I guess we'll need some sort of control flow based const prop to not hit these cases

[RalfJung]

Promoteds don't usually trigger const_err though. Some logic somewhere doesn't seem right here.

Remember that const_err is slated to become a hard error.

[oli-obk]

oh... I guess we should just not propagate const generics at all. We can't enfore wf bounds for promoteds, because they are implicitly generated. This is the same issue we have with associated constants:

This happens on stable: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c5377d51d296c31a0cb1284269833bf5

[RalfJung]

I don't see why we wouldn't const-prop them. We just have make the lint less silly.

This doesn't look like a const-prop thing at all to me. Const-prop doesn't emit const_err, or at least it shouldn't. The following would be expected to error:

if ... {
  const FOO: i32 = something / 0;
  FOO
} else { ... }

The bug is that here we get this error not for a const but for a promoted -- unlike consts, those do not have to evaluate successfully.

I wonder how anon-const-blocks should behave here... Cc https://github.com/rust-lang/rfcs/pull/2920 @ecstatic-morse

[oli-obk]

Ugh. Why are words so similar and why do I keep mixing them up.

I think we should not promote const generics at all. Const prop is entirely irrelevant here.

EDIT: we could promote direct uses of const generics (so let x = &N;) or once we have wf bounds for const expressions, we can promote anything which has a wf bound.

[felix91gr]

What is promoting, in this context? Sorry I have to ask, but this topic is very interesting to me and I can't follow what you guys are saying x3

[oli-obk]

Promotion is the action of implicitly generating constants even if the user did not explicitly write anything to that regard. Further reading is available in the reference: https://doc.rust-lang.org/stable/reference/destructors.html?highlight=promotion#constant-promotion

[felix91gr]

Ahh I see. So like, say we have a constant and we use it like this:

const SIZE : u32 = 10;

fn foo() -> {

  let a = [0; SIZE]; // maybe I wrote it wrong, but the idea is that a is an array of 0s of length SIZE.

  println!("{}", a);
}

Then a could be promoted to a constant by the compiler because it can prove that it can do so without problems. Right?

And then, after promotion, a is treated like a constant for all intents and purposes for the remainder of the compilation process.

[felix91gr]

(println! adds an implicit &). Promotion should not lead to hard errors though, so something is odd.

@RalfJung I think that maybe the error happens before that borrow. Before println! can use its &, the computation of 25/C should be breaking an assert! that says "denominator must be different than 0".

[RalfJung]

I think that maybe the error happens before that borrow. Before println! can use its &, the computation of 25/C should be breaking an assert! that says "denominator must be different than 0".

The error happens at compile-time, i.e., before run-time -- but the order of operations in the code is irrelevant.
If my theory is correct, then if you replace the println by let _val = &(25/C); you will still get the error, but if you remove the & the error goes away.

For more on promotion, also see https://github.com/rust-lang/const-eval/blob/master/promotion.md.

I think we should not promote const generics at all.

That seems odd though? I mean it's an okay temporary hack, but really these values can be pre-computed so promotion is fine. We just must not const_err lint in promoteds. I thought we already had logic for that but clearly that logic is not prepared for const generics.

Const prop is entirely irrelevant here.

Happy that we agree on that. :)

[oli-obk]

So, this lint is emitted here, which is specifically the "emit a lint for promoteds that fail to eval" logic. I guess we just need a new lint for this case?

[felix91gr]

@RalfJung you are completely right. Removing the & stops the error from happening.

With &: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1ccf02e34dcff742719986b7f2da3703

Without: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=451a58c859e193cc47ab7d615dbb4893

PS: thank you for the link :)

[felix91gr]

So, this lint is emitted here, which is specifically the "emit a lint for promoteds that fail to eval" logic. I guess we just need a new lint for this case?

I think the logic itself might be wrong. In the sense that this expression should never be promoted if C is zero in the first place, since that means that the expression 25 / C should never be evaluated.

[RalfJung]

Promotion cannot know if code will be evaluated or not, so that part is expected.

The linting part is interesting though. We don't even need any associated consts / const generics to trigger the problem:

fn main() {
    if false {
        let _val = &(1/0);
    }
}

The diagnostics here are not great, we actually get three errors: (Cc https://github.com/rust-lang/rust/issues/61821)

error: this operation will panic at runtime
 --> src/main.rs:3:21
  |
3 |         let _val = &(1/0);
  |                     ^^^^^ attempt to divide 1_i32 by zero
  |
  = note: `#[deny(unconditional_panic)]` on by default

error: reaching this expression at runtime will panic or abort
 --> src/main.rs:3:21
  |
3 |         let _val = &(1/0);
  |                    -^^^^^
  |                     |
  |                     dividing by zero
  |
  = note: `#[deny(const_err)]` on by default

error: erroneous constant used
 --> src/main.rs:3:20
  |
3 |         let _val = &(1/0);
  |                    ^^^^^^ referenced constant has errors

So, yeah, this should probably be a warning, similar to the overflowing arithmetic/bitshift lints.

[felix91gr]

Hmm.

Promotion cannot know if code will be evaluated or not, so that part is expected.

I mean it _could_ know.

I will be working on control-flow awareness for the const-prop optimization. Do you think it could be adapted/used for making better promoted errors?

[RalfJung]

I mean it could know.

No, it cannot. Not in general. See "halting problem". ;)

Sure we could be smarter in some special cases, but honestly I don't see the point of that. Promotion is already a horrible monster, let's not also make it path-dependent or anything like that.

[felix91gr]

Aside: correct me if I'm wrong, but I think that this issue is only relevant for non-const fn functions. For all others, it's just a thing of running them without any diagnostics or promotions at compile time and seeing if they do or do not converge (diverging meaning reaching a panic! or something similar).

Unless such a const fn is being called at runtime, in which case I have no idea. Probably that makes it equivalent to the example in the OP.

[RalfJung]

const fn are also fn and can be used at run-time like those. So in terms of promotion and diagnostics, there should be no difference between the two.

A const fn with arguments cannot just be "run at compile time to see if they diverge". And even an argument-less const fn could terminate only after the heat death of the universe.

So, no, let's just not go there. Promotion is part of code generation. Code generation should not depend on whether prior code diverges or anything like that. It doesn't solve the problem properly (there'll always be cases we cannot detect, see halting problem) and costs a huge amount of complexity.

[felix91gr]

No, it cannot. Not in general. See "halting problem". ;)

Of course, of course. But it could at least give true errors if it can prove that they will be errors, and issue warnings whenever it can't. That's what I mean ^^.

For example, in the OP it could even skip the warnings because it can be proven that no C will break it (up to wf errors).

And if you inverted the conditions in the if and else, it would tell you "hey with the zero you put in here, this will _definitely_ panic at runtime"

[RalfJung]

Please let's keep this issue focused on what kinds of lints we emit when we get an error while evaluating a promoted. Once we solved that problem and are happy with the solution, maybe we can also additionally consider whether the promoted is in dead code or not -- but considering that does not nothing to simplify the problem here (there'll always be "maybe dead code, maybe not" cases) and only makes things more complicated for no good reason.

IOW, your suggestions are scope creep. Currently we cannot even handle the general case properly. So let's fix that before even considering to do something else in special cases.

[felix91gr]

your suggestions are scope creep

I'd phrase it as "I think your suggestions are scope creep".

Please let's keep this issue focused on what kinds of lints we emit when we get an error while evaluating a promoted

But isn't precisely the issue about the lints being overly cautious? It says so in the title. Maybe you could open a different issue about the general topic.

[felix91gr]

A const fn with arguments cannot just be "run at compile time to see if they diverge". And even an argument-less const fn could terminate only after the heat death of the universe.

Related: I think this is currently how the bounds that specify const generics parameters' operations well-formedness is going to be handled. By running const fns on the const generic parameters. If you feel that running arbitrary const fns at compile time is unreasonable, do go to the discussion on Zulip because that seems to be the current solution so far. And you probably have a couple of good points to make there :)

[RalfJung]

The title was given by the OP and doesn't reflect the actual problem. As has been said above multiple times, the problem is not that the lint is too strict in some particular case or so; there is a fundamental flaw in how we emit the lint around promoteds. We have other lints for arithmetic and bitshift overflows that behave better; failing promoteds should be similar. No "is this code reachable" analysis is required for any of that.

I do not understand why you keep moving the discussion elsewhere from that problem. I won't respond to the meta-discussion here any more as it is draining and clearly leading nowhere.

[felix91gr]

I do not understand why you keep moving the discussion elsewhere from that problem.

Because I did not understand the problem. I still don't really understand it. I am definitely missing some context. The problem is more than what it seemed like to me when the issue was opened, and in actuality, the original post can be fixed with both your and my ideas independently, which didn't help me realize this.

I didn't do it with the intention to bother you, to derail the conversation or anything like that, and I'm sorry if it seemed that way at first glance 🙂

I'll see myself out. This is not a topic I can help with~

[felix91gr]

(PS: are the current labels alright? Since the real problem is now clearer, maybe the labels to have are different from the ones they were assigned originally)

[RalfJung]

Uh, I never know how to label things.^^ And we don't seem to have anything for "trouble around promotion"... amybe we should have "A-promotion" or so?

[RalfJung]

Hm, actually, arithmetic_overflow is deny-by-default as well -- I thought it was not. Curious.

So even independent of any improvements to all of these lints along the lines of what @felix91gr was imagining, we have two issues I think:

• Errors in promoteds shouldn't be const_err. const_err is planned to become a future-compat lint and then a hard error (https://github.com/rust-lang/rust/issues/71800), which is not something we can ever do for promoteds -- right, @oli-obk?
• All of these lints that determine error conditions like arithmetic overflow or div-by-zero at compile-time are currently deny-by-default. Is that really a good idea? We used to think "yes" because code like https://github.com/rust-lang/rust/issues/74696#issuecomment-665766660 is clearly odd, but with associated / generic consts, things are much less clear, as the OP example shows. We could either make the lint level depend on whether associated / generic consts are involved (which is not how lints usually work I think), or we could make them warn-by-default. (@felix91gr' analysis would be an even fancier verson of making the lint level depend on details of the error sitatuation.)

For the second point, here's an even more "grey area" example:

const C: i32 = 0;

fn main() {
    if C != 0 {
        let _val = 1/C;
    }
}

(@felix91gr also suggested having a fancy analysis that affects even whether we promote; I do not think that is something we should ever do. The decision to promote happens very early, before even MIR is constructed, so this would be technically challenging. It is also IMO conceptually a bad approach. My interpretation now is that this proposal was based on not yet having a full overview of how promotion works.)

[lcnr]

We could either make the lint level depend on whether associated / generic consts are involved (which is not how lints usually work I think)

I personally think this seems like a good idea, not really sure about

const C: i32 = 0;

fn main() {
    if C != 0 {
        let _val = 1/C;
    }
}

I think that we should err here as this fails no matter what happens without a breaking change while using const generics or associated const an expression only fails in some cases while succeeding in others

[RalfJung]

I think that we should err here as this fails no matter what happens without a breaking change while using const generics or associated const an expression only fails in some cases while succeeding in others

IMO it would be very strange if my example behaved different from yours. In both cases we first check the value of a constant before using it. C might not be public so there's nothing breaking about changing it, it might be a constant just to avoid some repetition.

Is there any precedent for making the default lint level depend on details of the error situation?
(We could of course make it two different lints, but that also seems odd.)

FWIW, my preference would be to just make the lints warn-by-default and leave it at that.

[lcnr]

In both cases we first check the value of a constant before using it.

That is not the distinction I am making here, my point is the following:

#![feature(const_generics)]

fn foo<const C: u8>() {
    if C > 0 {
        println!("Foo gives {}", 25 / C);
    } else {
        println!("Foo gives 0");
    }
}

fn main() {
    foo::<0>();
    foo::<1>();
}

Calling foo::<1>() uses 25 / C without causing a panic here. The code used here is the "right way" to write a method like this (as we use both branches depending on the value of C).

const C: i32 = 0;

fn main() {
    if C != 0 {
        let _val = 1/C;
    }
}

There is no possible instance of main (considering that it's already fully concrete) in which 1/C is actually useful. So either C is not zero, in which case 1/C does not warn, or 1/C is unreachable.

C might not be public so there's nothing breaking about changing it,

It seems quite weird to guard against the value of a local constant here.

But yeah, this doesn't seem important enough to warrant this much discussion, so I am also fine with just changing this error to warn by default.

[oli-obk]

Is there any precedent for making the default lint level depend on details of the error situation?

That's not possible at all in the current setup. We can opt to not emit the lint depending on the situation, so we could decide not to lint if there were any named constants involved (in contrast to constant values from literals)

[RalfJung]

Is there any precedent for making the default lint level depend on details of the error situation?

That's not possible at all in the current setup. We can opt to not emit the lint depending on the situation, so we could decide not to lint if there were any named constants involved (in contrast to constant values from literals)

Hm, that also sounds like a reasonable idea. Even as a warning it would be quite annoying after all. We should just try to ensure that we don't warn less than we would if things would not have gotten promoted.

But then we should do the same for the lints originating from ConstProp:

#![feature(const_generics)]

const D: u8 = 0;

fn foo<const C: u8>() {
    if C > 0 {
        let val = 25/C;
        println!("Foo gives {}", val);
    } else {
        let _val = 25/D;
        println!("Foo gives 0");
    }
}

fn main() {
    foo::<0>();
    foo::<1>();
}

This says

error: this operation will panic at runtime
  --> src/main.rs:10:20
   |
10 |         let _val = 25/D;
   |                    ^^^^ attempt to divide 25_u8 by zero
   |
   = note: `#[deny(unconditional_panic)]` on by default