From 603a75c8eaa8ee168e4333e4fba5eb782ed7192b Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sat, 26 Sep 2015 01:27:39 +0300 Subject: [PATCH 1/3] ensure that the types of methods are well-formed By RFC1214: Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF. The previous code only checked the trait-ref, which was not enough in several cases. As this is a soundness fix, it is a [breaking-change]. Fixes #28609 --- src/librustc_typeck/check/method/confirm.rs | 22 +++-- src/librustc_typeck/check/method/mod.rs | 3 + .../wf-method-late-bound-regions.rs | 33 ++++++++ .../wf-misc-methods-issue-28609.rs | 84 +++++++++++++++++++ src/test/compile-fail/wf-static-method.rs | 37 ++++++++ 5 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 src/test/compile-fail/wf-method-late-bound-regions.rs create mode 100644 src/test/compile-fail/wf-misc-methods-issue-28609.rs create mode 100644 src/test/compile-fail/wf-static-method.rs diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index b0e81803ba72f..72131627aa5d4 100644 --- a/src/librustc_typeck/check/method/confirm.rs +++ b/src/librustc_typeck/check/method/confirm.rs @@ -103,22 +103,23 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> { // Unify the (adjusted) self type with what the method expects. self.unify_receivers(self_ty, method_self_ty); - // Add any trait/regions obligations specified on the method's type parameters. - self.add_obligations(&pick, &all_substs, &method_predicates); - - // Create the final `MethodCallee`. + // Create the method type let method_ty = pick.item.as_opt_method().unwrap(); let fty = self.tcx().mk_fn(None, self.tcx().mk_bare_fn(ty::BareFnTy { sig: ty::Binder(method_sig), unsafety: method_ty.fty.unsafety, abi: method_ty.fty.abi.clone(), })); + + // Add any trait/regions obligations specified on the method's type parameters. + self.add_obligations(fty, &all_substs, &method_predicates); + + // Create the final `MethodCallee`. let callee = ty::MethodCallee { def_id: pick.item.def_id(), ty: fty, substs: self.tcx().mk_substs(all_substs) }; - // If this is an `&mut self` method, bias the receiver // expression towards mutability (this will switch // e.g. `Deref` to `DerefMut` in overloaded derefs and so on). @@ -422,11 +423,11 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> { } fn add_obligations(&mut self, - pick: &probe::Pick<'tcx>, + fty: Ty<'tcx>, all_substs: &subst::Substs<'tcx>, method_predicates: &ty::InstantiatedPredicates<'tcx>) { - debug!("add_obligations: pick={:?} all_substs={:?} method_predicates={:?}", - pick, + debug!("add_obligations: fty={:?} all_substs={:?} method_predicates={:?}", + fty, all_substs, method_predicates); @@ -439,6 +440,11 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> { self.fcx.add_wf_bounds( all_substs, self.call_expr); + + // the function type must also be well-formed (this is not + // implied by the substs being well-formed because of inherent + // impls and late-bound regions - see issue #28609). + self.fcx.register_wf_obligation(fty, self.span, traits::MiscObligation); } /////////////////////////////////////////////////////////////////////////// diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index edf1cc9b7ef38..e0ad51b4ea1b5 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/src/librustc_typeck/check/method/mod.rs @@ -255,6 +255,9 @@ pub fn lookup_in_trait_adjusted<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, traits::ObligationCause::misc(span, fcx.body_id), &method_bounds); + // Also register an obligation for the method type being well-formed. + fcx.register_wf_obligation(fty, span, traits::MiscObligation); + // FIXME(#18653) -- Try to resolve obligations, giving us more // typing information, which can sometimes be needed to avoid // pathological region inference failures. diff --git a/src/test/compile-fail/wf-method-late-bound-regions.rs b/src/test/compile-fail/wf-method-late-bound-regions.rs new file mode 100644 index 0000000000000..b9d292fd15685 --- /dev/null +++ b/src/test/compile-fail/wf-method-late-bound-regions.rs @@ -0,0 +1,33 @@ +// Copyright 2015 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. + +// A method's receiver must be well-formed, even if it has late-bound regions. +// Because of this, a method's substs being well-formed does not imply that +// the method's implied bounds are met. + +struct Foo<'b>(Option<&'b ()>); + +trait Bar<'b> { + fn xmute<'a>(&'a self, u: &'b u32) -> &'a u32; +} + +impl<'b> Bar<'b> for Foo<'b> { + fn xmute<'a>(&'a self, u: &'b u32) -> &'a u32 { u } +} + +fn main() { + let f = Foo(None); + let f2 = f; + let dangling = { + let pointer = Box::new(42); + f2.xmute(&pointer) //~ ERROR `pointer` does not live long enough + }; + println!("{}", dangling); +} diff --git a/src/test/compile-fail/wf-misc-methods-issue-28609.rs b/src/test/compile-fail/wf-misc-methods-issue-28609.rs new file mode 100644 index 0000000000000..055c86a03a7e6 --- /dev/null +++ b/src/test/compile-fail/wf-misc-methods-issue-28609.rs @@ -0,0 +1,84 @@ +// Copyright 2015 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 misc. method calls are well-formed + +use std::marker::PhantomData; +use std::ops::{Deref, Shl}; + +#[derive(Copy, Clone)] +struct S<'a, 'b: 'a> { + marker: PhantomData<&'a &'b ()>, + bomb: Option<&'b u32> +} + +type S2<'a> = S<'a, 'a>; + +impl<'a, 'b> S<'a, 'b> { + fn transmute_inherent(&self, a: &'b u32) -> &'a u32 { + a + } +} + +fn return_dangling_pointer_inherent(s: S2) -> &u32 { + let s = s; + s.transmute_inherent(&mut 42) //~ ERROR does not live long enough +} + +impl<'a, 'b> Deref for S<'a, 'b> { + type Target = &'a u32; + fn deref(&self) -> &&'a u32 { + self.bomb.as_ref().unwrap() + } +} + +fn return_dangling_pointer_coerce(s: S2) -> &u32 { + let four = 4; + let mut s = s; + s.bomb = Some(&four); //~ ERROR does not live long enough + &s +} + +fn return_dangling_pointer_unary_op(s: S2) -> &u32 { + let four = 4; + let mut s = s; + s.bomb = Some(&four); //~ ERROR does not live long enough + &*s +} + +impl<'a, 'b> Shl<&'b u32> for S<'a, 'b> { + type Output = &'a u32; + fn shl(self, t: &'b u32) -> &'a u32 { t } +} + +fn return_dangling_pointer_binary_op(s: S2) -> &u32 { + let s = s; + s << &mut 3 //~ ERROR does not live long enough +} + +fn return_dangling_pointer_method(s: S2) -> &u32 { + let s = s; + s.shl(&mut 3) //~ ERROR does not live long enough +} + +fn return_dangling_pointer_ufcs(s: S2) -> &u32 { + let s = s; + S2::shl(s, &mut 3) //~ ERROR does not live long enough +} + +fn main() { + let s = S { marker: PhantomData, bomb: None }; + let _inherent_dp = return_dangling_pointer_inherent(s); + let _coerce_dp = return_dangling_pointer_coerce(s); + let _unary_dp = return_dangling_pointer_unary_op(s); + let _binary_dp = return_dangling_pointer_binary_op(s); + let _method_dp = return_dangling_pointer_method(s); + let _ufcs_dp = return_dangling_pointer_ufcs(s); +} diff --git a/src/test/compile-fail/wf-static-method.rs b/src/test/compile-fail/wf-static-method.rs new file mode 100644 index 0000000000000..8f9b0cb80ba57 --- /dev/null +++ b/src/test/compile-fail/wf-static-method.rs @@ -0,0 +1,37 @@ +// Copyright 2015 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 static methods don't get to assume `Self` is well-formed + +trait Foo<'a, 'b>: Sized { + fn make_me() -> Self { loop {} } + fn static_evil(u: &'a u32) -> &'b u32; +} + +struct Evil<'a, 'b: 'a>(Option<&'a &'b ()>); + +impl<'a, 'b> Foo<'a, 'b> for Evil<'a, 'b> { + fn make_me() -> Self { Evil(None) } + fn static_evil(u: &'a u32) -> &'b u32 { + u //~ ERROR cannot infer an appropriate lifetime + } +} + +struct IndirectEvil<'a, 'b: 'a>(Option<&'a &'b ()>); + +impl<'a, 'b> Foo<'a, 'b> for IndirectEvil<'a, 'b> { + fn make_me() -> Self { IndirectEvil(None) } + fn static_evil(u: &'a u32) -> &'b u32 { + let me = Self::make_me(); //~ ERROR lifetime bound not satisfied + loop {} // (`me` could be used for the lifetime transmute). + } +} + +fn main() {} From ce70207250cf4397211939153826d6959f1133fa Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sat, 26 Sep 2015 02:52:46 +0300 Subject: [PATCH 2/3] fix fallout looks like some mix of #18653 and `projection_must_outlive`, but that needs to be investigated further (crater run?) --- src/librustc/middle/ty/structural_impls.rs | 4 +++- src/libstd/thread/mod.rs | 3 ++- src/test/compile-fail/issue-18959.rs | 1 + src/test/compile-fail/object-safety-issue-22040.rs | 1 + src/test/compile-fail/trait-test-2.rs | 1 + 5 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/librustc/middle/ty/structural_impls.rs b/src/librustc/middle/ty/structural_impls.rs index 4ed87e673d996..176dc5a743d05 100644 --- a/src/librustc/middle/ty/structural_impls.rs +++ b/src/librustc/middle/ty/structural_impls.rs @@ -417,7 +417,9 @@ impl<'tcx, A: Lift<'tcx>, B: Lift<'tcx>> Lift<'tcx> for (A, B) { impl<'tcx, T: Lift<'tcx>> Lift<'tcx> for [T] { type Lifted = Vec; fn lift_to_tcx(&self, tcx: &ty::ctxt<'tcx>) -> Option { - let mut result = Vec::with_capacity(self.len()); + // type annotation needed to inform `projection_must_outlive` + let mut result : Vec<>::Lifted> + = Vec::with_capacity(self.len()); for x in self { if let Some(value) = tcx.lift(x) { result.push(value); diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index 43c23ec8a4715..134b1ce16e2b6 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -266,7 +266,8 @@ impl Builder { let my_thread = Thread::new(name); let their_thread = my_thread.clone(); - let my_packet = Arc::new(UnsafeCell::new(None)); + let my_packet : Arc>>> + = Arc::new(UnsafeCell::new(None)); let their_packet = my_packet.clone(); let main = move || { diff --git a/src/test/compile-fail/issue-18959.rs b/src/test/compile-fail/issue-18959.rs index 95176da9020d5..5f6216a898a0b 100644 --- a/src/test/compile-fail/issue-18959.rs +++ b/src/test/compile-fail/issue-18959.rs @@ -22,6 +22,7 @@ fn foo(b: &Bar) { b.foo(&0) //~^ ERROR the trait `Foo` is not implemented for the type `Bar` //~| ERROR E0038 + //~| WARNING E0038 } fn main() { diff --git a/src/test/compile-fail/object-safety-issue-22040.rs b/src/test/compile-fail/object-safety-issue-22040.rs index 12407f06ca06c..06d2441d3c024 100644 --- a/src/test/compile-fail/object-safety-issue-22040.rs +++ b/src/test/compile-fail/object-safety-issue-22040.rs @@ -26,6 +26,7 @@ struct SExpr<'x> { impl<'x> PartialEq for SExpr<'x> { fn eq(&self, other:&SExpr<'x>) -> bool { println!("L1: {} L2: {}", self.elements.len(), other.elements.len()); + let result = self.elements.len() == other.elements.len(); println!("Got compare {}", result); diff --git a/src/test/compile-fail/trait-test-2.rs b/src/test/compile-fail/trait-test-2.rs index b11cbde292969..13fe314fbcdd1 100644 --- a/src/test/compile-fail/trait-test-2.rs +++ b/src/test/compile-fail/trait-test-2.rs @@ -21,4 +21,5 @@ fn main() { //~^ ERROR E0038 //~| ERROR E0038 //~| ERROR E0277 + //~| WARNING E0038 } From 2f23e171cb51b0f8b3658d0149217bde8e730059 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Fri, 2 Oct 2015 23:52:18 +0300 Subject: [PATCH 3/3] use the correct subtyping order in a test also, ensure that callers are checked. --- src/test/compile-fail/wf-static-method.rs | 43 ++++++++++++++++++----- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/src/test/compile-fail/wf-static-method.rs b/src/test/compile-fail/wf-static-method.rs index 8f9b0cb80ba57..6c6522fe658d2 100644 --- a/src/test/compile-fail/wf-static-method.rs +++ b/src/test/compile-fail/wf-static-method.rs @@ -8,30 +8,57 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// check that static methods don't get to assume `Self` is well-formed +// check that static methods don't get to assume their trait-ref +// is well-formed. +// FIXME(#27579): this is just a bug. However, our checking with +// static inherent methods isn't quite working - need to +// fix that before removing the check. -trait Foo<'a, 'b>: Sized { +trait Foo<'a, 'b, T>: Sized { fn make_me() -> Self { loop {} } - fn static_evil(u: &'a u32) -> &'b u32; + fn static_evil(u: &'b u32) -> &'a u32; } struct Evil<'a, 'b: 'a>(Option<&'a &'b ()>); -impl<'a, 'b> Foo<'a, 'b> for Evil<'a, 'b> { - fn make_me() -> Self { Evil(None) } - fn static_evil(u: &'a u32) -> &'b u32 { +impl<'a, 'b> Foo<'a, 'b, Evil<'a, 'b>> for () { + fn make_me() -> Self { } + fn static_evil(u: &'b u32) -> &'a u32 { u //~ ERROR cannot infer an appropriate lifetime } } struct IndirectEvil<'a, 'b: 'a>(Option<&'a &'b ()>); -impl<'a, 'b> Foo<'a, 'b> for IndirectEvil<'a, 'b> { +impl<'a, 'b> Foo<'a, 'b, ()> for IndirectEvil<'a, 'b> { fn make_me() -> Self { IndirectEvil(None) } - fn static_evil(u: &'a u32) -> &'b u32 { + fn static_evil(u: &'b u32) -> &'a u32 { let me = Self::make_me(); //~ ERROR lifetime bound not satisfied loop {} // (`me` could be used for the lifetime transmute). } } +impl<'a, 'b> Evil<'a, 'b> { + fn inherent_evil(u: &'b u32) -> &'a u32 { + u //~ ERROR cannot infer an appropriate lifetime + } +} + +// while static methods don't get to *assume* this, we still +// *check* that they hold. + +fn evil<'a, 'b>(b: &'b u32) -> &'a u32 { + <()>::static_evil(b) //~ ERROR cannot infer an appropriate lifetime +} + +fn indirect_evil<'a, 'b>(b: &'b u32) -> &'a u32 { + ::static_evil(b) + //~^ ERROR cannot infer an appropriate lifetime +} + +fn inherent_evil<'a, 'b>(b: &'b u32) -> &'a u32 { + ::inherent_evil(b) // bug? shouldn't this be an error +} + + fn main() {}