From ca98fefd04b2a0ccd784f96538c824c49210a418 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 4 Dec 2014 20:06:41 -0500 Subject: [PATCH 1/3] Fix two bugs in HRTB: 1. Categorize early-vs-late bindings on impls when constructing generics, so that we don't add unnecessary region parameters. 2. Correct the DeBruijn indices when substituting the self type into the method signature. Previously, the DeBruijn index for the self type was not being adjusted to account for the fn binder. This mean that when late-bound regions were instantiated, you sometimes wind up with two distinct lifetimes. Fixes #19537. --- src/librustc_typeck/astconv.rs | 17 +++++++---- src/librustc_typeck/collect.rs | 20 ++++++++++++- .../compile-fail/hrtb-debruijn-in-receiver.rs | 28 +++++++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 src/test/compile-fail/hrtb-debruijn-in-receiver.rs diff --git a/src/librustc_typeck/astconv.rs b/src/librustc_typeck/astconv.rs index 7f1aad8ca77c5..790d882f836f5 100644 --- a/src/librustc_typeck/astconv.rs +++ b/src/librustc_typeck/astconv.rs @@ -54,6 +54,7 @@ use middle::resolve_lifetime as rl; use middle::subst::{FnSpace, TypeSpace, AssocSpace, SelfSpace, Subst, Substs}; use middle::subst::{VecPerParamSpace}; use middle::ty::{mod, Ty}; +use middle::ty_fold; use rscope::{mod, UnelidableRscope, RegionScope, SpecificRscope, ShiftedRscope, BindingRscope}; use TypeAndSubsts; @@ -1062,7 +1063,8 @@ fn ty_of_method_or_bare_fn<'a, 'tcx, AC: AstConv<'tcx>>( opt_self_info: Option>, decl: &ast::FnDecl) -> (ty::BareFnTy<'tcx>, - Option) { + Option) +{ debug!("ty_of_method_or_bare_fn"); // New region names that appear inside of the arguments of the function @@ -1078,6 +1080,11 @@ fn ty_of_method_or_bare_fn<'a, 'tcx, AC: AstConv<'tcx>>( let (self_ty, mut implied_output_region) = match opt_self_info { None => (None, None), Some(self_info) => { + // Shift regions in the self type by 1 to account for the binding + // level introduced by the function itself. + let untransformed_self_ty = + ty_fold::shift_regions(this.tcx(), 1, &self_info.untransformed_self_ty); + // Figure out and record the explicit self category. let explicit_self_category = determine_explicit_self_category(this, &rb, &self_info); @@ -1087,21 +1094,19 @@ fn ty_of_method_or_bare_fn<'a, 'tcx, AC: AstConv<'tcx>>( (None, None) } ty::ByValueExplicitSelfCategory => { - (Some(self_info.untransformed_self_ty), None) + (Some(untransformed_self_ty), None) } ty::ByReferenceExplicitSelfCategory(region, mutability) => { (Some(ty::mk_rptr(this.tcx(), region, ty::mt { - ty: self_info.untransformed_self_ty, + ty: untransformed_self_ty, mutbl: mutability })), Some(region)) } ty::ByBoxExplicitSelfCategory => { - (Some(ty::mk_uniq(this.tcx(), - self_info.untransformed_self_ty)), - None) + (Some(ty::mk_uniq(this.tcx(), untransformed_self_ty)), None) } } } diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 74ac9c480defe..4ad6dc292a72d 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -1055,7 +1055,7 @@ pub fn convert(ccx: &CrateCtxt, it: &ast::Item) { ref selfty, ref impl_items) => { // Create generics from the generics specified in the impl head. - let ty_generics = ty_generics_for_type( + let ty_generics = ty_generics_for_impl( ccx, generics, CreateTypeParametersForAssociatedTypes); @@ -1655,6 +1655,24 @@ fn ty_generics_for_trait<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>, generics } +fn ty_generics_for_impl<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>, + generics: &ast::Generics, + create_type_parameters_for_associated_types: + CreateTypeParametersForAssociatedTypesFlag) + -> ty::Generics<'tcx> +{ + let early_lifetimes = resolve_lifetime::early_bound_lifetimes(generics); + debug!("ty_generics_for_impl: early_lifetimes={}", + early_lifetimes); + ty_generics(ccx, + subst::TypeSpace, + early_lifetimes.as_slice(), + generics.ty_params.as_slice(), + ty::Generics::empty(), + &generics.where_clause, + create_type_parameters_for_associated_types) +} + fn ty_generics_for_fn_or_method<'tcx,AC>( this: &AC, generics: &ast::Generics, diff --git a/src/test/compile-fail/hrtb-debruijn-in-receiver.rs b/src/test/compile-fail/hrtb-debruijn-in-receiver.rs new file mode 100644 index 0000000000000..2dbd16107b0d5 --- /dev/null +++ b/src/test/compile-fail/hrtb-debruijn-in-receiver.rs @@ -0,0 +1,28 @@ +// 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 the case where the `Self` type has a bound lifetime that must +// be adjusted in the fn signature. Issue #19537. + +use std::collections::HashMap; + +struct Foo<'a> { + map: HashMap +} + +impl<'a> Foo<'a> { + fn new() -> Foo<'a> { panic!() } + fn insert(&'a mut self) { } +} +fn main() { + let mut foo = Foo::new(); + foo.insert(); + foo.insert(); //~ ERROR cannot borrow +} From 31e46ac0a95d58fa95dcd154a0f8a56089e2f5b9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 6 Dec 2014 11:55:38 -0800 Subject: [PATCH 2/3] During method resolution, only reborrow if we are not doing an auto-ref. The current behavior leads to adjustments like `&&*` being applied instead of just `&` (when the unmodified receiver is a `&T` or an `&mut T`). This causes both safety errors and unexpected behavior. The safety errors result from regionck not being prepared for auto-ref-ref-like adjustments; this is worth fixing on its own, but I think the best way to do it is to modify regionck to use expr-use-visitor (and fix expr-use-visitor as well, which I don't think properly invokes `borrow` for each level of auto-ref), and for now it's simpler to just not produce the adjustment in question. (I have a separate patch porting regionck to use exprusevisitor for a different bug, so that is coming.) --- src/librustc_typeck/check/method/mod.rs | 1 + src/librustc_typeck/check/method/probe.rs | 39 +++++++++----- ...owck-return-variable-on-stack-via-clone.rs | 20 +++++++ ...thod-mut-self-modifies-mut-slice-lvalue.rs | 54 +++++++++++++++++++ 4 files changed, 102 insertions(+), 12 deletions(-) create mode 100644 src/test/compile-fail/borrowck-return-variable-on-stack-via-clone.rs create mode 100644 src/test/run-pass/method-mut-self-modifies-mut-slice-lvalue.rs diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index b6c9d8b2d2176..53bda93b28e02 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/src/librustc_typeck/check/method/mod.rs @@ -100,6 +100,7 @@ pub fn lookup<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, call_expr.repr(fcx.tcx()), self_expr.repr(fcx.tcx())); + let self_ty = fcx.infcx().resolve_type_vars_if_possible(self_ty); let pick = try!(probe::probe(fcx, span, method_name, self_ty, call_expr.id)); Ok(confirm::confirm(fcx, span, self_expr, call_expr, self_ty, pick, supplied_method_types)) } diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index 6ff276edbce7e..18ab4f79b0b9a 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -168,7 +168,7 @@ fn create_steps<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, check::autoderef( fcx, span, self_ty, None, NoPreference, |t, d| { - let adjustment = consider_reborrow(t, d); + let adjustment = AutoDeref(d); steps.push(CandidateStep { self_ty: t, adjustment: adjustment }); None::<()> // keep iterating until we can't anymore }); @@ -185,14 +185,6 @@ fn create_steps<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, } return steps; - - fn consider_reborrow(ty: Ty, d: uint) -> PickAdjustment { - // Insert a `&*` or `&mut *` if this is a reference type: - match ty.sty { - ty::ty_rptr(_, ref mt) => AutoRef(mt.mutbl, box AutoDeref(d+1)), - _ => AutoDeref(d), - } - } } impl<'a,'tcx> ProbeContext<'a,'tcx> { @@ -626,7 +618,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { return None; } - match self.pick_adjusted_method(step) { + match self.pick_by_value_method(step) { Some(result) => return Some(result), None => {} } @@ -644,11 +636,34 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { } } - fn pick_adjusted_method(&mut self, + fn pick_by_value_method(&mut self, step: &CandidateStep<'tcx>) -> Option> { - self.pick_method(step.self_ty).map(|r| self.adjust(r, step.adjustment.clone())) + /*! + * For each type `T` in the step list, this attempts to find a + * method where the (transformed) self type is exactly `T`. We + * do however do one transformation on the adjustment: if we + * are passing a region pointer in, we will potentially + * *reborrow* it to a shorter lifetime. This allows us to + * transparently pass `&mut` pointers, in particular, without + * consuming them for their entire lifetime. + */ + + let adjustment = match step.adjustment { + AutoDeref(d) => consider_reborrow(step.self_ty, d), + AutoUnsizeLength(..) | AutoRef(..) => step.adjustment.clone(), + }; + + return self.pick_method(step.self_ty).map(|r| self.adjust(r, adjustment.clone())); + + fn consider_reborrow(ty: Ty, d: uint) -> PickAdjustment { + // Insert a `&*` or `&mut *` if this is a reference type: + match ty.sty { + ty::ty_rptr(_, ref mt) => AutoRef(mt.mutbl, box AutoDeref(d+1)), + _ => AutoDeref(d), + } + } } fn pick_autorefd_method(&mut self, diff --git a/src/test/compile-fail/borrowck-return-variable-on-stack-via-clone.rs b/src/test/compile-fail/borrowck-return-variable-on-stack-via-clone.rs new file mode 100644 index 0000000000000..7529943f0bc59 --- /dev/null +++ b/src/test/compile-fail/borrowck-return-variable-on-stack-via-clone.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. + +// Check that when we clone a `&T` pointer we properly relate the +// lifetime of the pointer which results to the pointer being cloned. +// Bugs in method resolution have sometimes broken this connection. +// Issue #19261. + +fn leak<'a, T>(x: T) -> &'a T { + (&x).clone() //~ ERROR `x` does not live long enough +} + +fn main() { } diff --git a/src/test/run-pass/method-mut-self-modifies-mut-slice-lvalue.rs b/src/test/run-pass/method-mut-self-modifies-mut-slice-lvalue.rs new file mode 100644 index 0000000000000..00319d57f8da6 --- /dev/null +++ b/src/test/run-pass/method-mut-self-modifies-mut-slice-lvalue.rs @@ -0,0 +1,54 @@ +// 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 an `&mut self` method, when invoked on an lvalue whose +// type is `&mut [u8]`, passes in a pointer to the lvalue and not a +// temporary. Issue #19147. + +use std::raw; +use std::mem; +use std::slice; +use std::io::IoResult; + +trait MyWriter { + fn my_write(&mut self, buf: &[u8]) -> IoResult<()>; +} + +impl<'a> MyWriter for &'a mut [u8] { + fn my_write(&mut self, buf: &[u8]) -> IoResult<()> { + slice::bytes::copy_memory(*self, buf); + + let write_len = buf.len(); + unsafe { + *self = mem::transmute(raw::Slice { + data: self.as_ptr().offset(write_len as int), + len: self.len() - write_len, + }); + } + + Ok(()) + } +} + +fn main() { + let mut buf = [0_u8, .. 6]; + + { + let mut writer = buf.as_mut_slice(); + writer.my_write(&[0, 1, 2]).unwrap(); + writer.my_write(&[3, 4, 5]).unwrap(); + } + + // If `my_write` is not modifying `buf` in place, then we will + // wind up with `[3, 4, 5, 0, 0, 0]` because the first call to + // `my_write()` doesn't update the starting point for the write. + + assert_eq!(buf, [0, 1, 2, 3, 4, 5]); +} From 061a87e5194cde49b5501323ff5ca7ca5a172441 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Dec 2014 04:16:11 -0500 Subject: [PATCH 3/3] Fix an incorrect type annotation (shadowed lifetime parameter) that was masked by these bugs. --- src/librustc_trans/trans/datum.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_trans/trans/datum.rs b/src/librustc_trans/trans/datum.rs index 23a261842b2d5..edcc8edaf7f92 100644 --- a/src/librustc_trans/trans/datum.rs +++ b/src/librustc_trans/trans/datum.rs @@ -535,10 +535,10 @@ impl<'tcx, K: KindOps + fmt::Show> Datum<'tcx, K> { /// Copies the value into a new location. This function always preserves the existing datum as /// a valid value. Therefore, it does not consume `self` and, also, cannot be applied to affine /// values (since they must never be duplicated). - pub fn shallow_copy<'blk, 'tcx>(&self, - bcx: Block<'blk, 'tcx>, - dst: ValueRef) - -> Block<'blk, 'tcx> { + pub fn shallow_copy<'blk>(&self, + bcx: Block<'blk, 'tcx>, + dst: ValueRef) + -> Block<'blk, 'tcx> { /*! * Copies the value into a new location. This function always * preserves the existing datum as a valid value. Therefore,