const evaluating && and || does not short-circuit

[oli-obk]

This prints false

println!("{}", false && ((1u8 - 5u8) == 42));

This gives a compile time error

const X: bool = false && ((1u8 - 5u8) == 42);
println!("{}", X);

[bluss]

The first has ‘sub with overflow’ as a warning, the second as an error.

[oli-obk]

yes, but the execution of the first statement (during runtime) works, the execution of the second statement (during compile-time) doesn't.

[steveklabnik]

/cc @rust-lang/lang is this expected or not?

[nrc]

I think this is a bug, || and && should have the same semantics in a const as in any other context

[nikomatsakis]

Mmm, I'm not sure. I think this is expected, for the reason @oli-obk states.

[nikomatsakis]

I take it back. I'm not 100% sure what I think.

[nikomatsakis]

Put another way, I think the warning-vs-error thing is a red herring. The question is whether constant evaluation should ever look at the RHS. If it does, then the warning-vs-error is appropriate. That is, if you swapped the order of the arguments to &&, this would be unambiguously the correct behavior.

[nrc]

Right, I agree warning vs error is ok, but that the rhs should not be evaluated.

[nikomatsakis]

OK, I agree. Bug.

[ranma42]

I am not sure if it belongs to the same bug report, but I would also expect

println!("{}", if false { ((1u8 - 5u8) == 42) } else { false });

to report the same warning as

println!("{}", false && ((1u8 - 5u8) == 42));

Instead, the compiler seem to completely disregard the overflow in the first branch of if false, possibly because it detects at compile time that it the overflow is unreachable.

I am not sure if it is desirable for the compiler to report about issues found in dead code or if they should just be "hidden" behind a warning about the dead code. I would definitely not expect them to cause error (as in the const+&& case) and I think they should also not be silently ignored (as in the if false case).

[oli-obk]

@ranma42 yes, the behavior will now be the same. The warning will be gone since the second expression is unreachable.

The issue with "dead code" here is configurability. Maybe the first boolean expression is a value that is somewhat dynamic:

const N: usize = 0;
const ARR: [i32; N] = [42; N];
const X: bool = (N > 0) && ARR[0] == 99;

This code should not report a warning or an error about ARR[0] being unreachable or erroneous.

[ranma42]

@oli-obk I agree that in const-evaluated code it might be undesirable to have these warnings, but at least in "normal" code (including my first example with if) I would rather have a warning (at least about the dead code).

In my opinion, the different warning behaviour between const-evaluated expressions and expressions that are meant for runtime evaluation but that are optimised away could be reasonable. If I expect a boolean expression to be const-evaluated, I am assuming that some of it might be dead code (because of the shortcut operators).

Instead, for normal expressions, my basic assumption is that the compiler is unable to evaluate them before runtime; if they can be evaluated at compile time (at least partially, i.e. if they have dead code), I probably wrote something wrong. If I intentionally write a normal expression with dead code, I would not be surprised if the compiler required me some additional annotation to suppress the warning (cfr. the unused variable warning suppressed by the underscore prefix).

[oli-obk]

I see your point. That is indeed a different issue and should have it's own issue number. This issue is solely about constant evaluation of the && and || operators

[ranma42]

Right, added as #29803.

[oli-obk]

This issue will be postponed until the two const evaluators have been unified

[varkor]

@oli-obk: will this be fixed by #46882?

[oli-obk]

Oh good point. I'm assuming this'll be solved partially.

false && ((1u8 - 5u8) == 42 will not error, instead produce a warning

[oli-obk]

I'll add some tests

[oli-obk]

Nevermind, this is not fixed. After #46882 is merged, it should be as simple as removing all constness checks from

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/hair/cx/expr.rs#L292

[gnzlbg]

What's blocking this?

[oli-obk]

#49146 is the tracking issue for if and match. Implementing that will automatically resolve && and ||

[oli-obk]

This has been solved together with #49146