Skip to content

Commit 5b2e869

Browse files
committed
Reject specialized Drop impls.
See Issue 8142 for discussion. This makes it illegal for a Drop impl to be more specialized than the original item. So for example, all of the following are now rejected (when they would have been blindly accepted before): ```rust struct S<A> { ... }; impl Drop for S<i8> { ... } // error: specialized to concrete type struct T<'a> { ... }; impl Drop for T<'static> { ... } // error: specialized to concrete region struct U<A> { ... }; impl<A:Clone> Drop for U<A> { ... } // error: added extra type requirement struct V<'a,'b>; impl<'a,'b:a> Drop for V<'a,'b> { ... } // error: added extra region requirement ``` Due to examples like the above, this is a [breaking-change]. (The fix is to either remove the specialization from the `Drop` impl, or to transcribe the requirements into the struct/enum definition; examples of both are shown in the PR's fixed to `libstd`.) ---- This is likely to be the last thing blocking the removal of the `#[unsafe_destructor]` attribute. Includes two new error codes for the new dropck check. Update run-pass tests to accommodate new dropck pass. Update tests and docs to reflect new destructor restriction. ---- Implementation notes: We identify Drop impl specialization by not being as parametric as the struct/enum definition via unification. More specifically: 1. Attempt unification of a skolemized instance of the struct/enum with an instance of the Drop impl's type expression where all of the impl's generics (i.e. the free variables of the type expression) have been replaced with unification variables. 2. If unification fails, then reject Drop impl as specialized. 3. If unification succeeds, check if any of the skolemized variables "leaked" into the constraint set for the inference context; if so, then reject Drop impl as specialized. 4. Otherwise, unification succeeded without leaking skolemized variables: accept the Drop impl. We identify whether a Drop impl is injecting new predicates by simply looking whether the predicate, after an appropriate substitution, appears on the struct/enum definition.
1 parent 290c8de commit 5b2e869

File tree

13 files changed

+341
-18
lines changed

13 files changed

+341
-18
lines changed

src/doc/trpl/unsafe.md

+9-8
Original file line numberDiff line numberDiff line change
@@ -197,15 +197,16 @@ use std::ptr;
197197
198198
// Define a wrapper around the handle returned by the foreign code.
199199
// Unique<T> has the same semantics as Box<T>
200-
pub struct Unique<T> {
200+
//
201+
// NB: For simplicity and correctness, we require that T has kind Send
202+
// (owned boxes relax this restriction).
203+
pub struct Unique<T: Send> {
201204
// It contains a single raw, mutable pointer to the object in question.
202205
ptr: *mut T
203206
}
204207
205208
// Implement methods for creating and using the values in the box.
206209
207-
// NB: For simplicity and correctness, we require that T has kind Send
208-
// (owned boxes relax this restriction).
209210
impl<T: Send> Unique<T> {
210211
pub fn new(value: T) -> Unique<T> {
211212
unsafe {
@@ -239,11 +240,11 @@ impl<T: Send> Unique<T> {
239240
// Unique<T>, making the struct manage the raw pointer: when the
240241
// struct goes out of scope, it will automatically free the raw pointer.
241242
//
242-
// NB: This is an unsafe destructor, because rustc will not normally
243-
// allow destructors to be associated with parameterized types, due to
244-
// bad interaction with managed boxes. (With the Send restriction,
245-
// we don't have this problem.) Note that the `#[unsafe_destructor]`
246-
// feature gate is required to use unsafe destructors.
243+
// NB: This is an unsafe destructor; rustc will not normally allow
244+
// destructors to be associated with parameterized types (due to
245+
// historically failing to check them soundly). Note that the
246+
// `#[unsafe_destructor]` feature gate is currently required to use
247+
// unsafe destructors.
247248
#[unsafe_destructor]
248249
impl<T: Send> Drop for Unique<T> {
249250
fn drop(&mut self) {

src/librustc/middle/infer/higher_ranked/mod.rs

+58
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use super::{CombinedSnapshot, cres, InferCtxt, HigherRankedType, SkolemizationMap};
1515
use super::combine::{Combine, Combineable};
1616

17+
use middle::subst;
1718
use middle::ty::{self, Binder};
1819
use middle::ty_fold::{self, TypeFoldable};
1920
use syntax::codemap::Span;
@@ -455,6 +456,63 @@ impl<'a,'tcx> InferCtxtExt for InferCtxt<'a,'tcx> {
455456
}
456457
}
457458

459+
/// Constructs and returns a substitution that, for a given type
460+
/// scheme parameterized by `generics`, will replace every generic
461+
/// parmeter in the type with a skolemized type/region (which one can
462+
/// think of as a "fresh constant", except at the type/region level of
463+
/// reasoning).
464+
///
465+
/// Since we currently represent bound/free type parameters in the
466+
/// same way, this only has an effect on regions.
467+
///
468+
/// (Note that unlike a substitution from `ty::construct_free_substs`,
469+
/// this inserts skolemized regions rather than free regions; this
470+
/// allows one to use `fn leak_check` to catch attmepts to unify the
471+
/// skolemized regions with e.g. the `'static` lifetime)
472+
pub fn construct_skolemized_substs<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>,
473+
generics: &ty::Generics<'tcx>,
474+
snapshot: &CombinedSnapshot)
475+
-> (subst::Substs<'tcx>, SkolemizationMap)
476+
{
477+
let mut map = FnvHashMap();
478+
479+
// map T => T
480+
let mut types = subst::VecPerParamSpace::empty();
481+
push_types_from_defs(infcx.tcx, &mut types, generics.types.as_slice());
482+
483+
// map early- or late-bound 'a => fresh 'a
484+
let mut regions = subst::VecPerParamSpace::empty();
485+
push_region_params(infcx, &mut map, &mut regions, generics.regions.as_slice(), snapshot);
486+
487+
let substs = subst::Substs { types: types,
488+
regions: subst::NonerasedRegions(regions) };
489+
return (substs, map);
490+
491+
fn push_region_params<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>,
492+
map: &mut SkolemizationMap,
493+
regions: &mut subst::VecPerParamSpace<ty::Region>,
494+
region_params: &[ty::RegionParameterDef],
495+
snapshot: &CombinedSnapshot)
496+
{
497+
for r in region_params {
498+
let br = r.to_bound_region();
499+
let skol_var = infcx.region_vars.new_skolemized(br, &snapshot.region_vars_snapshot);
500+
let sanity_check = map.insert(br, skol_var);
501+
assert!(sanity_check.is_none());
502+
regions.push(r.space, skol_var);
503+
}
504+
}
505+
506+
fn push_types_from_defs<'tcx>(tcx: &ty::ctxt<'tcx>,
507+
types: &mut subst::VecPerParamSpace<ty::Ty<'tcx>>,
508+
defs: &[ty::TypeParameterDef<'tcx>]) {
509+
for def in defs {
510+
let ty = ty::mk_param_from_def(tcx, def);
511+
types.push(def.space, ty);
512+
}
513+
}
514+
}
515+
458516
pub fn skolemize_late_bound_regions<'a,'tcx,T>(infcx: &InferCtxt<'a,'tcx>,
459517
binder: &ty::Binder<T>,
460518
snapshot: &CombinedSnapshot)

src/librustc/middle/infer/mod.rs

+9
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
726726
})
727727
}
728728

729+
pub fn construct_skolemized_subst(&self,
730+
generics: &ty::Generics<'tcx>,
731+
snapshot: &CombinedSnapshot)
732+
-> (subst::Substs<'tcx>, SkolemizationMap) {
733+
/*! See `higher_ranked::construct_skolemized_subst` */
734+
735+
higher_ranked::construct_skolemized_substs(self, generics, snapshot)
736+
}
737+
729738
pub fn skolemize_late_bound_regions<T>(&self,
730739
value: &ty::Binder<T>,
731740
snapshot: &CombinedSnapshot)

src/librustc/middle/ty.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1793,6 +1793,9 @@ impl RegionParameterDef {
17931793
pub fn to_early_bound_region(&self) -> ty::Region {
17941794
ty::ReEarlyBound(self.def_id.node, self.space, self.index, self.name)
17951795
}
1796+
pub fn to_bound_region(&self) -> ty::BoundRegion {
1797+
ty::BoundRegion::BrNamed(self.def_id, self.name)
1798+
}
17961799
}
17971800

17981801
/// Information about the formal type/lifetime parameters associated

src/librustc_typeck/check/dropck.rs

+238-2
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,249 @@ use check::regionck::{self, Rcx};
1212

1313
use middle::infer;
1414
use middle::region;
15-
use middle::subst;
15+
use middle::subst::{self, Subst};
1616
use middle::ty::{self, Ty};
1717
use util::ppaux::{Repr, UserString};
1818

1919
use syntax::ast;
20-
use syntax::codemap::Span;
20+
use syntax::codemap::{self, Span};
21+
22+
/// check_drop_impl confirms that the Drop implementation identfied by
23+
/// `drop_impl_did` is not any more specialized than the type it is
24+
/// attached to (Issue #8142).
25+
///
26+
/// This means:
27+
///
28+
/// 1. The self type must be nominal (this is already checked during
29+
/// coherence),
30+
///
31+
/// 2. The generic region/type parameters of the impl's self-type must
32+
/// all be parameters of the Drop impl itself (i.e. no
33+
/// specialization like `impl Drop for Foo<i32>`), and,
34+
///
35+
/// 3. Any bounds on the generic parameters must be reflected in the
36+
/// struct/enum definition for the nominal type itself (i.e.
37+
/// cannot do `struct S<T>; impl<T:Clone> Drop for S<T> { ... }`).
38+
///
39+
pub fn check_drop_impl(tcx: &ty::ctxt, drop_impl_did: ast::DefId) -> Result<(), ()> {
40+
let ty::TypeScheme { generics: ref dtor_generics,
41+
ty: ref dtor_self_type } = ty::lookup_item_type(tcx, drop_impl_did);
42+
let dtor_predicates = ty::lookup_predicates(tcx, drop_impl_did);
43+
match dtor_self_type.sty {
44+
ty::ty_enum(self_type_did, self_to_impl_substs) |
45+
ty::ty_struct(self_type_did, self_to_impl_substs) |
46+
ty::ty_closure(self_type_did, self_to_impl_substs) => {
47+
try!(ensure_drop_params_and_item_params_correspond(tcx,
48+
drop_impl_did,
49+
dtor_generics,
50+
dtor_self_type,
51+
self_type_did));
52+
53+
ensure_drop_predicates_are_implied_by_item_defn(tcx,
54+
drop_impl_did,
55+
&dtor_predicates,
56+
self_type_did,
57+
self_to_impl_substs)
58+
}
59+
_ => {
60+
// Destructors only work on nominal types. This was
61+
// already checked by coherence, so we can panic here.
62+
let span = tcx.map.def_id_span(drop_impl_did, codemap::DUMMY_SP);
63+
tcx.sess.span_bug(
64+
span, &format!("should have been rejected by coherence check: {}",
65+
dtor_self_type.repr(tcx)));
66+
}
67+
}
68+
}
69+
70+
fn ensure_drop_params_and_item_params_correspond<'tcx>(
71+
tcx: &ty::ctxt<'tcx>,
72+
drop_impl_did: ast::DefId,
73+
drop_impl_generics: &ty::Generics<'tcx>,
74+
drop_impl_ty: &ty::Ty<'tcx>,
75+
self_type_did: ast::DefId) -> Result<(), ()>
76+
{
77+
// New strategy based on review suggestion from nikomatsakis.
78+
//
79+
// (In the text and code below, "named" denotes "struct/enum", and
80+
// "generic params" denotes "type and region params")
81+
//
82+
// 1. Create fresh skolemized type/region "constants" for each of
83+
// the named type's generic params. Instantiate the named type
84+
// with the fresh constants, yielding `named_skolem`.
85+
//
86+
// 2. Create unification variables for each of the Drop impl's
87+
// generic params. Instantiate the impl's Self's type with the
88+
// unification-vars, yielding `drop_unifier`.
89+
//
90+
// 3. Attempt to unify Self_unif with Type_skolem. If unification
91+
// succeeds, continue (i.e. with the predicate checks).
92+
93+
let ty::TypeScheme { generics: ref named_type_generics,
94+
ty: named_type } =
95+
ty::lookup_item_type(tcx, self_type_did);
96+
97+
let infcx = infer::new_infer_ctxt(tcx);
98+
infcx.try(|snapshot| {
99+
let (named_type_to_skolem, skol_map) =
100+
infcx.construct_skolemized_subst(named_type_generics, snapshot);
101+
let named_type_skolem = named_type.subst(tcx, &named_type_to_skolem);
102+
103+
let drop_impl_span = tcx.map.def_id_span(drop_impl_did, codemap::DUMMY_SP);
104+
let drop_to_unifier =
105+
infcx.fresh_substs_for_generics(drop_impl_span, drop_impl_generics);
106+
let drop_unifier = drop_impl_ty.subst(tcx, &drop_to_unifier);
107+
108+
if let Ok(()) = infer::mk_eqty(&infcx, true, infer::TypeOrigin::Misc(drop_impl_span),
109+
named_type_skolem, drop_unifier) {
110+
// Even if we did manage to equate the types, the process
111+
// may have just gathered unsolvable region constraints
112+
// like `R == 'static` (represented as a pair of subregion
113+
// constraints) for some skolemization constant R.
114+
//
115+
// However, the leak_check method allows us to confirm
116+
// that no skolemized regions escaped (i.e. were related
117+
// to other regions in the constraint graph).
118+
if let Ok(()) = infcx.leak_check(&skol_map, snapshot) {
119+
return Ok(())
120+
}
121+
}
122+
123+
span_err!(tcx.sess, drop_impl_span, E0366,
124+
"Implementations of Drop cannot be specialized");
125+
let item_span = tcx.map.span(self_type_did.node);
126+
tcx.sess.span_note(item_span,
127+
"Use same sequence of generic type and region \
128+
parameters that is on the struct/enum definition");
129+
return Err(());
130+
})
131+
}
132+
133+
/// Confirms that every predicate imposed by dtor_predicates is
134+
/// implied by assuming the predicates attached to self_type_did.
135+
fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>(
136+
tcx: &ty::ctxt<'tcx>,
137+
drop_impl_did: ast::DefId,
138+
dtor_predicates: &ty::GenericPredicates<'tcx>,
139+
self_type_did: ast::DefId,
140+
self_to_impl_substs: &subst::Substs<'tcx>) -> Result<(), ()> {
141+
142+
// Here is an example, analogous to that from
143+
// `compare_impl_method`.
144+
//
145+
// Consider a struct type:
146+
//
147+
// struct Type<'c, 'b:'c, 'a> {
148+
// x: &'a Contents // (contents are irrelevant;
149+
// y: &'c Cell<&'b Contents>, // only the bounds matter for our purposes.)
150+
// }
151+
//
152+
// and a Drop impl:
153+
//
154+
// impl<'z, 'y:'z, 'x:'y> Drop for P<'z, 'y, 'x> {
155+
// fn drop(&mut self) { self.y.set(self.x); } // (only legal if 'x: 'y)
156+
// }
157+
//
158+
// We start out with self_to_impl_substs, that maps the generic
159+
// parameters of Type to that of the Drop impl.
160+
//
161+
// self_to_impl_substs = {'c => 'z, 'b => 'y, 'a => 'x}
162+
//
163+
// Applying this to the predicates (i.e. assumptions) provided by the item
164+
// definition yields the instantiated assumptions:
165+
//
166+
// ['y : 'z]
167+
//
168+
// We then check all of the predicates of the Drop impl:
169+
//
170+
// ['y:'z, 'x:'y]
171+
//
172+
// and ensure each is in the list of instantiated
173+
// assumptions. Here, `'y:'z` is present, but `'x:'y` is
174+
// absent. So we report an error that the Drop impl injected a
175+
// predicate that is not present on the struct definition.
176+
177+
assert_eq!(self_type_did.krate, ast::LOCAL_CRATE);
178+
179+
let drop_impl_span = tcx.map.def_id_span(drop_impl_did, codemap::DUMMY_SP);
180+
181+
// We can assume the predicates attached to struct/enum definition
182+
// hold.
183+
let generic_assumptions = ty::lookup_predicates(tcx, self_type_did);
184+
185+
let assumptions_in_impl_context = generic_assumptions.instantiate(tcx, &self_to_impl_substs);
186+
assert!(assumptions_in_impl_context.predicates.is_empty_in(subst::SelfSpace));
187+
assert!(assumptions_in_impl_context.predicates.is_empty_in(subst::FnSpace));
188+
let assumptions_in_impl_context =
189+
assumptions_in_impl_context.predicates.get_slice(subst::TypeSpace);
190+
191+
// An earlier version of this code attempted to do this checking
192+
// via the traits::fulfill machinery. However, it ran into trouble
193+
// since the fulfill machinery merely turns outlives-predicates
194+
// 'a:'b and T:'b into region inference constraints. It is simpler
195+
// just to look for all the predicates directly.
196+
197+
assert!(dtor_predicates.predicates.is_empty_in(subst::SelfSpace));
198+
assert!(dtor_predicates.predicates.is_empty_in(subst::FnSpace));
199+
let predicates = dtor_predicates.predicates.get_slice(subst::TypeSpace);
200+
for predicate in predicates {
201+
// (We do not need to worry about deep analysis of type
202+
// expressions etc because the Drop impls are already forced
203+
// to take on a structure that is roughly a alpha-renaming of
204+
// the generic parameters of the item definition.)
205+
206+
// This path now just checks *all* predicates via the direct
207+
// lookup, rather than using fulfill machinery.
208+
//
209+
// However, it may be more efficient in the future to batch
210+
// the analysis together via the fulfill , rather than the
211+
// repeated `contains` calls.
212+
213+
if !assumptions_in_impl_context.contains(&predicate) {
214+
let item_span = tcx.map.span(self_type_did.node);
215+
let req = predicate.user_string(tcx);
216+
span_err!(tcx.sess, drop_impl_span, E0367,
217+
"The requirement `{}` is added only by the Drop impl.", req);
218+
tcx.sess.span_note(item_span,
219+
"The same requirement must be part of \
220+
the struct/enum definition");
221+
}
222+
}
223+
224+
if tcx.sess.has_errors() {
225+
return Err(());
226+
}
227+
Ok(())
228+
}
21229

230+
/// check_safety_of_destructor_if_necessary confirms that the type
231+
/// expression `typ` conforms to the "Drop Check Rule" from the Sound
232+
/// Generic Drop (RFC 769).
233+
///
234+
/// ----
235+
///
236+
/// The Drop Check Rule is the following:
237+
///
238+
/// Let `v` be some value (either temporary or named) and 'a be some
239+
/// lifetime (scope). If the type of `v` owns data of type `D`, where
240+
///
241+
/// (1.) `D` has a lifetime- or type-parametric Drop implementation, and
242+
/// (2.) the structure of `D` can reach a reference of type `&'a _`, and
243+
/// (3.) either:
244+
///
245+
/// (A.) the Drop impl for `D` instantiates `D` at 'a directly,
246+
/// i.e. `D<'a>`, or,
247+
///
248+
/// (B.) the Drop impl for `D` has some type parameter with a
249+
/// trait bound `T` where `T` is a trait that has at least
250+
/// one method,
251+
///
252+
/// then 'a must strictly outlive the scope of v.
253+
///
254+
/// ----
255+
///
256+
/// This function is meant to by applied to the type for every
257+
/// expression in the program.
22258
pub fn check_safety_of_destructor_if_necessary<'a, 'tcx>(rcx: &mut Rcx<'a, 'tcx>,
23259
typ: ty::Ty<'tcx>,
24260
span: Span,

0 commit comments

Comments
 (0)