Errors in constants and promoteds stop compilation (via deny-by-default 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

Other example (by @RalfJung)

const D: u8 = 0; // imagine this is in a different crate

pub fn foo() {
    if D > 0 {
        let _val = 25/D; // ERROR
    } else {
        panic!()
    }
}

[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 😄

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 #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 rust-lang/rfcs#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 #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 (Make const_err a future-incompatibility lint and eventually a hard error #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 Errors in constants and promoteds stop compilation (via deny-by-default lint) even in dead code #74696 (comment) 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

[RalfJung]

#80579 (if we can land it) will stop promoting division unless we can prove the divisor is non-zero, so the original error here is gone.

However, this issue remains -- the unconditional_panic (and arithmetic_overflow) lints are emitted more often than we might like; maybe we should make them smarter and/or warn-by-default instead of deny-by-default.

[oli-obk]

maybe we should make them smarter

@felix91gr is working on that

[felix91gr]

Indeed. If the mir-opts are available at this level, you should have better info about this kind of code after I'm done. I'm working on using dataflow for constant folding, which will be able to ignore dead code and in general, be aware of the control flow of the program :)

[RalfJung]

I was talking about lints, not constant folding... so I am a bit confused now.

Better constant folding sounds great. :) However, I don't think it is a great idea to conflate optimizations and lints too much, that seems extremely fragile.

Above we had rather simple proposals like checking if there are named constants involved... what happened to those?^^

[felix91gr]

Ah, my bad. Okay, so if this is happening at the level of lints... (sorry, it's been a while since I read this thread) hmm...

If the compiler lints, like some clippy lints do, are using constant folding to know some variable values in advance, then this kind of error should be fixed after my changes are done. Unless the error stems from somewhere else (which it might!)

I agree with being tied to optimizations being a fragile thing, however in this case I tend to look at the usage of mir opts in this way: "can I know this value at compile time?". And if the optimizations are sound (which should be the case, otherwise the analysis itself becomes unsound), then I feel it's fine to use them to infer more about the code being processed.

[felix91gr]

PS: oh god I'm re-reading this issue and I might've done it again 🤦🏻 sorry @RalfJung, I owe your patience twice now.

Okay so what Oli is referring to is that many of those deny lints use some level of const-prop to infer errors at compile time. This is fine, only problem is that our const-prop engine is still too weak. And even though it's conservative (as it should be), for some reason in the context of const_err it isn't. It gives false negatives; something is amiss there.

The work I'm doing should hopefully make the const-prop engine much more precise and complete than it is right now, without making it less conservative. So that should help make a lot more things "smarter" in the scenarios posted in this thread.

I think there is something else going on there though... since it should never deny valid code.

Again Ralf, my apologies. I got things mixed up again.

[RalfJung]

That makes sense... but I am not sure yet how it affects examples like this:

#![feature(const_generics)]

const D: u8 = 0;

pub fn foo<const C: u8>() {
    if C > 0 {
        let _val = 25/C;
    } else if D > 0 {
        let _val = 25/D; // ERROR
    } else {
        panic!()
    }
}

Before we said the best approach to avoid these errors is to not lint when the expression involves a named const.

Are you saying that, instead, we should rely on the underlying analysis being smarter about detecting dead code? Will that truly be good enough? Lints and optimizations have somewhat dual constraints here... lints should avoid false positives (so "considering more code dead" is okay), but optimizations must never miss a potential case.

[felix91gr]

Are you saying that, instead, we should rely on the underlying analysis being smarter about detecting dead code?

I'm confident that in this example, dataflow-based const prop will ignore the second branch (else if D > 0), since it is aware of control flow and it already knows the value of D. Basically, that branch should never be visited during the analysis.

This example also shows a soundness issue* with the current const prop engine: a named const should not be propagated into dead code.

* though a minor one, since it invalidates valid code, as opposed to validating invalid code which would be truly disastrous.

Lints and optimizations have somewhat dual constraints here... lints should avoid false positives (so "considering more code dead" is okay), but optimizations must never miss a potential case.

I think I see what you mean there. In this context though, I think we're on an edge case of that duality. Since const prop is intended more as process of enrichment of the MIR.

I think the only way I can illustrate it is by pointing to lints which require good const-prop to work. Take a look at this one: rust-lang/rust-clippy#628

Before we said the best approach to avoid these errors is to not lint when the expression involves a named const.

This seems very valid to me as a workaround while I work on the new implementation! :)

