Correct the subtyping relations created by the pattern typechecking

code. Previously we were creating a subtyping relation in the wrong
direction. We now just unify types, which is stronger than necessary
but turns out fine.

Fixes #19552.
Fixes #19997.

Author: nikomatsakis

src/librustc_typeck/check/_match.rs

@@ -46,6 +46,19 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
             check_expr(fcx, &**lt);
             let expr_ty = fcx.expr_ty(&**lt);
             fcx.write_ty(pat.id, expr_ty);
+
+            // somewhat surprising: in this case, the subtyping
+            // relation goes the opposite way as the other
+            // cases. Actually what we really want is not a subtyping
+            // relation at all but rather that there exists a LUB (so
+            // that they can be compared). However, in practice,
+            // constants are always scalars or strings.  For scalars
+            // subtyping is irrelevant, and for strings `expr_ty` is
+            // type is `&'static str`, so if we say that
+            //
+            //     &'static str <: expected
+            //
+            // that's equivalent to there existing a LUB.
             demand::suptype(fcx, pat.span, expected, expr_ty);
         }
         ast::PatRange(ref begin, ref end) => {
@@ -54,10 +67,16 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
 
             let lhs_ty = fcx.expr_ty(&**begin);
             let rhs_ty = fcx.expr_ty(&**end);
-            if require_same_types(
-                tcx, Some(fcx.infcx()), false, pat.span, lhs_ty, rhs_ty,
-                || "mismatched types in range".to_string())
-                && (ty::type_is_numeric(lhs_ty) || ty::type_is_char(rhs_ty)) {
+
+            let lhs_eq_rhs =
+                require_same_types(
+                    tcx, Some(fcx.infcx()), false, pat.span, lhs_ty, rhs_ty,
+                    || "mismatched types in range".to_string());
+
+            let numeric_or_char =
+                lhs_eq_rhs && (ty::type_is_numeric(lhs_ty) || ty::type_is_char(lhs_ty));
+
+            if numeric_or_char {
                 match valid_range_bounds(fcx.ccx, &**begin, &**end) {
                     Some(false) => {
                         span_err!(tcx.sess, begin.span, E0030,
@@ -75,6 +94,8 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
             }
 
             fcx.write_ty(pat.id, lhs_ty);
+
+            // subtyping doens't matter here, as the value is some kind of scalar
             demand::eqtype(fcx, pat.span, expected, lhs_ty);
         }
         ast::PatEnum(..) | ast::PatIdent(..) if pat_is_const(&tcx.def_map, pat) => {
@@ -89,20 +110,29 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
                 ast::BindByRef(mutbl) => {
                     // if the binding is like
                     //    ref x | ref const x | ref mut x
-                    // then the type of x is &M T where M is the mutability
-                    // and T is the expected type
+                    // then `x` is assigned a value of type `&M T` where M is the mutability
+                    // and T is the expected type.
                     let region_var = fcx.infcx().next_region_var(infer::PatternRegion(pat.span));
                     let mt = ty::mt { ty: expected, mutbl: mutbl };
                     let region_ty = ty::mk_rptr(tcx, tcx.mk_region(region_var), mt);
+
+                    // `x` is assigned a value of type `&M T`, hence `&M T <: typeof(x)` is
+                    // required. However, we use equality, which is stronger. See (*) for
+                    // an explanation.
                     demand::eqtype(fcx, pat.span, region_ty, typ);
                 }
                 // otherwise the type of x is the expected type T
                 ast::BindByValue(_) => {
+                    // As above, `T <: typeof(x)` is required but we
+                    // use equality, see (*) below.
                     demand::eqtype(fcx, pat.span, expected, typ);
                 }
             }
+
             fcx.write_ty(pat.id, typ);
 
+            // if there are multiple arms, make sure they all agree on
+            // what the type of the binding `x` ought to be
             let canon_id = pcx.map[path.node];
             if canon_id != pat.id {
                 let ct = fcx.local_ty(pat.span, canon_id);
@@ -138,7 +168,10 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
             let uniq_ty = ty::mk_uniq(tcx, inner_ty);
 
             if check_dereferencable(pcx, pat.span, expected, &**inner) {
-                demand::suptype(fcx, pat.span, expected, uniq_ty);
+                // Here, `demand::subtype` is good enough, but I don't
+                // think any errors can be introduced by using
+                // `demand::eqtype`.
+                demand::eqtype(fcx, pat.span, expected, uniq_ty);
                 fcx.write_ty(pat.id, uniq_ty);
                 check_pat(pcx, &**inner, inner_ty);
             } else {
@@ -158,7 +191,10 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
             let rptr_ty = ty::mk_rptr(tcx, tcx.mk_region(region), mt);
 
             if check_dereferencable(pcx, pat.span, expected, &**inner) {
-                demand::suptype(fcx, pat.span, expected, rptr_ty);
+                // `demand::subtype` would be good enough, but using
+                // `eqtype` turns out to be equally general. See (*)
+                // below for details.
+                demand::eqtype(fcx, pat.span, expected, rptr_ty);
                 fcx.write_ty(pat.id, rptr_ty);
                 check_pat(pcx, &**inner, inner_ty);
             } else {
@@ -188,7 +224,11 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
             };
 
             fcx.write_ty(pat.id, pat_ty);
-            demand::suptype(fcx, pat.span, expected, pat_ty);
+
+            // `demand::subtype` would be good enough, but using
+            // `eqtype` turns out to be equally general. See (*)
+            // below for details.
+            demand::eqtype(fcx, pat.span, expected, pat_ty);
 
             for elt in before.iter() {
                 check_pat(pcx, &**elt, inner_ty);
@@ -210,6 +250,56 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
         }
         ast::PatMac(_) => tcx.sess.bug("unexpanded macro")
     }
+
+
+    // (*) 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. Most importantly, there is no loss of expressiveness
+    // here. What we are saying is that the type of `x`
+    // becomes *exactly* what is expected. This might seem
+    // like 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.
 }
 
 pub fn check_dereferencable<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,

src/test/compile-fail/regions-pattern-typing-issue-19552.rs

@@ -0,0 +1,18 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn assert_send<T: Send>(_t: T) {}
+
+fn main() {
+    let line = String::new();
+    match [line.as_slice()] { //~ ERROR `line` does not live long enough
+        [ word ] => { assert_send(word); }
+    }
+}

src/test/compile-fail/regions-pattern-typing-issue-19997.rs

@@ -0,0 +1,20 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn main() {
+    let a0 = 0u8;
+    let f = 1u8;
+    let mut a1 = &a0;
+    match (&a1,) {
+        (&ref b0,) => {
+            a1 = &f; //~ ERROR cannot assign
+        }
+    }
+}

src/test/run-pass/regions-reassign-let-bound-pointer.rs

@@ -0,0 +1,23 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Check that the type checker permits us to reassign `z` which
+// started out with a longer lifetime and was reassigned to a shorter
+// one (it should infer to be the intersection).
+
+fn foo(x: &int) {
+    let a = 1;
+    let mut z = x;
+    z = &a;
+}
+
+pub fn main() {
+    foo(&1);
+}

src/test/run-pass/regions-reassign-match-bound-pointer.rs

@@ -0,0 +1,26 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Check that the type checker permits us to reassign `z` which
+// started out with a longer lifetime and was reassigned to a shorter
+// one (it should infer to be the intersection).
+
+fn foo(x: &int) {
+    let a = 1;
+    match x {
+        mut z => {
+            z = &a;
+        }
+    }
+}
+
+pub fn main() {
+    foo(&1);
+}

src/test/run-pass/regions-relate-bound-regions-on-closures-to-inference-variables.rs

@@ -0,0 +1,65 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Test that this fairly specialized, but also reasonable, pattern
+// typechecks. The pattern involves regions bound in closures that
+// wind up related to inference variables.
+//
+// NB. Changes to the region implementatiosn have broken this pattern
+// a few times, but it happens to be used in the compiler so those
+// changes were caught. However, those uses in the compiler could
+// easily get changed or refactored away in the future.
+
+struct Ctxt<'tcx> {
+    x: &'tcx Vec<int>
+}
+
+struct Foo<'a,'tcx:'a> {
+    cx: &'a Ctxt<'tcx>,
+}
+
+impl<'a,'tcx> Foo<'a,'tcx> {
+    fn bother(&mut self) -> int {
+        self.elaborate_bounds(|this| {
+            // (*) Here: type of `this` is `&'f0 Foo<&'f1, '_2>`,
+            // where `'f0` and `'f1` are fresh, free regions that
+            // result from the bound regions on the closure, and `'2`
+            // is a region inference variable created by the call. Due
+            // to the constraints on the type, we find that `'_2 : 'f1
+            // + 'f2` must hold (and can be assumed by the callee).
+            // Region inference has to do some clever stuff to avoid
+            // inferring `'_2` to be `'static` in this case, because
+            // it is created outside the closure but then related to
+            // regions bound by the closure itself. See the
+            // `region_inference.rs` file (and the `givens` field, in
+            // particular) for more details.
+            this.foo()
+        })
+    }
+
+    fn foo(&mut self) -> int {
+        22
+    }
+
+    fn elaborate_bounds(
+        &mut self,
+        mk_cand: for<'b>|this: &mut Foo<'b, 'tcx>| -> int)
+        -> int
+    {
+        mk_cand(self)
+    }
+}
+
+fn main() {
+    let v = vec!();
+    let cx = Ctxt { x: &v };
+    let mut foo = Foo { cx: &cx };
+    assert_eq!(foo.bother(), 22); // just so the code is not dead, basically
+}