[nll] `_` patterns should not count as borrows

[nikomatsakis]

Historically, we have considered let _ = foo to be a no-op. That is, it does not read or "access" foo in any way. This is why the following code compiles normally. However, it does NOT compile with NLL, because we have an "artificial read" of the matched value (or so it seems):

#![feature(nll)]
#![allow(unused_variables)]

struct Vi<'a> {
    ed: Editor<'a>,
}

struct Editor<'a> {
    ctx: &'a mut Context,
}

struct Context {
    data: bool,
}

impl Context {
    fn read_line(&mut self) {
        let ed = Editor { ctx: self };
        
        match self.data {
            _ => {
                let vi = Vi { ed };
            }
        }
    }
}

fn main() {
}

Found in liner-0.4.4.

I believe that we added this artificial read in order to fix #47412, which had to do with enum reads and so forth. It seems like that fix was a bit too strong (cc @eddyb).

UPDATE: Current status as of 2018-10-02

Thing	AST	MIR	Want	Example	Issue
let _ = <unsafe-field>	💚	💚	❌	playground	#54003
match <unsafe_field> { _ => () }	❌	❌	❌	playground	#54003
let _ = <moved>	💚	💚	💚	playground	💯
match <moved> { _ => () }	❌	❌	💚	playground
let _ = <borrowed>	💚	💚	💚	playground	💯
match <borrowed> { _ => () }	💚	💚	💚	playground	💯

[nikomatsakis]

OK, the code is sort of inconsistent right now with respect to what let _ = foo permits. Here is my analysis of the current state:

Thing	AST	MIR	Example
let _ = <unsafe-field>	💚	💚	playground
match <unsafe_field> { _ => () }	❌	❌	playground
let _ = <moved>	💚	💚	playground
match <moved> { _ => () }	❌	❌	playground
let _ = <borrowed>	💚	💚	playground
match <borrowed> { _ => () }	💚	❌	playground

The explanations for what is going on here in the code are:

• let _ = ... processes the pattern in a simplified, irrefutable mode during MIR processing. This mode does not insert the dummy accesses that the match mode does (because there is no need, generally speaking). This means that let _ = some.path is always a no-op and just doesn't appear in the MIR at all.
• match <place> { _ => () } in contrast uses the "match mode" which can handle any number of arms.
• Unsafe code checking happens on MIR, which I now think is mildly wrong (HAIR seems correct to me)

I'm not 100% sure why the old borrow check acepts match <borrowed> { _ => () }; the EUV does seem to issue a "match discriminant" borrow for match <..>. I'd have to dig in more.

[nikomatsakis]

Given the chart above, it seems like we could call the current behavior of NLL a kind of "bug fix" when it comes to match, though I personally find the inconsistency with let _ = <borrowed> disquieting.

[nikomatsakis]

cc @rust-lang/lang -- I'd like opinions on what behavior we think should happen around _ patterns. In general, my mental model has been that _ patterns are "no-ops". They do not "access or touch" the value that they are matching against. And that is certainly true some of the time. For example, we accept this code:

fn main() {
    let foo = (Box::new(22), Box::new(44));
    match foo { (_, y) => () }
    drop(foo.0); // ok, foo.0 was not moved
}

However, we seem to require that you can only match on things are that are "fully present" and not (e.g.) partially moved. Hence it is an error to swap the drop and match in that example.

This requirement that the match discriminant be valid is actually consistent with NLL's view on matching: NLL views the match desugaring as first borrowing the value that we are going to match upon (with a shared borrow). This borrow persists until an arm is chosen. This prevents match guards from doing weird stuff like mutating the value we are matching on. Requiring the match discriminant to be valid is also consistent with some of the discussions we've had about how to think about exhaustiveness and matches with zero arms like match x { }.

That said I also think that let _ = ... and match <block> { _ => () } should be equivalent. So perhaps we should "fix" the behavior of let_ = ...? Or do we special-case match expressions with a single arm to say that they do not have a "tracking borrow" (in which case we would accept the examples above).

I would also -- personally -- potentially draw a distinction between the "unsafe check" and the checks for what data is moved. I don't think of the unsafe check as a "flow sensitive" check but rather something very simple -- if you access the field, even in dead code, you must be in an unsafe block. Hence I am not too thrilled that let _ = <unsafe fields> would type-check, and I consider that a regression from the MIR-based borrow check. In fact, this is basically why I don't think that MIR is a good fit for unsafe checking (HAIR would be better).

[scottmcm]

While I've learned that _ is a complete no-op, I've always found it weird that

let _ = mutex.lock().unwrap();

and

let _guard = mutex.lock().unwrap();

do fundamentally different things.

I expected _ to behave as if the compiler gave it a name, like how argument-position '_ works.

[nikomatsakis]

@scottmcm understood — but I think too late to change that. =)

Update: (It would also mean that there is no way to extract some fields of a struct without moving the rest... unless we had some other pattern for "don't touch")

[nikomatsakis]

This is what I personally think the table should look like:

Thing	AST	MIR	Example
let _ = <unsafe-field>	❌	❌	playground
match <unsafe_field> { _ => () }	❌	❌	playground
let _ = <moved>	💚	💚	playground
match <moved> { _ => () }	💚	💚	playground
let _ = <borrowed>	💚	💚	playground
match <borrowed> { _ => () }	💚	💚	playground

My reasoning is this:

• I think "unsafe checks" are not "flow-sensitive" checks -- they are just based on what you do and where, not whether the code is reachable etc.
• On the other hand, initialization and borrow checkers are flow-sensitive, and hence let _ = is known not to touch the thing that is being matched against (same with match with one _ pattern).
  • This is concordant with the "most likely" way to understand exhaustiveness, I think, which is that match foo { _ => () } basically is a non-access and hence cannot be UB.

I think the way I would implement this in MIR desugaring is:

• Move unsafe checking to operate on HAIR, I guess, or else add some kind of "unsafe_check()" instruction that hangs around just so that the unsafe checker can see it.
• If you are lowering a match with one arm, then use the "irrefutable" logic for handling it (since the pattern must be irrefutable) -- this will, in turn, mean that match foo { _ => () } doesn't add anything extra
  • In particular, it doesn't add the implicit borrows or other accesses that would otherwise result

This is not entirely "backwards compatible", but accepting let _ = <unsafe field> is a regression in any case, one which was incompletely fixed. In general the move to doing unsafe checking on MIR led to a number of complications around dead code, which is why I would prefer to move away from that.

[eddyb]

@nikomatsakis so I think the packed field access checking needs to be on MIR (to have access to the final place operations), but raw pointer dereferences and union field accesses could trivially be on HAIR (and we should maybe also move some linting from HIR to HAIR).

[nikomatsakis]

so I think the packed field access checking needs to be on MIR (to have access to the final place operations)

Hmm, that's surprising to me. Maybe I'm forgetting which bits of desugaring are done on HAIR vs MIR -- I would think that the field accesses would be quite easy to spot. @eddyb can you give me an example of where MIR would be better?