NLL: need to decide match guard static control-flow semantics

[pnkfelix]

Here is a test that already exists in the run-pass suite:

fn main() {
    let x: Box<_> = box 1;

    let v = (1, 2);

    match v {
        (2, 1) if take(x) => (),
        (1, 2) if take(x) => (),
        _ => (),
    }
}

fn take<T>(_: T) -> bool { false }

The above is accepted under AST-borrowck. Unfortunately, it is rejected by MIR-borrowck:

% ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc -Z borrowck=compare ../src/test/run-pass/move-guard-const.rs
error[E0382]: use of moved value: `x` (Mir)
  --> /home/pnkfelix/Dev/Mozilla/rust.git/src/test/run-pass/move-guard-const.rs:22:24
   |
21 |         (2, 1) if take(x) => (),
   |                        - value moved here
22 |         (1, 2) if take(x) => (),
   |                        ^ value used here after move
   |
   = note: move occurs because `x` has type `std::boxed::Box<i32>`, which does not implement the `Copy` trait

error: aborting due to previous error

So here's the issue: We were hoping, in MIR-borrowck, to apply a pretty conservative semantics to model the control-flow of match arms, to maximize freedom in choosing future match-desugaring strategies.

But clearly we already allow moves in match guards in a way that we cannot just use the current naive "every guard might be executed regardless of its associated pattern" ... at least, not without marking it as a breaking-change to the language.

It might not be that hard to encode some amount of intelligence into the control-flow, in terms of modelling pattern disjointness in the arms. But its not what we're currently doing, so we need to figure out if doing that is a blocker for deploying MIR-borrowck (and thus NLL).

[nikomatsakis]

It seems that the reason we accept this program is because of a very narrow exception in the CFG generation that specifically targets "all constant" arms:

rust/src/librustc/cfg/construct.rs

Lines 498 to 526 in 619ced0

	// If both this pattern and the previous pattern
	// were free of bindings, they must consist only
	// of "constant" patterns. Note we cannot match an
	// all-constant pattern, fail the guard, and then
	// match *another* all-constant pattern. This is
	// because if the previous pattern matches, then
	// we *cannot* match this one, unless all the
	// constants are the same (which is rejected by
	// `check_match`).
	//
	// We can use this to be smarter about the flow
	// along guards. If the previous pattern matched,
	// then we know we will not visit the guard in
	// this one (whether or not the guard succeeded),
	// if the previous pattern failed, then we know
	// the guard for that pattern will not have been
	// visited. Thus, it is not possible to visit both
	// the previous guard and the current one when
	// both patterns consist only of constant
	// sub-patterns.
	//
	// However, if the above does not hold, then all
	// previous guards need to be wired to visit the
	// current guard pattern.
	if prev_has_bindings || this_has_bindings {
	while let Some(prev) = prev_guards.pop() {
	self.add_contained_edge(prev, guard_start);
	}
	}

[petrochenkov]

The "all constant" rule seems very wrong.

First, the "all the constants are the same" condition is no longer "rejected by check_match".

    match v {
        (1, 2) => (),
        (1, 2) => (), // OK, only a warning
        _ => (),
    }

Second, it was never (since 1.0) rejected in presence of if guards, e.g. this compiles on Rust 1.0

fn main() {
    let x = Box::new(1);

    let v = (1, 2);

    match v {
        (1, 2) if rand(x) => (),
        (1, 2) if rand(x) => (), // May consume `x` twice
        _ => (),
    }
}

fn rand<T>(_: T) -> bool { false /* or true */}

https://godbolt.org/g/5tdLJk

[nikomatsakis]

@petrochenkov I'm not a big fan of the rule, but then it may be we need to keep some variant of it for backwards compatibility. Still, that's a pretty interesting example -- is there a bug filed for that? Maybe we can use it as justification to simplify the code and remove this silly rule. It looks to have been put in place just to satisfy a particular run-pass test, and hopefully isn't relied upon much in crater (have to test).

[nikomatsakis]

Maybe we should whip up a branch just for cratering purposes that removes that relevant code from the CFG.

[pnkfelix]

@nikomatsakis Nice! I say that @petrochenkov 's example lets us file this under "we can issue breaking changes to fix soundness holes", because that certainly is a soundness hole, right?

[nikomatsakis]

@pnkfelix seems like it

[nikomatsakis]

The main question is how much real-world code relies on this. It's hard to imagine there being much.

[pnkfelix]

#49447 got merged, so its possible we may be able to close this once we've confirmed that no one complains about the effect of #49447

[KamilaBorowska]

This issue seems to be solved?

[pnkfelix]

@xfix ah, yes; for some reason in my head I was lumping this in with #27282, but it is clearly a distinct bug and it does seem to be resolved, given that I have not seen any complaints about #49447.