Tracking issue for Pattern Guards with Bind-By-Move

[alexcrichton]

Tracking issue for "Pattern Guards with Bind-By-Move" (rust-lang/rfcs#107)

Steps:

• Implement the RFC (already done as part of NLL work)
• Adjust documentation (see instructions on forge)
• Stabilization PR (see instructions on forge)

[jonhoo]

I keep running into this (or rather, the simpler #14252) in places where I now end up with an extra level of indentation:

match x {
    Foo(v) => if do_a {
            // ...
        } else if do_b {
            // ...
        } else if do_c {
            // ...
        } else {
            // ...
        }
}

when it could be

match x {
    Foo(v) if do_a => {
        // ...
    },
    Foo(v) if do_b => {
        // ...
    },
    Foo(v) if do_c => {
        // ...
    },
    Foo(v) => {
        // ...
  }
}

[pnkfelix]

(i believe this feature is effectively implemented by NLL, namely as PR #49870. I should probably port any examples, including corner case examples noted on PR #42088, to the NLL test suite just to make sure.)

[pnkfelix]

Actually perhaps the right thing for me to do would be to put in the feature(bind_by_move_pattern_guards) and then have that, when NLL is enabled, be the thing that toggles what is currently controlled by -Z disable_ast_check_for_mutation_in_guard

[pnkfelix]

Now that PR #54034 has landed, I'm unassigning myself.

(But, if I understand correctly, the issue should stay open until feature(bind_by_move_pattern_guards) gets stabilized)

[pnkfelix]

(I guess it would be good to remove the -Zdisable-ast-check-for-mutation-in-guard flag and replaces uses of it in our test suite with #![feature(bind_by_move_pattern_guards)])

[Centril]

We discussed this in the language team meeting pre-triage (but not the proper language team meeting; cc @rust-lang/lang). Of note, with respect to #15287 (comment), is that NLL has been stabilized on both editions.

Therefore, the participants at the pre-triage (e.g. me, @nikomatsakis, and @pnkfelix) found that we should have "clearance to prep for a stabilization report". This would entail writing up a report (example: #57175 (comment)) of:

• what the feature does (hopefully in a way that has enough info to incorporate into the reference eventually),
• short motivation,
• relevant test files.
• other stuff if relevant.

@matthewjasper Since you've been working on things related to this, do you think you could survey the implementation and tests and check whether improvements need to be made in either and then possibly write up a report?

[matthewjasper]

Motivation

The naive lowering of a bind-by-move pattern with a guard in a match expression would result in the use of a moved value if the guard returns false. As such we currently emit this error if the user tries to do this:

• E0008 - Cannot bind-by-move for a pattern with a guard.

The error explanation provides a more detailed explanation of the problem.

The checks for match exhaustiveness assume that the place being matched on is not modified until an arm is chosen, which is currently (mostly) checked with these errors:

• E0301 - Cannot mutably borrow in a pattern guard
• E0302 - Cannot assign in a pattern guard

Note: The current checks are not sufficient, leading to soundness bugs (see #27282)

These are now handled by careful lowering to MIR so that the MIR borrow checker will check them for soundness. As these errors are unnecessary and occasionally come up, this feature removes them.

Explanation

The feature gate currently disables the checks for the above errors.

Bind by move guards

We cannot handle these in the simplest way for the reasons given above. A by value binding in a pattern with a guard is lowered as follows:

• Before evaluating the guard we create a binding by shared reference
• In the guard we use the shared reference when the binding is accessed.
• If the guard evaluates to true then we do the expected binding.

For example: let u = match x { v if f(&v) => v, ... } would become something like

let u;
let __v_ref = &x;
if !f(&*__v_ref) goto next_arm
let v = x;
u = v;

Note: This lowering is observable in how we promote temporaries, how long different borrows can last and in some error messages. I hope to address the error messages soon.

Checking for mutation

Note: This is already active (emitting warnings), since it catches additional soundness issues to the old check.

To handle the cases in E0301 and E0302 "fake borrows" are added to the parts of the match scrutinee in the guards. More precisely: a shallow borrow is taken of any place that is:

• Compared to a value, e.g. x.0 in match x { (false, _) if ... }
• A reference/pointer/box that is dereferenced to access a compared value or a binding, for example x in match x { &(false, _) if ... or match x { &v if ... }

The borrow lasts from the start of the guard to just after the guard is evaluated successfully. As such if a guard unconditionally diverges, then the borrow will not be active in the guard. A shallow borrow only affects a place and its parents. A shallow borrow of x.0 will prevent mutation of x and x.0, but not *(x.0) or x.0.f.

Tests

As a general observation, most of the tests are using #![feature(nll)] at the moment, so aren't testing what we would be stabilizing if we want to stabilize this soon.

Bind by move guards

The tests for this feature are here:

https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-0107-bind-by-move-pattern-guards

Of particular note is https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-0107-bind-by-move-pattern-guards/rfc-reject-double-move-across-arms.rs. This test compiles in migrate mode, albeit with a scary warning, and will double free a Vec.

Mutation in guards

• https://github.com/rust-lang/rust/blob/master/src/test/run-pass/issues/issue-24535-allow-mutable-borrow-in-match-guard.rs
• https://github.com/rust-lang/rust/blob/master/src/test/ui/nll/match-guards-partially-borrow.rs

Work before stabilization

• Update tests to use the migrate borrow check mode where possible.
• Make the new warnings/errors always be hard errors, or decide that stabilizing unsound code is OK.
  • One solution to this is to treat the errors that are no longer emitted as being AST borrow check errors for the purposes of migrate mode.
  • Another is to wait for full NLL stabilization.

Related work

The following errors are also apparently unnecessary restrictions on patterns. There should probably be a check for why these are errors and whether they can be removed.

• E0007 - cannot bind in a subpattern
• E0009 - cannot bind by ref and by move
• E0303 - cannot bind in a subpattern (again)

[Centril]

@matthewjasper Thanks for the report!

As a general observation, most of the tests are using #![feature(nll)] at the moment, so aren't testing what we would be stabilizing if we want to stabilize this soon.

So we would need to adjust some tests then to make sure that stable behavior is exercised.

Of particular note is https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-0107-bind-by-move-pattern-guards/rfc-reject-double-move-across-arms.rs. This test compiles in migrate mode, albeit with a scary warning, and will double free a Vec.

Make the new warnings/errors always be hard errors, or decide that stabilizing unsound code is OK.

Let's not pick the soundness hole solution... ;)

• One solution to this is to treat the errors that are no longer emitted as being AST borrow check errors for the purposes of migrate mode.

So this would mean that we don't introduce new unsound behavior on stable, right?

• Another is to wait for full NLL stabilization.

Given that we've transitioned into NLL on both 2018 & 2015 now, what is your guesstimate as to when we can do full NLL stabilization?

Related work

Let's follow up with fresh issues?

[matthewjasper]

• One solution to this is to treat the errors that are no longer emitted as being AST borrow check errors for the purposes of migrate mode.

So this would mean that we don't introduce new unsound behavior on stable, right?

Correct

• Another is to wait for full NLL stabilization.

Given that we've transitioned into NLL on both 2018 & 2015 now, what is your guesstimate as to when we can do full NLL stabilization?

This year. (1.39 or 1.40)

[Centril]

Cool;

• One solution to this is to treat the errors that are no longer emitted as being AST borrow check errors for the purposes of migrate mode.

So this would mean that we don't introduce new unsound behavior on stable, right?

Correct

@nikomatsakis @pnkfelix Interested to hear what you think about this. My hunch is to say that we should do it assuming we have the tests to show that it works.

[nikomatsakis]

We discussed in the lang team meeting -- treating the existing errors as "AST borrowck errors" for purpose of migration mode seems like a good solution, and we are generally in favor of moving forward here once that is done plus test cases are finished.

[matthewjasper]

If anyone wants to work on this then some steps to get started are

• Find any tests that use feature(bind_by_move_pattern_guards) and feature(nll) or compile-flags: -Zborrowck=mir and remove the nll or -Zborrowck=mir flag.
  • Note any tests that now fail (i.e. the code now compiles with only warnings), make sure that they fail to compile with no feature flags.
• Make the check_match query return SignalledError. If we emit an error, or suppress an error due to bind_by_move_pattern_guards, we should return SawSomeError.
• Make the borrowck query first check check_match, and if it returns SawSomeError, then borrowck should not bother running and just return BorrowCheckResult { signalled_any_error: SawSomeError } (the other field is removed in Remove rustc_mir dependency from rustc_borrowck #61590)
• Check that all of the tests now pass.

[Centril]

Plan in #15287 (comment) was implemented in #63059.