-
Notifications
You must be signed in to change notification settings - Fork 13.3k
#[may_dangle]
attribute
#37117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#[may_dangle]
attribute
#37117
Changes from 10 commits
b0eee76
4bb68be
e8ccc68
7d2d5bc
9a649c3
e185cd5
fa7f69c
8dd9493
c239fee
0271a9a
818ac08
85d2e4d
0d8f716
4124d8e
10a58ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1226,16 +1226,17 @@ impl<'a, 'gcx, 'tcx> Rebuilder<'a, 'gcx, 'tcx> { | |
lifetime: hir::Lifetime, | ||
region_names: &HashSet<ast::Name>) | ||
-> hir::HirVec<hir::TyParam> { | ||
ty_params.iter().map(|ty_param| { | ||
let bounds = self.rebuild_ty_param_bounds(ty_param.bounds.clone(), | ||
ty_params.into_iter().map(|ty_param| { | ||
let bounds = self.rebuild_ty_param_bounds(ty_param.bounds, | ||
lifetime, | ||
region_names); | ||
hir::TyParam { | ||
name: ty_param.name, | ||
id: ty_param.id, | ||
bounds: bounds, | ||
default: ty_param.default.clone(), | ||
default: ty_param.default, | ||
span: ty_param.span, | ||
pure_wrt_drop: ty_param.pure_wrt_drop, | ||
} | ||
}).collect() | ||
} | ||
|
@@ -1295,7 +1296,9 @@ impl<'a, 'gcx, 'tcx> Rebuilder<'a, 'gcx, 'tcx> { | |
let mut lifetimes = Vec::new(); | ||
for lt in add { | ||
lifetimes.push(hir::LifetimeDef { lifetime: *lt, | ||
bounds: hir::HirVec::new() }); | ||
bounds: hir::HirVec::new(), | ||
pure_wrt_drop: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Not that I care all that much, but the formatting here seems odd. I'd expect to either put the |
||
}); | ||
} | ||
for lt in &generics.lifetimes { | ||
if keep.contains(<.lifetime.name) || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -402,16 +402,27 @@ fn iterate_over_potentially_unsafe_regions_in_type<'a, 'b, 'gcx, 'tcx>( | |
// unbounded type parameter `T`, we must resume the recursive | ||
// analysis on `T` (since it would be ignored by | ||
// type_must_outlive). | ||
if has_dtor_of_interest(tcx, ty) { | ||
debug!("iterate_over_potentially_unsafe_regions_in_type \ | ||
{}ty: {} - is a dtorck type!", | ||
(0..depth).map(|_| ' ').collect::<String>(), | ||
ty); | ||
|
||
cx.rcx.type_must_outlive(infer::SubregionOrigin::SafeDestructor(cx.span), | ||
ty, tcx.mk_region(ty::ReScope(cx.parent_scope))); | ||
|
||
return Ok(()); | ||
let dropck_kind = has_dtor_of_interest(tcx, ty); | ||
debug!("iterate_over_potentially_unsafe_regions_in_type \ | ||
ty: {:?} dropck_kind: {:?}", ty, dropck_kind); | ||
match dropck_kind { | ||
DropckKind::NoBorrowedDataAccessedInMyDtor => { | ||
// The maximally blind attribute. | ||
} | ||
DropckKind::BorrowedDataMustStrictlyOutliveSelf => { | ||
cx.rcx.type_must_outlive(infer::SubregionOrigin::SafeDestructor(cx.span), | ||
ty, tcx.mk_region(ty::ReScope(cx.parent_scope))); | ||
return Ok(()); | ||
} | ||
DropckKind::RevisedSelf(revised_ty) => { | ||
cx.rcx.type_must_outlive(infer::SubregionOrigin::SafeDestructor(cx.span), | ||
revised_ty, tcx.mk_region(ty::ReScope(cx.parent_scope))); | ||
// Do not return early from this case; we want | ||
// to recursively process the internal structure of Self | ||
// (because even though the Drop for Self has been asserted | ||
// safe, the types instantiated for the generics of Self | ||
// may themselves carry dropck constraints.) | ||
} | ||
} | ||
|
||
debug!("iterate_over_potentially_unsafe_regions_in_type \ | ||
|
@@ -492,16 +503,145 @@ fn iterate_over_potentially_unsafe_regions_in_type<'a, 'b, 'gcx, 'tcx>( | |
} | ||
} | ||
|
||
#[derive(Copy, Clone, PartialEq, Eq, Debug)] | ||
enum DropckKind<'tcx> { | ||
/// The "safe" kind; i.e. conservatively assume any borrow | ||
/// accessed by dtor, and therefore such data must strictly | ||
/// outlive self. | ||
/// | ||
/// Equivalent to RevisedTy with no change to the self type. | ||
/// | ||
/// FIXME: this name may not be general enough; it should be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (given the followup note in the paragraph below this FIXME, I'm tempted to just delete this fixme and the comment below it. I think I added the fixme in response to a concern that @arielb1 had raised, but I am no longer sure.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete it, I say :) /me slayer of FIXMEs |
||
/// talking about Phantom lifetimes rather than just borrows. | ||
/// | ||
/// (actually, pnkfelix is not 100% sure that's the right | ||
/// viewpoint. If I'm holding a phantom lifetime just to | ||
/// constrain a reference type that occurs solely in *negative* | ||
/// type positions, then my destructor cannot itself ever actually | ||
/// access such references, right? And don't we end up essentially | ||
/// requring people to put a fake borrow inside a PhantomData in | ||
/// order to make phantom lifetimes work anyway?) | ||
BorrowedDataMustStrictlyOutliveSelf, | ||
|
||
/// The nearly completely-unsafe kind. | ||
/// | ||
/// Equivalent to RevisedSelf with *all* parameters remapped to () | ||
/// (maybe...?) | ||
NoBorrowedDataAccessedInMyDtor, | ||
|
||
/// Assume all borrowed data access by dtor occurs as if Self has the | ||
/// type carried by this variant. In practice this means that some | ||
/// of the type parameters are remapped to `()`, because the developer | ||
/// has asserted that the destructor will not access their contents. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "...and some region parameters mapped to |
||
RevisedSelf(Ty<'tcx>), | ||
} | ||
|
||
/// Returns the classification of what kind of check should be applied | ||
/// to `ty`, which may include a revised type where some of the type | ||
/// parameters are re-mapped to `()` to reflect the destructor's | ||
/// "purity" with respect to their actual contents. | ||
fn has_dtor_of_interest<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, | ||
ty: Ty<'tcx>) -> bool { | ||
ty: Ty<'tcx>) -> DropckKind<'tcx> { | ||
match ty.sty { | ||
ty::TyAdt(def, _) => { | ||
def.is_dtorck(tcx) | ||
ty::TyAdt(adt_def, substs) => { | ||
if !adt_def.is_dtorck(tcx) { | ||
return DropckKind::NoBorrowedDataAccessedInMyDtor; | ||
} | ||
|
||
// Find the `impl<..> Drop for _` to inspect any | ||
// attributes attached to the impl's generics. | ||
let opt_dtor_method = adt_def.destructor(); | ||
let dtor_method = if let Some(dtor_method) = opt_dtor_method { | ||
dtor_method | ||
} else { | ||
return DropckKind::BorrowedDataMustStrictlyOutliveSelf; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you put a comment here as to what this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having dug a bit deeper, I think that if the type |
||
}; | ||
|
||
let method = tcx.impl_or_trait_item(dtor_method); | ||
let impl_id: DefId = method.container().id(); | ||
let revised_ty = revise_self_ty(tcx, adt_def, impl_id, substs); | ||
return DropckKind::RevisedSelf(revised_ty); | ||
} | ||
ty::TyTrait(..) | ty::TyProjection(..) | ty::TyAnon(..) => { | ||
debug!("ty: {:?} isn't known, and therefore is a dropck type", ty); | ||
true | ||
return DropckKind::BorrowedDataMustStrictlyOutliveSelf; | ||
}, | ||
_ => false | ||
_ => { | ||
return DropckKind::NoBorrowedDataAccessedInMyDtor; | ||
} | ||
} | ||
} | ||
|
||
// Constructs new Ty just like the type defined by `adt_def` coupled | ||
// with `substs`, except each type and lifetime parameter marked as | ||
// `#[may_dangle]` in the Drop impl (identified by `impl_id`) is | ||
// respectively mapped to `()` or `'static`. | ||
// | ||
// For example: If the `adt_def` maps to: | ||
// | ||
// enum Foo<'a, X, Y> { ... } | ||
// | ||
// and the `impl_id` maps to: | ||
// | ||
// impl<#[may_dangle] 'a, X, #[may_dangle] Y> Drop for Foo<'a, X, Y> { ... } | ||
// | ||
// then revises input: `Foo<'r,i64,&'r i64>` to: `Foo<'static,i64,()>` | ||
fn revise_self_ty<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, | ||
adt_def: ty::AdtDef<'tcx>, | ||
impl_id: DefId, | ||
substs: &Substs<'tcx>) -> Ty<'tcx> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: not that I care all that much, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, at least one other fn definition in this file uses this style (I think I may have even double checked about that when I first wrote this helper), but I'm not wedded to the style. |
||
// Get generics for `impl Drop` to query for `#[may_dangle]` attr. | ||
let impl_bindings = tcx.lookup_generics(impl_id); | ||
|
||
// Get Substs attached to Self on `impl Drop`; process in parallel | ||
// with `substs`, replacing dangling entries as appropriate. | ||
let self_substs = { | ||
let impl_self_ty: Ty<'tcx> = tcx.lookup_item_type(impl_id).ty; | ||
if let ty::TyAdt(self_adt_def, self_substs) = impl_self_ty.sty { | ||
assert_eq!(adt_def, self_adt_def); | ||
self_substs | ||
} else { | ||
bug!("Self in `impl Drop for _` must be an Adt."); | ||
} | ||
}; | ||
|
||
// Walk `substs` + `self_substs`, build new substs appropriate for | ||
// `adt_def`; each non-dangling param reuses entry from `substs`. | ||
let substs = Substs::for_item( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Egads. =) I see why you warned me this was kind of gross. But I see the logic in it and don't immediately see a better way to do it. I guess it only makes sense because of the limitations we put onto drop impls in the first place, right? Otherwise things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this technique is crucially relying on details of how we constrain I'll add a comment. |
||
tcx, | ||
adt_def.did, | ||
|def, _| { | ||
let r_orig = substs.region_for_def(def); | ||
let impl_self_orig = self_substs.region_for_def(def); | ||
let r = if let ty::Region::ReEarlyBound(ref ebr) = *impl_self_orig { | ||
if impl_bindings.region_param(ebr).pure_wrt_drop { | ||
tcx.mk_region(ty::ReStatic) | ||
} else { | ||
r_orig | ||
} | ||
} else { | ||
bug!("substs for an impl must map regions to ReEarlyBound"); | ||
}; | ||
debug!("has_dtor_of_interest mapping def {:?} orig {:?} to {:?}", | ||
def, r_orig, r); | ||
r | ||
}, | ||
|def, _| { | ||
let t_orig = substs.type_for_def(def); | ||
let impl_self_orig = self_substs.type_for_def(def); | ||
let t = if let ty::TypeVariants::TyParam(ref pt) = impl_self_orig.sty { | ||
if impl_bindings.type_param(pt).pure_wrt_drop { | ||
tcx.mk_nil() | ||
} else { | ||
t_orig | ||
} | ||
} else { | ||
bug!("substs for an impl must map types to TyParam"); | ||
}; | ||
debug!("has_dtor_of_interest mapping def {:?} orig {:?} {:?} to {:?} {:?}", | ||
def, t_orig, t_orig.sty, t, t.sty); | ||
t | ||
}); | ||
|
||
return tcx.mk_adt(adt_def, &substs); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a comment would be nice here (and below); perhaps including a link to the RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see there are comments on the
ty
data structures