Wrong result after short circuit in const_let

[dtolnay]

I would expect this program to print true. As of rustc 1.30.0-nightly (33b923f 2018-08-18) it prints false. If you change const fn to fn then it prints true as expected.

#![feature(const_fn, const_let)]

const fn id(condition: bool) -> bool {
    let mut ret = true;

    // If condition is true, we short circuit and ret remains true.
    // If condition is false, we assign ret = false.
    let _ = condition || { ret = false; true };

    ret
}

fn main() {
    println!("{}", id(true));
}

Mentioning @oli-obk.
Mentioning const_let tracking issue #48821.

[oli-obk]

Uh.... Apparently short circuiting operators don't exist in consts:

rust/src/librustc_mir/hair/cx/expr.rs

Lines 328 to 358 in d2048b6

	// FIXME(eddyb) use logical ops in constants when
	// they can handle that kind of control-flow.
	(hir::BinOpKind::And, hir::Constness::Const) => {
	ExprKind::Binary {
	op: BinOp::BitAnd,
	lhs: lhs.to_ref(),
	rhs: rhs.to_ref(),
	}
	}
	(hir::BinOpKind::Or, hir::Constness::Const) => {
	ExprKind::Binary {
	op: BinOp::BitOr,
	lhs: lhs.to_ref(),
	rhs: rhs.to_ref(),
	}
	}
	(hir::BinOpKind::And, hir::Constness::NotConst) => {
	ExprKind::LogicalOp {
	op: LogicalOp::And,
	lhs: lhs.to_ref(),
	rhs: rhs.to_ref(),
	}
	}
	(hir::BinOpKind::Or, hir::Constness::NotConst) => {
	ExprKind::LogicalOp {
	op: LogicalOp::Or,
	lhs: lhs.to_ref(),
	rhs: rhs.to_ref(),
	}
	}

cc @eddyb

Since we did not have a way to observe short circuiting before the const_let feature, this transformation was correct.

Note that const fn is unnecessary:

#![feature(const_let)]

const COND: bool = true;

const X: bool = {
    let mut ret = true;

    // If COND is true, we short circuit and ret remains true.
    // If COND is false, we assign ret = false.
    let _ = COND || { ret = false; true };

    ret
};

fn main() {
    println!("{}", X);
}

[eddyb]

Oh wow, whoops, at least we haven't stabilized anything that could observe this, whew.

[RalfJung]

So where is the tracking issue to allow if in const so that we can get rid of this hack?

if seems like a fairly innocent feature, even more so than loop.

[oli-obk]

if is very innocent if you ignore associated constant bodies and using associated constants in generic functions (where the associated constant is taken from a generic parameter).

@eddyb can we just mark any MIR containing SwitchInt to be NOT_PROMOTABLE, NEEDS_DROP and MUTABLE_INTERIOR (https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L51) even if they aren't? And then later loosen the constraints?

[RalfJung]

if is very innocent if you ignore associated constant bodies and using associated constants in generic functions (where the associated constant is taken from a generic parameter).

What is the problem there?

[oli-obk]

We need to check whether

if T::FOO {
    Some(Cell::new(42))
} else {
    None
}

results in a Freeze value or not. While this example is fairly easy, once you get local variables and mutable state it gets very messy and needs a proper dataflow analysis.

Since (Some(Cell::new(42)), None).1 currently counts as "contains interior mutability", imo we can just say that if true { None } else { Some(Cell::new(42)) } also does

[RalfJung]

Oh, we are already more precise than the type is?

[RalfJung]

I think the place that has the best information about this kind of stuff is the sanity check. So if we make that part of the query for constants, it could probably do this.

[Centril]

(Noting that) this no longer compiles on nightly:

error[E0019]: constant function contains unimplemented expression type
 --> src/main.rs:8:13
  |
8 |     let _ = condition || { ret = false; true };
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

[RalfJung]

So I think this can be closed because there's no longer an incorrect result. See #49146 for the tracking issue for control flow in const fn (that includes short-circuiting).