From e7d96934c16c915d18be391836fbf0ebca6c558b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 3 May 2013 12:11:15 -0400 Subject: [PATCH] Correct mismatch between the way that pattern ids and expression ids map to types (pattern ids map to the input type, expression ids map to the output type) --- src/librustc/middle/borrowck/check_loans.rs | 2 +- .../middle/borrowck/gather_loans/lifetime.rs | 8 +- src/librustc/middle/borrowck/mod.rs | 18 +- src/librustc/middle/mem_categorization.rs | 195 +++++++++--------- src/librustc/middle/trans/_match.rs | 17 +- src/librustc/middle/trans/datum.rs | 4 +- src/librustc/middle/trans/expr.rs | 2 +- 7 files changed, 131 insertions(+), 115 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 70da9c938055..c2dc2fb22ab5 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -378,7 +378,7 @@ pub impl<'self> CheckLoanCtxt<'self> { // Dynamically check writes to `@mut` let key = root_map_key { - id: base.id, + id: guarantor.id, derefs: deref_count }; debug!("Inserting write guard at %?", key); diff --git a/src/librustc/middle/borrowck/gather_loans/lifetime.rs b/src/librustc/middle/borrowck/gather_loans/lifetime.rs index fdfb26c0d083..43fff110a7a7 100644 --- a/src/librustc/middle/borrowck/gather_loans/lifetime.rs +++ b/src/librustc/middle/borrowck/gather_loans/lifetime.rs @@ -97,7 +97,7 @@ impl GuaranteeLifetimeContext { ); if !omit_root { - self.check_root(base, derefs, ptr_mutbl, discr_scope); + self.check_root(cmt, base, derefs, ptr_mutbl, discr_scope); } else { debug!("omitting root, base=%s, base_scope=%?", base.repr(self.tcx()), base_scope); @@ -168,12 +168,14 @@ impl GuaranteeLifetimeContext { } fn check_root(&self, + cmt_deref: mc::cmt, cmt_base: mc::cmt, derefs: uint, ptr_mutbl: ast::mutability, discr_scope: Option) { - debug!("check_root(cmt_base=%s, derefs=%? ptr_mutbl=%?, \ + debug!("check_root(cmt_deref=%s, cmt_base=%s, derefs=%?, ptr_mutbl=%?, \ discr_scope=%?)", + cmt_deref.repr(self.tcx()), cmt_base.repr(self.tcx()), derefs, ptr_mutbl, @@ -234,7 +236,7 @@ impl GuaranteeLifetimeContext { }; // Add a record of what is required - let rm_key = root_map_key {id: cmt_base.id, derefs: derefs}; + let rm_key = root_map_key {id: cmt_deref.id, derefs: derefs}; let root_info = RootInfo {scope: root_scope, freeze: opt_dyna}; self.bccx.root_map.insert(rm_key, root_info); diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 3f1022372372..a44f743c9ead 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -172,11 +172,19 @@ pub struct BorrowStats { pub type LoanMap = @mut HashMap; -// the keys to the root map combine the `id` of the expression with -// the number of types that it is autodereferenced. So, for example, -// if you have an expression `x.f` and x has type ~@T, we could add an -// entry {id:x, derefs:0} to refer to `x` itself, `{id:x, derefs:1}` -// to refer to the deref of the unique pointer, and so on. +// The keys to the root map combine the `id` of the deref expression +// with the number of types that it is *autodereferenced*. So, for +// example, imagine I have a variable `x: @@@T` and an expression +// `(*x).f`. This will have 3 derefs, one explicit and then two +// autoderefs. These are the relevant `root_map_key` values that could +// appear: +// +// {id:*x, derefs:0} --> roots `x` (type: @@@T, due to explicit deref) +// {id:*x, derefs:1} --> roots `*x` (type: @@T, due to autoderef #1) +// {id:*x, derefs:2} --> roots `**x` (type: @T, due to autoderef #2) +// +// Note that there is no entry with derefs:3---the type of that expression +// is T, which is not a box. #[deriving(Eq, IterBytes)] pub struct root_map_key { id: ast::node_id, diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 5921e4b0e4ca..f1c337125d70 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -92,12 +92,12 @@ pub enum ptr_kind { pub enum interior_kind { interior_tuple, // elt in a tuple interior_anon_field, // anonymous field (in e.g. - // struct Foo(int, int); + // struct Foo(int, int); interior_variant(ast::def_id), // internals to a variant of given enum interior_field(ast::ident, // name of field - ast::mutability), // declared mutability of field + ast::mutability), // declared mutability of field interior_index(ty::t, // type of vec/str/etc being deref'd - ast::mutability) // mutability of vec content + ast::mutability) // mutability of vec content } #[deriving(Eq)] @@ -108,18 +108,27 @@ pub enum MutabilityCategory { McInherited // Inherited from the fact that owner is mutable. } +// `cmt`: "Category, Mutability, and Type". +// // a complete categorization of a value indicating where it originated // and how it is located, as well as the mutability of the memory in // which the value is stored. // -// note: cmt stands for "categorized mutable type". +// *WARNING* The field `cmt.type` is NOT necessarily the same as the +// result of `node_id_to_type(cmt.id)`. This is because the `id` is +// always the `id` of the node producing the type; in an expression +// like `*x`, the type of this deref node is the deref'd type (`T`), +// but in a pattern like `@x`, the `@x` pattern is again a +// dereference, but its type is the type *before* the dereference +// (`@T`). So use `cmt.type` to find the type of the value in a consistent +// fashion. For more details, see the method `cat_pattern` #[deriving(Eq)] pub struct cmt_ { id: ast::node_id, // id of expr/pat producing this value span: span, // span of same expr/pat cat: categorization, // categorization of expr mutbl: MutabilityCategory, // mutability of expr as lvalue - ty: ty::t // type of the expr + ty: ty::t // type of the expr (*see WARNING above*) } pub type cmt = @cmt_; @@ -245,19 +254,6 @@ pub fn cat_def( return mcx.cat_def(expr_id, expr_span, expr_ty, def); } -pub fn cat_variant( - tcx: ty::ctxt, - method_map: typeck::method_map, - arg: N, - enum_did: ast::def_id, - cmt: cmt) -> cmt { - - let mcx = &mem_categorization_ctxt { - tcx: tcx, method_map: method_map - }; - return mcx.cat_variant(arg, enum_did, cmt); -} - pub trait ast_node { fn id(&self) -> ast::node_id; fn span(&self) -> span; @@ -273,16 +269,6 @@ impl ast_node for @ast::pat { fn span(&self) -> span { self.span } } -pub trait get_type_for_node { - fn ty(&self, node: N) -> ty::t; -} - -impl get_type_for_node for ty::ctxt { - fn ty(&self, node: N) -> ty::t { - ty::node_id_to_type(*self, node.id()) - } -} - pub struct mem_categorization_ctxt { tcx: ty::ctxt, method_map: typeck::method_map, @@ -336,6 +322,14 @@ pub impl MutabilityCategory { } pub impl mem_categorization_ctxt { + fn expr_ty(&self, expr: @ast::expr) -> ty::t { + ty::expr_ty(self.tcx, expr) + } + + fn pat_ty(&self, pat: @ast::pat) -> ty::t { + ty::node_id_to_type(self.tcx, pat.id) + } + fn cat_expr(&self, expr: @ast::expr) -> cmt { match self.tcx.adjustments.find(&expr.id) { None => { @@ -385,7 +379,7 @@ pub impl mem_categorization_ctxt { expr.id, pprust::expr_to_str(expr, self.tcx.sess.intr())); let tcx = self.tcx; - let expr_ty = tcx.ty(expr); + let expr_ty = self.expr_ty(expr); match expr.node { ast::expr_unary(ast::deref, e_base) => { if self.method_map.contains_key(&expr.id) { @@ -402,7 +396,8 @@ pub impl mem_categorization_ctxt { assert!(!self.method_map.contains_key(&expr.id)); let base_cmt = self.cat_expr(base); - self.cat_field(expr, base_cmt, f_name, expr.id) + self.cat_field(expr, base_cmt, f_name, + self.expr_ty(expr), expr.id) } ast::expr_index(base, _) => { @@ -554,19 +549,6 @@ pub impl mem_categorization_ctxt { } } - fn cat_variant(&self, - arg: N, - enum_did: ast::def_id, - cmt: cmt) -> cmt { - @cmt_ { - id: arg.id(), - span: arg.span(), - cat: cat_interior(cmt, interior_variant(enum_did)), - mutbl: cmt.mutbl.inherit(), - ty: self.tcx.ty(arg) - } - } - fn cat_rvalue(&self, elt: N, expr_ty: ty::t) -> cmt { @cmt_ { id:elt.id(), @@ -598,6 +580,7 @@ pub impl mem_categorization_ctxt { node: N, base_cmt: cmt, f_name: ast::ident, + f_ty: ty::t, field_id: ast::node_id) -> cmt { let f_mutbl = match field_mutbl(self.tcx, base_cmt.ty, f_name, field_id) { @@ -617,7 +600,7 @@ pub impl mem_categorization_ctxt { span: node.span(), cat: cat_interior(base_cmt, f_interior), mutbl: m, - ty: self.tcx.ty(node) + ty: f_ty } } @@ -697,8 +680,8 @@ pub impl mem_categorization_ctxt { } fn cat_index(&self, - elt: N, - base_cmt: cmt) -> cmt { + elt: N, + base_cmt: cmt) -> cmt { let mt = match ty::index(base_cmt.ty) { Some(mt) => mt, None => { @@ -756,27 +739,17 @@ pub impl mem_categorization_ctxt { } } - fn cat_tuple_elt(&self, - elt: N, - cmt: cmt) -> cmt { + fn cat_imm_interior(&self, + node: N, + base_cmt: cmt, + interior_ty: ty::t, + interior: interior_kind) -> cmt { @cmt_ { - id: elt.id(), - span: elt.span(), - cat: cat_interior(cmt, interior_tuple), - mutbl: cmt.mutbl.inherit(), - ty: self.tcx.ty(elt) - } - } - - fn cat_anon_struct_field(&self, - elt: N, - cmt: cmt) -> cmt { - @cmt_ { - id: elt.id(), - span: elt.span(), - cat: cat_interior(cmt, interior_anon_field), - mutbl: cmt.mutbl.inherit(), - ty: self.tcx.ty(elt) + id: node.id(), + span: node.span(), + cat: cat_interior(base_cmt, interior), + mutbl: base_cmt.mutbl.inherit(), + ty: interior_ty } } @@ -797,27 +770,37 @@ pub impl mem_categorization_ctxt { // we can be sure that the binding will remain valid for the // duration of the arm. // - // The correspondence between the id in the cmt and which - // pattern is being referred to is somewhat...subtle. In - // general, the id of the cmt is the id of the node that - // produces the value. For patterns, that's actually the - // *subpattern*, generally speaking. + // (*) There is subtlety concerning the correspondence between + // pattern ids and types as compared to *expression* ids and + // types. This is explained briefly. on the definition of the + // type `cmt`, so go off and read what it says there, then + // come back and I'll dive into a bit more detail here. :) OK, + // back? // - // To see what I mean about ids etc, consider: + // In general, the id of the cmt should be the node that + // "produces" the value---patterns aren't executable code + // exactly, but I consider them to "execute" when they match a + // value. So if you have something like: // // let x = @@3; // match x { // @@y { ... } // } // - // Here the cmt for `y` would be something like + // In this case, the cmt and the relevant ids would be: + // + // CMT Id Type of Id Type of cmt // // local(x)->@->@ + // ^~~~~~~^ `x` from discr @@int @@int + // ^~~~~~~~~~^ `@@y` pattern node @@int @int + // ^~~~~~~~~~~~~^ `@y` pattern node @int int // - // where the id of `local(x)` is the id of the `x` that appears - // in the match, the id of `local(x)->@` is the `@y` pattern, - // and the id of `local(x)->@->@` is the id of the `y` pattern. - + // You can see that the types of the id and the cmt are in + // sync in the first line, because that id is actually the id + // of an expression. But once we get to pattern ids, the types + // step out of sync again. So you'll see below that we always + // get the type of the *subpattern* and use that. let tcx = self.tcx; debug!("cat_pattern: id=%d pat=%s cmt=%s", @@ -839,22 +822,27 @@ pub impl mem_categorization_ctxt { match self.tcx.def_map.find(&pat.id) { Some(&ast::def_variant(enum_did, _)) => { // variant(x, y, z) - for subpats.each |subpat| { - let subcmt = self.cat_variant(*subpat, enum_did, cmt); - self.cat_pattern(subcmt, *subpat, op); + for subpats.each |&subpat| { + let subpat_ty = self.pat_ty(subpat); // see (*) + let subcmt = + self.cat_imm_interior(pat, cmt, subpat_ty, + interior_variant(enum_did)); + self.cat_pattern(subcmt, subpat, op); } } Some(&ast::def_fn(*)) | Some(&ast::def_struct(*)) => { - for subpats.each |subpat| { - let cmt_field = self.cat_anon_struct_field(*subpat, - cmt); - self.cat_pattern(cmt_field, *subpat, op); + for subpats.each |&subpat| { + let subpat_ty = self.pat_ty(subpat); // see (*) + let cmt_field = + self.cat_imm_interior(pat, cmt, subpat_ty, + interior_anon_field); + self.cat_pattern(cmt_field, subpat, op); } } Some(&ast::def_const(*)) => { - for subpats.each |subpat| { - self.cat_pattern(cmt, *subpat, op); + for subpats.each |&subpat| { + self.cat_pattern(cmt, subpat, op); } } _ => { @@ -876,39 +864,43 @@ pub impl mem_categorization_ctxt { ast::pat_struct(_, ref field_pats, _) => { // {f1: p1, ..., fN: pN} for field_pats.each |fp| { - let cmt_field = self.cat_field(fp.pat, cmt, fp.ident, pat.id); + let field_ty = self.pat_ty(fp.pat); // see (*) + let cmt_field = self.cat_field(pat, cmt, fp.ident, + field_ty, pat.id); self.cat_pattern(cmt_field, fp.pat, op); } } ast::pat_tup(ref subpats) => { // (p1, ..., pN) - for subpats.each |subpat| { - let subcmt = self.cat_tuple_elt(*subpat, cmt); - self.cat_pattern(subcmt, *subpat, op); + for subpats.each |&subpat| { + let subpat_ty = self.pat_ty(subpat); // see (*) + let subcmt = self.cat_imm_interior(pat, cmt, subpat_ty, + interior_tuple); + self.cat_pattern(subcmt, subpat, op); } } ast::pat_box(subpat) | ast::pat_uniq(subpat) | ast::pat_region(subpat) => { // @p1, ~p1 - let subcmt = self.cat_deref(subpat, cmt, 0); + let subcmt = self.cat_deref(pat, cmt, 0); self.cat_pattern(subcmt, subpat, op); } ast::pat_vec(ref before, slice, ref after) => { - for before.each |pat| { - let elt_cmt = self.cat_index(*pat, cmt); - self.cat_pattern(elt_cmt, *pat, op); + for before.each |&before_pat| { + let elt_cmt = self.cat_index(pat, cmt); + self.cat_pattern(elt_cmt, before_pat, op); } - for slice.each |slice_pat| { - let slice_ty = self.tcx.ty(*slice_pat); - let slice_cmt = self.cat_rvalue(*slice_pat, slice_ty); - self.cat_pattern(slice_cmt, *slice_pat, op); + for slice.each |&slice_pat| { + let slice_ty = self.pat_ty(slice_pat); + let slice_cmt = self.cat_rvalue(pat, slice_ty); + self.cat_pattern(slice_cmt, slice_pat, op); } - for after.each |pat| { - let elt_cmt = self.cat_index(*pat, cmt); - self.cat_pattern(elt_cmt, *pat, op); + for after.each |&after_pat| { + let elt_cmt = self.cat_index(pat, cmt); + self.cat_pattern(elt_cmt, after_pat, op); } } @@ -1145,4 +1137,3 @@ impl Repr for interior_kind { } } } - diff --git a/src/librustc/middle/trans/_match.rs b/src/librustc/middle/trans/_match.rs index be39edd2d9b7..80e34ca48142 100644 --- a/src/librustc/middle/trans/_match.rs +++ b/src/librustc/middle/trans/_match.rs @@ -947,6 +947,17 @@ pub fn collect_record_or_struct_fields(bcx: block, } } +pub fn pats_require_rooting(bcx: block, + m: &[@Match], + col: uint) + -> bool { + vec::any(m, |br| { + let pat_id = br.pats[col].id; + let key = root_map_key {id: pat_id, derefs: 0u }; + bcx.ccx().maps.root_map.contains_key(&key) + }) +} + pub fn root_pats_as_necessary(bcx: block, m: &[@Match], col: uint, @@ -1303,7 +1314,10 @@ pub fn compile_submatch(bcx: block, if pat_id == 0 { pat_id = br.pats[col].id; } } - bcx = root_pats_as_necessary(bcx, m, col, val); + // If we are not matching against an `@T`, we should not be + // required to root any values. + assert!(any_box_pat(m, col) || !pats_require_rooting(bcx, m, col)); + let rec_fields = collect_record_or_struct_fields(bcx, m, col); if rec_fields.len() > 0 { let pat_ty = node_id_type(bcx, pat_id); @@ -1364,6 +1378,7 @@ pub fn compile_submatch(bcx: block, // Unbox in case of a box field if any_box_pat(m, col) { + bcx = root_pats_as_necessary(bcx, m, col, val); let llbox = Load(bcx, val); let box_no_addrspace = non_gc_box_cast(bcx, llbox); let unboxed = diff --git a/src/librustc/middle/trans/datum.rs b/src/librustc/middle/trans/datum.rs index af7165c53a7c..095798ae2127 100644 --- a/src/librustc/middle/trans/datum.rs +++ b/src/librustc/middle/trans/datum.rs @@ -675,7 +675,7 @@ pub impl Datum { fn try_deref(&self, bcx: block, // block wherein to generate insn's span: span, // location where deref occurs - expr_id: ast::node_id, // id of expr being deref'd + expr_id: ast::node_id, // id of deref expr derefs: uint, // number of times deref'd already is_auto: bool) // if true, only deref if auto-derefable -> (Option, block) @@ -810,7 +810,7 @@ pub impl Datum { } fn deref(&self, bcx: block, - expr: @ast::expr, // the expression whose value is being deref'd + expr: @ast::expr, // the deref expression derefs: uint) -> DatumBlock { match self.try_deref(bcx, expr.span, expr.id, derefs, false) { diff --git a/src/librustc/middle/trans/expr.rs b/src/librustc/middle/trans/expr.rs index ff493c46a176..1a9824dcfe8a 100644 --- a/src/librustc/middle/trans/expr.rs +++ b/src/librustc/middle/trans/expr.rs @@ -835,7 +835,7 @@ fn trans_lvalue_unadjusted(bcx: block, expr: @ast::expr) -> DatumBlock { } ast::expr_unary(ast::deref, base) => { let basedatum = unpack_datum!(bcx, trans_to_datum(bcx, base)); - basedatum.deref(bcx, base, 0) + basedatum.deref(bcx, expr, 0) } _ => { bcx.tcx().sess.span_bug(