Stabilize bind_by_move_pattern_guards in Rust 1.39.0

[Centril]

Closes #15287.

After stabilizing #![feature(bind_by_move_pattern_guards)], you can now use bind-by-move bindings in patterns and take references to those bindings in if guards of match expressions. For example, the following now becomes legal:

fn main() {
    let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

    match array {
        nums
//      ---- `nums` is bound by move.
            if nums.iter().sum::<u8>() == 10
//                 ^------ `.iter()` implicitly takes a reference to `nums`.
        => {
            drop(nums);
//          --------- Legal as `nums` was bound by move and so we have ownership.
        }
        _ => unreachable!(),
    }
}

r? @matthewjasper

[Centril]

Stabilization proposal

I propose that we stabilize #![feature(bind_by_move_pattern_guards)].

@rfcbot merge

Tracking issue: #15287
Version target: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

What is stabilized

After stabilizing #![feature(bind_by_move_pattern_guards)], you can now use bind-by-move bindings in patterns and take references to those bindings in if guards of match expressions. For example, the following now becomes legal:

fn main() {
    let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

    match array {
        nums
//      ---- `nums` is bound by move.
            if nums.iter().sum::<u8>() == 10
//                 ^------ `.iter()` implicitly takes a reference to `nums`.
        => {
            drop(nums);
//          --------- Legal as `nums` was bound by move and so we have ownership.
        }
        _ => unreachable!(),
    }
}

Why?

Stabilizing #![feature(bind_by_move_pattern_guards)] has a few notable justifications:

1. It allows you to continue using match guards when this is more natural. This is in particular useful when there is a catch-all arm which would need to be otherwise duplicated if the guards are moved into the match arm bodies using if expressions. Worse, the standard rustfmt style would move these if expressions into a block thereby adding rightward drift. This in turn harms readability.

2. The checks made without #![feature(bind_by_move_pattern_guards)] are redundant as the NLL borrow checker already does the actual checks necessary for soundness. By removing the redundant checks, we can simplify the compiler and the language as seen in this PR.

Technical details

The following is lifted directly, in a slightly adjusted form, from @matthewjasper's technical report.

Bind by move guards

A by-value binding in a pattern with an if guard is lowered as follows:

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

For example, in:

let return_place = match scrutinee {
    binding if fun(&binding) => binding,
    ...
}

would lower to roughly:

let return_place;

let __binding_ref = &__scrutinee_tmp;
if !fun(&*__binding_ref)
    goto next_arm

let binding = __scrutinee_tmp;
return_place = binding;

Note: This lowering is observable in how we promote temporaries and how long different borrows can last.

Checking for mutation

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 $expr }.
• A reference / pointer / box that is dereferenced to access a compared value or a binding, e.g., x in match x { &(false, _) if $expr or match x { &v if $expr }.

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

The tests can be primarily seen in the PR itself. Here are some of them:

• Feature gate test being removed

• A basic test of the static and dynamic semantics of the feature.

• You cannot mutate in a match arm using a closure. This was previously a soundness hole plugged by Make #![feature(bind_by_move_pattern_guards)] sound without #[feature(nll)] #63059 for stable Rust.

• You cannot drop(..) the match scrutinee in a guard when it is then bound in another arm. This was previously a soundness hole plugged by Make #![feature(bind_by_move_pattern_guards)] sound without #[feature(nll)] #63059 for stable Rust.

• You cannot assign to a mutably borrowed match scrutinee in a guard.

• You cannot mutably reborrow a ref mut borrow in a guard since it is immutable in the guard.

• Avoid exposing the exact matching algorithm in borrow checking for forward compatibility.

• Dynamic semantics: ref mut variables don't change address between the guard and the arm expression.

• A partially mutably borrowed place can be matched on, so long as we don't have to read any values that are mutably borrowed to determine which arm to take. Also tests that we don't allow mutating the value being matched on in a way that changes which patterns it matches, until we have chosen an arm.

• A test of the dynamic and static semantics using interior mutability in guards.

• Under NLL + bind_by_move_pattern_guards, we can use a less conservative approach for disallowing mutations in guards. More specifically, here we test that a guard such as _ if slice.iter().any(|&x| x == 2) => true, where a mutable borrow of the iterator is taken by .any(..) but does not mutate the scrutinee, is allowed.

• A simple passing test demonstrating that E0008 is now removed. This is a good test showing the basic functionality.

• The E0008 error code test being removed.

• The E0301 error code test being removed as it is now covered by E0596 instead.

• The E0302 error code test being removed as it is now covered by E0594 instead.

• The borrow checker correctly rejects a use-after-free by dropping in a guard and then using the dropped value in another arm.

• The borrow checker correctly rejects a use-after-free by dropping in a guard and then using the dropped value in the same arm.

• A mir-opt test checking that StorageDead and Drops are generated properly for bindings in matches.

History

• On 2014-05-17, the idea of bind-by-move guards is raised in Allow by-move binding for patterns with guards that don't reference the bindings #14252.

• On 2014-06-04, @zwarich proposes pattern guards with bind-by-move in RFC: Pattern Guards with Bind-By-Move rfcs#107 which is subsequently accepted on 2014-07-01.

• On 2018-05-04, the RFC is "effectively implemented" by NLL as part of Immutably and implicitly borrow all pattern ids for their guards (NLL only) #49870 which was written @pnkfelix and reviewed by @nikomatsakis as well as @arielb1.

• On 2018-09-18, the actual feature gate bind_by_move_pattern_guards as well as tests are added in Add feature to enable bind by move pattern guards #54034 which was written by @pnkfelix and reviewed by @nikomatsakis and @varkor.

• On 2018-10-01, the -Z disable_ast_check_for_mutation_in_guard flag is removed and the feature gate is used in some tests instead in Remove -Z disable_ast_check_for_mutation_in_guard #54676 which was written by @pnkfelix and reviewed by @alexcrichton.

• On 2019-01-03, the feature is discussed by the language team. @nikomatsakis expressed the wish for NLL progressing further on both editions before stabilizing the feature.

• On 2019-05-02, the feature was again discussed by the language team. We noted that we should prepare the feature for stabilization and asked @matthewjasper to provide a report.

• On 2019-05-04, @matthewjasper provides a report on the feature gate upon which the current stabilization report is based on. The report provides a motivation, an explanation, and highlights soundness holes without #![feature(nll)] a few tests which are lacking.

• On 2019-08-04, the soundness holes, when using #![feature(bind_by_move_pattern_guards)] without also using #![feature(nll)], as noted by @matthewjasper are plugged in Make #![feature(bind_by_move_pattern_guards)] sound without #[feature(nll)] #63059 which was written by @Centril and reviewed by @matthewjasper. The soundness holes are fixed by avoiding to downgrade NLL errors as warnings in certain circumstances.

• This PR stabilizes bind_by_move_pattern_guards.

Related future work

As @matthewjasper notes in the pre-report:

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)

