implement pattern-binding-modes RFC

[tbg]

See the RFC and tracking issue #42640

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[tbg]

@nikomatsakis I updated the mem categorization code. Maybe I missed something, but your example borrowcks now (see match-defbm-ref-region.rs).

[bors]

☔️ The latest upstream changes (presumably #44678) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

@tschottdorf sorry for being slow. Start of the impl period has gotten me pretty slammed!

[tbg]

No worries, if you're getting anything in before Friday that'll be enough :-)

[nikomatsakis]

I left a few nits and suggestions. It seems like the major unknown is how to deal with the "patterns contain binding" test. I know that @arielb1 and I had some discussion about this at some point that I'd like to re-read. =)

[nikomatsakis]

I left a few nits and suggestions. It seems like the major unknown is how to deal with the "patterns contain binding" test. I know that @arielb1 and I had some discussion about this at some point that I'd like to re-read. =)

[nikomatsakis]

I did point you to do the adjustment after, but I think I was wrong. I think we should move this callback to occur after. At first I thought we would do the callback both before and after each adjustment, but since we supply both the pattern and the cmt, I think it's better to just do a callback after the adjustments. We are therefore giving (a) the pattern that is being used and (b) the cmt that is being matched by this pattern. Looking at the use-sites of this function, all of them expect this and will work properly.

[tbg]

Ok, will give that a try. Do you have an example or another way of testing that would break the current code and works with the callback invoked at the right place?

[nikomatsakis]

mm let me think about it. There may be such a thing.

[nikomatsakis]

I cannot find a way that it makes any difference -- all the consumers of this except for an obscure one in clippy only care about binding patterns, and those are never adjusted. However, it still seems more correct. Right now, we are passing in the bare pattern paired with the cmt that corresponds to (logically) the adjusted pattern. This is also coherent but it seems a bit .. less expected. i.e., if I match on the pattern and get (e.g.) PatTuple, I would expect the cmt to have tuple type (but in this code, it may have reference type).

[tbg]

I just want to double check your comment above because the code that you commented on gives the cmt for the bare pattern and the bare pattern, whereas moving the callback gives the adjusted cmt and (still) the bare pattern. Your wording makes it sound like you'd actually prefer option 1, which is the current code (right?).

[nikomatsakis]

So, what I meant is this:

