From 9b820d0d634483e5400b1da52340a0606a32c84a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 4 Jan 2015 05:22:59 -0500 Subject: [PATCH 1/3] 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. --- src/librustc_typeck/check/_match.rs | 108 ++++++++++++++++-- .../regions-pattern-typing-issue-19552.rs | 18 +++ .../regions-pattern-typing-issue-19997.rs | 20 ++++ .../regions-reassign-let-bound-pointer.rs | 23 ++++ .../regions-reassign-match-bound-pointer.rs | 26 +++++ ...ions-on-closures-to-inference-variables.rs | 65 +++++++++++ 6 files changed, 251 insertions(+), 9 deletions(-) create mode 100644 src/test/compile-fail/regions-pattern-typing-issue-19552.rs create mode 100644 src/test/compile-fail/regions-pattern-typing-issue-19997.rs create mode 100644 src/test/run-pass/regions-reassign-let-bound-pointer.rs create mode 100644 src/test/run-pass/regions-reassign-match-bound-pointer.rs create mode 100644 src/test/run-pass/regions-relate-bound-regions-on-closures-to-inference-variables.rs diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index 7c431b4fc0bd0..d0fd5732f233d 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/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>, diff --git a/src/test/compile-fail/regions-pattern-typing-issue-19552.rs b/src/test/compile-fail/regions-pattern-typing-issue-19552.rs new file mode 100644 index 0000000000000..3f722c9433bb2 --- /dev/null +++ b/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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn assert_send(_t: T) {} + +fn main() { + let line = String::new(); + match [line.as_slice()] { //~ ERROR `line` does not live long enough + [ word ] => { assert_send(word); } + } +} diff --git a/src/test/compile-fail/regions-pattern-typing-issue-19997.rs b/src/test/compile-fail/regions-pattern-typing-issue-19997.rs new file mode 100644 index 0000000000000..da839d7217261 --- /dev/null +++ b/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 or the MIT license +// , 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 + } + } +} diff --git a/src/test/run-pass/regions-reassign-let-bound-pointer.rs b/src/test/run-pass/regions-reassign-let-bound-pointer.rs new file mode 100644 index 0000000000000..ecf79de622222 --- /dev/null +++ b/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 or the MIT license +// , 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); +} diff --git a/src/test/run-pass/regions-reassign-match-bound-pointer.rs b/src/test/run-pass/regions-reassign-match-bound-pointer.rs new file mode 100644 index 0000000000000..18312b17339ce --- /dev/null +++ b/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 or the MIT license +// , 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); +} diff --git a/src/test/run-pass/regions-relate-bound-regions-on-closures-to-inference-variables.rs b/src/test/run-pass/regions-relate-bound-regions-on-closures-to-inference-variables.rs new file mode 100644 index 0000000000000..aa0ed023da3e2 --- /dev/null +++ b/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 or the MIT license +// , 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 +} + +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 +} From a17a7c9f7572685df59e287c48450ccb09e8313a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 4 Jan 2015 05:23:29 -0500 Subject: [PATCH 2/3] Miscellaneous reformatting and commenting. --- src/librustc_typeck/check/_match.rs | 19 +++++++++++-------- src/librustc_typeck/check/demand.rs | 13 ++++++------- src/librustc_typeck/check/mod.rs | 2 +- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index d0fd5732f233d..7ff0e1758f526 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -30,7 +30,9 @@ use syntax::print::pprust; use syntax::ptr::P; pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, - pat: &ast::Pat, expected: Ty<'tcx>) { + pat: &ast::Pat, + expected: Ty<'tcx>) +{ let fcx = pcx.fcx; let tcx = pcx.fcx.ccx.tcx; @@ -90,7 +92,7 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, } } else { span_err!(tcx.sess, begin.span, E0029, - "only char and numeric types are allowed in range"); + "only char and numeric types are allowed in range"); } fcx.write_ty(pat.id, lhs_ty); @@ -154,8 +156,9 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, check_pat_struct(pcx, pat, path, fields.as_slice(), etc, expected); } ast::PatTup(ref elements) => { - let element_tys: Vec<_> = range(0, elements.len()).map(|_| fcx.infcx() - .next_ty_var()).collect(); + let element_tys: Vec<_> = + range(0, elements.len()).map(|_| fcx.infcx().next_ty_var()) + .collect(); let pat_ty = ty::mk_tup(tcx, element_tys.clone()); fcx.write_ty(pat.id, pat_ty); demand::eqtype(fcx, pat.span, expected, pat_ty); @@ -183,8 +186,8 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, let inner_ty = fcx.infcx().next_ty_var(); let mutbl = - ty::deref(fcx.infcx().shallow_resolve(expected), true) - .map_or(ast::MutImmutable, |mt| mt.mutbl); + ty::deref(fcx.infcx().shallow_resolve(expected), true).map(|mt| mt.mutbl) + .unwrap_or(ast::MutImmutable); let mt = ty::mt { ty: inner_ty, mutbl: mutbl }; let region = fcx.infcx().next_region_var(infer::PatternRegion(pat.span)); @@ -217,8 +220,8 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, let region = fcx.infcx().next_region_var(infer::PatternRegion(pat.span)); ty::mk_slice(tcx, tcx.mk_region(region), ty::mt { ty: inner_ty, - mutbl: ty::deref(expected_ty, true) - .map_or(ast::MutImmutable, |mt| mt.mutbl) + mutbl: ty::deref(expected_ty, true).map(|mt| mt.mutbl) + .unwrap_or(ast::MutImmutable) }) } }; diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 9af9eaf75f5a5..565964012085a 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -18,14 +18,15 @@ use syntax::ast; use syntax::codemap::Span; use util::ppaux::Repr; -// Requires that the two types unify, and prints an error message if they -// don't. +// Requires that the two types unify, and prints an error message if +// they don't. pub fn suptype<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, sp: Span, - expected: Ty<'tcx>, actual: Ty<'tcx>) { - suptype_with_fn(fcx, sp, false, expected, actual, + ty_expected: Ty<'tcx>, ty_actual: Ty<'tcx>) { + suptype_with_fn(fcx, sp, false, ty_expected, ty_actual, |sp, e, a, s| { fcx.report_mismatched_types(sp, e, a, s) }) } +/// As `suptype`, but call `handle_err` if unification for subtyping fails. pub fn suptype_with_fn<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>, sp: Span, b_is_expected: bool, @@ -48,9 +49,7 @@ pub fn eqtype<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) { match infer::mk_eqty(fcx.infcx(), false, infer::Misc(sp), actual, expected) { Ok(()) => { /* ok */ } - Err(ref err) => { - fcx.report_mismatched_types(sp, expected, actual, err); - } + Err(ref err) => { fcx.report_mismatched_types(sp, expected, actual, err); } } } diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 82525b71052d2..0a7c406e63172 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -4683,7 +4683,7 @@ impl<'tcx> Repr<'tcx> for Expectation<'tcx> { pub fn check_decl_initializer(fcx: &FnCtxt, nid: ast::NodeId, init: &ast::Expr) - { +{ let local_ty = fcx.local_ty(init.span, nid); check_expr_coercable_to_type(fcx, init, local_ty) } From dbfa05411bad5d31cb594a02ddbf5333dcb0ad5a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 4 Jan 2015 07:23:21 -0500 Subject: [PATCH 3/3] Cleanup type-checking of constants, but do not try to fix #20489. --- src/librustc/middle/ty.rs | 4 ++++ src/librustc_typeck/check/_match.rs | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index c720032bef264..0d853295502dc 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -1779,6 +1779,10 @@ impl<'tcx> Generics<'tcx> { !self.regions.is_empty_in(space) } + pub fn is_empty(&self) -> bool { + self.types.is_empty() && self.regions.is_empty() + } + pub fn to_bounds(&self, tcx: &ty::ctxt<'tcx>, substs: &Substs<'tcx>) -> GenericBounds<'tcx> { GenericBounds { diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index 7ff0e1758f526..49627120d2291 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -103,8 +103,18 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>, ast::PatEnum(..) | ast::PatIdent(..) if pat_is_const(&tcx.def_map, pat) => { let const_did = tcx.def_map.borrow()[pat.id].clone().def_id(); let const_scheme = ty::lookup_item_type(tcx, const_did); - fcx.write_ty(pat.id, const_scheme.ty); - demand::suptype(fcx, pat.span, expected, const_scheme.ty); + assert!(const_scheme.generics.is_empty()); + let const_ty = pcx.fcx.instantiate_type_scheme(pat.span, + &Substs::empty(), + &const_scheme.ty); + fcx.write_ty(pat.id, const_ty); + + // FIXME(#20489) -- we should limit the types here to scalars or something! + + // As with PatLit, what we really want here is that there + // exist a LUB, but for the cases that can occur, subtype + // is good enough. + demand::suptype(fcx, pat.span, expected, const_ty); } ast::PatIdent(bm, ref path, ref sub) if pat_is_binding(&tcx.def_map, pat) => { let typ = fcx.local_ty(pat.span, pat.id);