[RalfJung]

This example also shows a soundness issue* with the current const prop engine: a named const should not be propagated into dead code.

Why not?

[felix91gr]

The reason I'm thinking of is this:

Dead code will never happen, and therefore the asserts inside it will never happen either. Yet division, which has an assert inside it ("the divisor is not 0") will be processed by constprop.

Asserts become meaningless in dead code (right? Unless I'm missing something in the style of Why even unused data needs to be valid). And therefore if we fail an assert in dead code, we're rejecting code that should be accepted otherwise.

Propagating named constants into dead code (and indeed, anything into dead code) has the potential to do this, and therefore is something that we'd rather not do.

Does that make sense?

[RalfJung]

Incorrectly emitting a lint is not a soundness issue though. A soundness issue is when you can cause UB in safe code. Also see the UCG definition of soundness.

Sure, emitting lints about dead code might be considered false positives for the lint, but OTOH we do perform type-checking even in dead code and type errors in dead code are not false positives, either.

[felix91gr]

Ah, my bad. You're right. The word I should be using is "completeness" instead of soundness, then, right?

So it's be like this:

• False positive: analysis is less complete because we're rejecting valid code.
• False negative: analysis is not sound anymore because we're accepting invalid code.

but OTOH we do perform type-checking even in dead code and type errors in dead code are not false positives, either.

I wonder if there's a fundamental difference in this context between type checking and "value checking" (by which I mean asserts based on values in the program).

Because yeah, I think it makes sense to do type checking even for dead code. But the liveness of code is mostly a function of the compile-time available values, right? So maybe they are different... and checking values in dead code can be considered unnecessary?

[RalfJung]

Completeness of the lint would be technically correct I think, but to avoid confusion I think it is easier to talk about false-negatives / false-positives of the lint.

I wonder if there's a fundamental difference in this context between type checking and "value checking" (by which I mean asserts based on values in the program).

Types like NonZero* kind of blur that line, though. ;)

[RalfJung]

Also see #81265 for the kinds of problems that are caused by using an optimization pass to emit lints. I am more and more convinced that that's just bad design. It leads to coupling effects that are very hard to control.

[felix91gr]

I think this is a valuable feature to have, but it's a pain when it gives false positives, and we should change that. Is there a model or a set of rules that we could use to decide where it makes sense to emit lints?

I like the idea of piggybacking on certain optimizations for the purpose of emitting lints, because many of the operations used in the optimizations can also be used to conclude truths about the code. Ideally I'd want them to exist separately, since the overlap of areas might not be the same.

(I was gonna mention dead code, but dead code should stop being a problem after I add the non-lexical constprop pass)

Edit: sorry, I forgot to add the following 2 paragraphs.

So ideally I'd want things to exist separately. It's very useful to have a lint alert you of a division-by-0 that's not obvious when reading the code but becomes clear after processing it in the MIR. This is currently done at the optimization level, by triggering asserts (like divisor is different from 0 in this case) whenever you can evaluate them at compile time. It should be feasible to add this computation and linting process to a different, separate analysis which only looked at code for the purpose of detecting errors and warnings.

However, some linting will have to happen at the optimization level, unless we can prove before optimizing that no asserts will be triggered. I think currently we're at the point of "optimization triggers a lot of lints, and some are false positives". I think the next step might be "optimization triggers a lot of lints, which rarely are false positives", and the one after that "optimization almost never triggers lints, and most lints that used to come from opts now come from MIRI-based analyses".

[RalfJung]

@lcnr promotion changed a lot since the report was made, and the main example in your post no longer shows an error. The 2nd example needs #![allow(unconditional_panic)] but then it works. Is the bug fixed? If not, could you update the example?

[lcnr]

i think that's fine 👍