Apendix

How does the compiler detect the need for a bind-by-move?

Summary

An explanation of binding modes exists in the reference. The following summary is distilled from the algorithm below which is in turn a summary of the actual librustc_typeck source in fn check_pat_walk. Please note that what changes here is to allow taking references in if guards for existing by-move bindings. The algorithm for determining binding modes remains unchanged.

The default-binding-mode:

• starts out as the one provided by the outer context or by-value if there is no outer context.
• becomes by-value on a &mut? pattern.
• becomes by-reference on a non_ref_pat.

A binding is:

• by-reference when it is a ref mut? binding,
• by-value when it is a mut binding,
• by-the-default-binding-mode otherwise.

A by-value binding is by-copy if the type is Copy and by-move otherwise.

Algorithm for binding modes

The default binding mode (def_bm)

1. The initial def_bm is provided by the outer checking context (or BindByValue if it is the outer-most pattern).

2. non_ref_pat is defined as all pattern forms except:

   • a literal pattern where the type resolves to a reference type,
   • a path pattern to a constant (free, inherent, or associated),
   • a wildcard pattern _,
   • an identifier pattern (ref mut?)? $ident (@ $pat)?

3. The pattern is a reference pattern, set def_bm = BindByValue.

4. The pattern is a non_ref_pat and the scrutinee expression (e.g. match scrutinee { ... }) is of a reference type:

   • Set def_bm = BindByReference.

The binding mode

1. When the pattern is an identifier pattern:

   • On $ident (@ $pat)?, the binding mode is def_bm.
   • On mut $ident (@ $pat)?, the binding mode is BindByValue.
   • Otherwise (ref mut? $ident (@ $pat)?), the binding mode is BindByReference.

   Then proceed recursively, passing the local def_bm, determining the binding modes of the sub-pattern if there is one (the $pat in @ $pat).

2. Otherwise proceed recursively, passing the local def_bm, determining the binding modes of any sub-patterns.

When the binding mode is by-move

The binding mode is bind-by-move when it is BindByValue and the type is not Copy:

fn foo() {
    #[derive(PartialEq, Clone, Copy)] // `x` is by-value-copy ==> OK today.
    #[derive(PartialEq, Clone)] // `x` is by-value-move ==> ERROR today.
    struct S;

    match S {
        x // `x` is by-value.
            if x == S => drop(x),
        _ => {}
    }
}

[rfcbot]

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Centril
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

I'm trying to understand better. Under what circumstances will the compiler use a bind-by-move pattern, and under what circumstances will it not? I saw the example, but that just shows me when the compiler will do so, not when it won't. How does the compiler detect the need for a bind-by-move?

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[matthewjasper]

@bors r+

[bors]

📌 Commit aaa9762 has been approved by matthewjasper

[bors]

⌛ Testing commit aaa9762 with merge 45859b7...

[bors]

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 45859b7 to master...