• Consider first match &Some(3) { &Some(ref x) => .
  • When cat_pattern is first invoked, we have the cmt C for the discriminant (&Some(3)) and the pattern &Some(ref x), and we invoke the callback with that
  • We then process the pattern, yielding the CMT for *&Some(3) and the pattern Some(ref x), and we invoke the callback with that.
  • Finally, we process the CMT one step further to Downcast<Some>(*&Some(3)).0 and invoke callback with with the pattern ref x.
• Under the code as it is there now:
  • When cat_pattern is first invoked, we still have the same cmt C for the discriminant (&Some(3)), but the pattern is for Some(x). We invoke the callback with that.
  • Then we adjust the CMT to *&Some(3). No callback occurs after this.
  • Then we process the pattern, downcasting to Downcast<Some>(*&Some(3)).0, and invoke callback with pattern x, where the ref is implied.

These don't quite match -- there aren't the same number of callbacks, to start, but also the cmt and patterns don't match up. If we move the callback to after the adjustment, we will get:

• Under the code as proposed:
  • When cat_pattern is first invoked, we still have the same cmt C for the discriminant (&Some(3)), but the pattern is for Some(x).
  • Then we adjust the CMT to *&Some(3).
  • Now we callback with CMT *&Some(3) and pattern Some(x), just as before.
  • Then we process the pattern, downcasting to Downcast<Some>(*&Some(3)).0, and invoke callback with pattern x, where the ref is implied.

[tbg]

Done.

[nikomatsakis]

Make this private.

[tbg]

Done.

[nikomatsakis]

Nit: I'd prefer the arguments to each be on their own line.

[tbg]

Done.

[nikomatsakis]

OK, we gotta figure this out.

[nikomatsakis]

I forget, did you have examples?

[tbg]

Among possibly other things, we treat string literals incorrectly.

match s {
            "inf" => T::INFINITY,
            "NaN" => T::NAN,
            _ => { return Err(pfe_invalid()); }
        }

   Compiling core v0.0.0 (file:///Users/tschottdorf/rust/binding-modes/src/libcore)
error[E0308]: mismatched types
   --> src/libcore/num/dec2flt/mod.rs:219:13
    |
219 |             "inf" => T::INFINITY,
    |             ^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/num/dec2flt/mod.rs:220:13
    |
220 |             "NaN" => T::NAN,
    |             ^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/str/mod.rs:130:13
    |
130 |             "true"  => Ok(true),
    |             ^^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/str/mod.rs:131:13
    |
131 |             "false" => Ok(false),
    |             ^^^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error: aborting due to 4 previous errors

error: Could not compile `core`.

[nikomatsakis]

Yeah it seems like string and byte literals (and maybe slices?) are the other places this would go wrong. But I guess more generally one can have a constant with references in it (e.g., const X: &'static u32 = &22 -- I forget if you can use such a constant in a match though.

[tbg]

I added a computation of the type and make the true/false depend on whether there's a TyRef. May not be the right thing to do, but at least it works somewhat.

[nikomatsakis]

Hmm. It's not great to call this here, in that it can report an error, which might then get reported again later on (by check_pat_path). Not sure how best to fix this though!

[nikomatsakis]

Hmm, we might be able to just modify resolve_ty_and_def_ufcs to cache the result. It already writes the final result into a table:

// Write back the new resolution.
let hir_id = self.tcx.hir.node_to_hir_id(node_id);
self.tables.borrow_mut().type_dependent_defs_mut().insert(hir_id, def);

seems like we can check that cache in the case that we get TypeRelative.

(cc @eddyb -- this seem ok to you?)

[nikomatsakis]

So I've been advocating for us to organize the tests for new language features like this by putting them into a directory. So can we move this to ths directory:

src/test/compile-fail/rfc-XXX-default-binding-mode/

The idea is that we should be able to browse this directory (as well as corresponding directories in src/test/run-pass and src/test/ui) to get a good idea of how the feature behaves.

[tbg]

Done.

[nikomatsakis]

move this file to a directory, as described before

[tbg]

Done.

[nikomatsakis]

directory

[tbg]

Done.

[nikomatsakis]

directory

[tbg]

Done.

[nikomatsakis]

directory

[tbg]

Done.

[nikomatsakis]

@tschottdorf

I think that it might be considered unfortunate that
typeck is now unaware of the fact that autoderefs were inserted, and so the error messages may
refer to types that weren't written down.

Was there some particular test that caught your eye? Most of them actually looked like improvements to me.

[tbg]

Thanks for the review @nikomatsakis! Haven't changed anything (will do tomorrow), just posted a few responses to comments to get you unstuck.

[tbg]

Thanks for the review @nikomatsakis! Haven't changed anything (will do tomorrow), just posted a few responses to comments to get you unstuck.

[tbg]

Ok, will give that a try. Do you have an example or another way of testing that would break the current code and works with the callback invoked at the right place?

[tbg]

#43399 (comment)

[tbg]

Among possibly other things, we treat string literals incorrectly.

match s {
            "inf" => T::INFINITY,
            "NaN" => T::NAN,
            _ => { return Err(pfe_invalid()); }
        }

   Compiling core v0.0.0 (file:///Users/tschottdorf/rust/binding-modes/src/libcore)
error[E0308]: mismatched types
   --> src/libcore/num/dec2flt/mod.rs:219:13
    |
219 |             "inf" => T::INFINITY,
    |             ^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/num/dec2flt/mod.rs:220:13
    |
220 |             "NaN" => T::NAN,
    |             ^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/str/mod.rs:130:13
    |
130 |             "true"  => Ok(true),
    |             ^^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error[E0308]: mismatched types
   --> src/libcore/str/mod.rs:131:13
    |
131 |             "false" => Ok(false),
    |             ^^^^^^^ expected str, found reference
    |
    = note: expected type `str`
               found type `&'static str`

error: aborting due to 4 previous errors

error: Could not compile `core`.

[nikomatsakis]

OK, so, I was thinking some more about the problem of how to replace the code that scans (syntactically) for ref bindings. I think that we can keep using the syntactical check and things will be ok, but I'm not sure I want to. Let me lay out the reasoning in full. @arielb1 I'd appreciate your thoughts on this! This is rather complicated.

Let's focus on the case of a let binding with an explicit type, as it is the most complicated:

let P: T = E

What makes this complicated is that we sometimes need to coerce the result of evaluating E to the type T (i.e., subtyping won't do, because runtime adjustments are needed). But that combines oddly with ref bindings, since coercion will produce a new temporary value, and we are then "borrowing" that new value. But when you write (for example) something with an lvalue initializer:

let ref mut x = something

this is generally equivalent to let x = &mut something. That is, we are borrowing something.

So what we do TODAY is to say this, effectively:

• If there are (syntactic) ref-bindings in the pattern P, then E is treated "as an lvalue". That is, we will not permit coercions, the patterns will match against E "in place". If E is a local variable, field, etc, then you will get references into that storage.
• If there are NOT (syntactic) ref-bindings in the pattern P, then E is treated "as an rvalue". That is, we permit coercions, and we decompose the result. This break-down works fine today because not having syntactic ref-bindings means there are no ref-bindings at all: so it's fine that we coerce into a fresh value, as we are taking ownership of various bits of the coerced result, not borrowing them.

However, in the new system, we now have to account for the possibility that there will be "type-based" ref-bindings in the pattern P that are not syntactically evident. However, we know that if such ref-bindings exist, they occur "beneath" a & or &mut reference. That is, there may be borrows we cannot see, but they will only be borrows of the referents of a reference, not the reference itself.

I think this means we don't have to worry about the big danger that we were afraid of. In particular, no coercion ever modifies the referent of a reference -- we don't know where that memory lives, and we don't know that we have the freedom to change it. In other words, if we coerce the result of E, it will only affect the owned content of that result, not its borrowed content -- and for their to be borrows of the owned content, they must be done with a ref binding.

(There is one exception of sorts: we do permit &'a mut (Trait + 'b) to be coerced to &'a mut (Trait + 'c) where 'b: 'c. This would ordinarily be unsound, but it's ok because we don't permit reassignment of unsized types. It's not clear that such a coercion matters here anyway, since there is no way to write a pattern that matches the "trait" part without the &mut. But even if it were possible, the resulting binding would not permit mutation of its referent.)

The other cases where we use this check, I think, can never perform coercion, and so are not as problematic. They only have to worry about subtyping, and in that case the same logic above applies -- we will only ever take a ref mut binding to the referent an &mut, which is invariant anyhow:

• match -- no coercion is done
• let P = E -- the "expected type" is a fresh inference variable, so no coercion performed

[arielb1]

@nikomatsakis

Now that I'm reading this again:
In HAIR/MIR, the soundness of the situation is quite simple: coercions and subtyping occur only on vexprs. Subtyping can operate on references within a vexpr, but it's still at heart a vexpr operation. Match expressions do their subtyping when they bind the (potentially references) to the fields of the scrutinee into the bindings.

If we just wanted to be consistent with that, then the #23116 example:

#![allow(dead_code)]
use std::fmt::Debug;
struct S(Box<Debug + 'static>);
impl S {
    fn bar<'a>(&'a mut self)->&'a mut Box<Debug + 'a> {
        match self.0 { ref mut x => x } // should not compile, but does
    }
}
fn main() {}

Would perform a lexpr->vexpr->lexpr conversion, and therefore a temporary, creating MIR as follows:

temp = self.0; // lexpr -> vexpr
scrutinee_temp = &mut temp; // vexpr -> lexpr + coercion
x = &mut (*scrutinee_temp); // match binding
ret_ptr = x; // return value assignment
return

Which would of course cause a "borrow does not live long enough" when it catches you trying to return a borrow of the local temp.

Obviously, creating such coercion temporaries by default will annoy anyone who tries to use ref patterns to actually match parts of the scrutinee value by reference, so we don't. If we see a ref pattern, we prevent coercions and the resulting round-trips.

However, if all the ref bindings occur behind a reference, creating the temporary can't actually be annoying in this way - you don't take references to the value, so the temporary is invisible and we don't need to avoid it. Therefore, avoiding coercions exactly when there are explicit ref patterns is a implementable and non-annoying strategy.

An aside

The above description is not actually implementation-coherent with typeck - to satisfy closure inference, match bindings can't use subtyping, so rustc actually sometimes performs subtyping on immutable lexprs - see this comment:

rust/src/librustc_typeck/check/_match.rs

Lines 331 to 379 in dcb4378

	// (*) In most of the cases above (literals and constants being
	// the exception), we relate types using strict equality, evewn
	// though subtyping would be sufficient. There are a few reasons
	// for this, some of which are fairly subtle and which cost me
	// (nmatsakis) an hour or two debugging to remember, so I thought
	// I'd write them down this time.
	//
	// 1. There is no loss of expressiveness here, though it does
	// cause some inconvenience. What we are saying is that the type
	// of `x` becomes *exactly* what is expected. This can cause unnecessary
	// errors in some cases, such as this one:
	// it will cause errors in a case like this:
	//
	// ```
	// fn foo<'x>(x: &'x int) {
	// let a = 1;
	// let mut z = x;
	// z = &a;
	// }
	// ```
	//
	// The reason we might get an error is that `z` might be
	// assigned a type like `&'x int`, and then we would have
	// a problem when we try to assign `&a` to `z`, because
	// the lifetime of `&a` (i.e., the enclosing block) is
	// shorter than `'x`.
	//
	// HOWEVER, this code works fine. The reason is that the
	// expected type here is whatever type the user wrote, not
	// the initializer's type. In this case the user wrote
	// nothing, so we are going to create a type variable `Z`.
	// Then we will assign the type of the initializer (`&'x
	// int`) as a subtype of `Z`: `&'x int <: Z`. And hence we
	// will instantiate `Z` as a type `&'0 int` where `'0` is
	// a fresh region variable, with the constraint that `'x :
	// '0`. So basically we're all set.
	//
	// Note that there are two tests to check that this remains true
	// (`regions-reassign-{match,let}-bound-pointer.rs`).
	//
	// 2. Things go horribly wrong if we use subtype. The reason for
	// THIS is a fairly subtle case involving bound regions. See the
	// `givens` field in `region_inference`, as well as the test
	// `regions-relate-bound-regions-on-closures-to-inference-variables.rs`,
	// for details. Short version is that we must sometimes detect
	// relationships between specific region variables and regions
	// bound in a closure signature, and that detection gets thrown
	// off when we substitute fresh region variables here to enable
	// subtyping.

Note that typeck's strategy is imperfect and leads to spurious errors in some situations

fn foo<'x>(mut x: (&'x isize, ())) {
    let a = 1;
    let (mut _z, ref _y) = x;
    _z = &a; //~ ERROR no subtyping for you!
}
      
fn main() {}

We'll also have to solve the closure inference wonkyness when we get to MIR-based regionck, but hopefully the MIR-based regionck won't have wonky anyway.

[arielb1]

While this argument works today, it will put us in a somewhat sticky situation when we get DerefPure:

struct Newtype<T>(T);

impl<T> Deref for Newtype<T> {
    type Target = T;
    fn deref(&self) -> &T { &self.0 }
}
impl<T> DerefMut for Newtype<T> {
    fn deref_mut(&mut self) -> &mut T { &mut self.0 }
}
unsafe impl<T> DerefPure for Newtype<T> { /* ... */ }

fn main() {
    let n = Newtype(("hello",));
    {
        let s = "hi there".to_string();
        let (mut d,) = n;
        *d = &s; // create a temporary? fail? both options are unsatisfying
    }
}

[nikomatsakis]

My take is that we can land this without fully resolving the "coercion chicken" question. I've filed #44848 to track that.

[bors]

☔️ The latest upstream changes (presumably #44735) made this pull request unmergeable. Please resolve the merge conflicts.

[xfix]

Shouldn't default variant panic too, considering it's unexpected result value?

[tbg]

Done.

[nikomatsakis]

@tschottdorf I wrote up a list of test cases here. Can you just check that you have tests covering those cases?

[tbg]

@nikomatsakis confirmed, though one of your test cases doesn't compile (out of scope of this PR, see #42640 (comment)). I added a compile-fail test that documents this but you probably want to file an issue unless there's already one.

Could you review the FIXMEs (just search for FIXME(tschottdorf) in the full diff and leave a comment on each, the comment could just be "leave as is for now"). I do want to address them, though if you're desperate about merging this, I could do it in a follow-up.

[nikomatsakis]

Leave as is, link to #44848.

[tbg]

Done.

[nikomatsakis]

Leave as is, link to #44848.

[tbg]

Done.

[nikomatsakis]

Change as discussed below (move callback later).

[tbg]

Done.

[nikomatsakis]

It is correct. If you make it false, I think it would affect how the error in this example is printed.

[tbg]

Ack, done.

[nikomatsakis]

I think we should test match &s { too.

[nikomatsakis]

(Though I think this would maybe give an error?)

[tbg]

Yeah, this gives an error. What would you like me to do? Move to ui tests & file an issue? Try to fix?

    let s: &'static str = "abc";

    match &s {
            "abc" => true,
            _ => panic!(),
    };

[nikomatsakis]

Booleans? Not much else really.

[nikomatsakis]

Do we have a test with named constants of struct type etc?

[tbg]

Like this?

#[derive(PartialEq, Eq)]
struct Foo {
    bar: i32,    
}

const FOO: Foo = Foo{bar: 5};

fn main() {
    let f = Foo{bar:6};
    
    match &f {
        FOO => {},
        _ => panic!(),
    }
}

Doesn't compile (but should), added to ui tests for now.

[nikomatsakis]

@tschottdorf

Could you review the FIXMEs

Done

[tbg]

Thanks! Mostly done except for avoiding double type errors. Plus there may be some non-compiling tests that you'll want to advise me on/open follow-up issues for (all have comments in this review).

[tbg]

Thanks! Mostly done except for avoiding double type errors. Plus there may be some non-compiling tests that you'll want to advise me on/open follow-up issues for (all have comments in this review).

[tbg]

Done.

[tbg]

Done.

[tbg]

Done.

[tbg]

Done.

[tbg]

Ack, done.

[tbg]

Filed #44912.

[tbg]

Like this?

#[derive(PartialEq, Eq)]
struct Foo {
    bar: i32,    
}

const FOO: Foo = Foo{bar: 5};

fn main() {
    let f = Foo{bar:6};
    
    match &f {
        FOO => {},
        _ => panic!(),
    }
}

Doesn't compile (but should), added to ui tests for now.

[tbg]

Yeah, this gives an error. What would you like me to do? Move to ui tests & file an issue? Try to fix?

    let s: &'static str = "abc";

    match &s {
            "abc" => true,
            _ => panic!(),
    };

[tbg]

Yep. I'll try to do that tomorrow.

[tbg]

I'll take a look at this tomorrow.

[tbg]

Added a feature gate and docs in the unstable book. Mod bikeshedding on the feature gate name and avoiding doubly reporting errors I think I'm done here. Anything else missing?

[tbg]

@nikomatsakis, re #44614 (comment):

what's an example that would return an error twice? I'll need that to test that I fixed something, and wasn't able to put one together myself.

I did add caching in resolve_ty_and_def_ufcs, see if that's what you had in mind.

[tbg]

Re: #44614 (comment) (same problem for Lit), assume I wanted to pre-typecheck all the Lit instances, how would I do that? Add a separate walk that populates a map, and then pass that map into check_pat_arg and look into the map for each Lit? Would also like an example that fires a double error, wasn't able to put one together.

[nikomatsakis]

Regarding the double errors: This is an example of a test-case that gives double errors without your fix, but no longer does.

fn main() {
    let foo = 22;
    match foo {
        u32::XXX => { }
        _ => { }
    }
}

[nikomatsakis]

@tschottdorf

Add a separate walk that populates a map, and then pass that map into check_pat_arg and look into the map for each Lit?

I imagine something like this, yes. Basically a pre-pass over all the patterns that finds literals and type-checks them -- this would probably happen when check_pat is first invoked. Then during the rest of the code that walks patterns, when we encounter a pattern literal, we would not invoke

 let ty = self.check_expr(&lt);

but rather

let ty = self.node_ty(lt.hir_id);

which will read out the (previously computed) type .

In other words, you don't need to add a new map, we already have the tables.

In terms of a test that would go awry... hmm... that may be a bit harder. =) I am trying to think if it's possible to get an error while type-checking a literal! Maybe it's not.

[tbg]

Squashed and ready for more review and merge.

@nikomatsakis double-error worked fine. As for the literals, I realized I didn't have to add an extra walk -- we were always calling check_expr twice in the same order, so now I just use node_ty for the second callsite.

[nikomatsakis]

@bors r+

[bors]

📌 Commit de55b4f has been approved by nikomatsakis

[bors]

⌛️ Testing commit de55b4f with merge e11f6d5...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e11f6d5 to master...

[csmoe]

this kind of lint seems a bit weird since n is already n.