-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Generalize dropck to ignore item-less traits #24898
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
Changes from all commits
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 |
---|---|---|
|
@@ -450,45 +450,57 @@ fn iterate_over_potentially_unsafe_regions_in_type<'a, 'tcx>( | |
|
||
let dtor_typescheme = ty::lookup_item_type(rcx.tcx(), impl_did); | ||
let dtor_generics = dtor_typescheme.generics; | ||
let dtor_predicates = ty::lookup_predicates(rcx.tcx(), impl_did); | ||
|
||
let has_pred_of_interest = dtor_predicates.predicates.iter().any(|pred| { | ||
// In `impl<T> Drop where ...`, we automatically | ||
// assume some predicate will be meaningful and thus | ||
// represents a type through which we could reach | ||
// borrowed data. However, there can be implicit | ||
// predicates (namely for Sized), and so we still need | ||
// to walk through and filter out those cases. | ||
|
||
let result = match *pred { | ||
ty::Predicate::Trait(ty::Binder(ref t_pred)) => { | ||
let def_id = t_pred.trait_ref.def_id; | ||
match rcx.tcx().lang_items.to_builtin_kind(def_id) { | ||
Some(ty::BoundSend) | | ||
Some(ty::BoundSized) | | ||
Some(ty::BoundCopy) | | ||
Some(ty::BoundSync) => false, | ||
_ => true, | ||
|
||
let mut has_pred_of_interest = false; | ||
|
||
let mut seen_items = Vec::new(); | ||
let mut items_to_inspect = vec![impl_did]; | ||
'items: while let Some(item_def_id) = items_to_inspect.pop() { | ||
if seen_items.contains(&item_def_id) { | ||
continue; | ||
} | ||
|
||
for pred in ty::lookup_predicates(rcx.tcx(), item_def_id).predicates { | ||
let result = match pred { | ||
ty::Predicate::Equate(..) | | ||
ty::Predicate::RegionOutlives(..) | | ||
ty::Predicate::TypeOutlives(..) | | ||
ty::Predicate::Projection(..) => { | ||
// For now, assume all these where-clauses | ||
// may give drop implementation capabilty | ||
// to access borrowed data. | ||
true | ||
} | ||
} | ||
ty::Predicate::Equate(..) | | ||
ty::Predicate::RegionOutlives(..) | | ||
ty::Predicate::TypeOutlives(..) | | ||
ty::Predicate::Projection(..) => { | ||
// we assume all of these where-clauses may | ||
// give the drop implementation the capabilty | ||
// to access borrowed data. | ||
true | ||
} | ||
}; | ||
|
||
if result { | ||
debug!("typ: {} has interesting dtor due to generic preds, e.g. {}", | ||
typ.repr(rcx.tcx()), pred.repr(rcx.tcx())); | ||
ty::Predicate::Trait(ty::Binder(ref t_pred)) => { | ||
let def_id = t_pred.trait_ref.def_id; | ||
if ty::trait_items(rcx.tcx(), def_id).len() != 0 { | ||
// If trait has items, assume it adds | ||
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. This assumption is what you mean by
right? Do you know of circumstances where the assumption is 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. @huonw So, there are (as of this morning?) three kinds of items that traits can define: associated methods, associated types, and associated constants. Its easy for an associated method to access borrowed data and thus violate "drop safety" (the term I'm using for now for violations like that demonstrated in #24895). But for associated types and associated constants, the story is less clear to me. E.g. can one implement an associated constant to be a function that takes So, if associated constants cannot in fact violate drop safety, then that would be an example where this (conservative) assumption is false. But in the long term, it seems like a good idea to just be conservative here and assume that any associated item is a potential drop-safety hole. 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. Hm, I hadn't considered that sort of associated constant example. Interesting. Conservative is definitely fine by me. |
||
// capability to access borrowed data. | ||
true | ||
} else { | ||
// Trait without items is itself | ||
// uninteresting from POV of dropck. | ||
// | ||
// However, may have parent w/ items; | ||
// so schedule checking of predicates, | ||
items_to_inspect.push(def_id); | ||
// and say "no capability found" for now. | ||
false | ||
} | ||
} | ||
}; | ||
|
||
if result { | ||
has_pred_of_interest = true; | ||
debug!("typ: {} has interesting dtor due to generic preds, e.g. {}", | ||
typ.repr(rcx.tcx()), pred.repr(rcx.tcx())); | ||
break 'items; | ||
} | ||
} | ||
|
||
result | ||
}); | ||
seen_items.push(item_def_id); | ||
} | ||
|
||
// In `impl<'a> Drop ...`, we automatically assume | ||
// `'a` is meaningful and thus represents a bound | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT | ||
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. This test has no //~ ERRORs? 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 that's clearly wrong. :) 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. Oh, it seems it does (line 38) and I just can't read... |
||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// Check that child trait who only has items via its *parent* trait | ||
// does cause dropck to inject extra region constraints. | ||
|
||
#![allow(non_camel_case_types)] | ||
|
||
trait Parent { fn foo(&self); } | ||
trait Child: Parent { } | ||
|
||
impl Parent for i32 { fn foo(&self) { } } | ||
impl<'a> Parent for &'a D_Child<i32> { | ||
fn foo(&self) { | ||
println!("accessing child value: {}", self.0); | ||
} | ||
} | ||
|
||
impl Child for i32 { } | ||
impl<'a> Child for &'a D_Child<i32> { } | ||
|
||
struct D_Child<T:Child>(T); | ||
impl <T:Child> Drop for D_Child<T> { fn drop(&mut self) { self.0.foo() } } | ||
|
||
fn f_child() { | ||
// `_d` and `d1` are assigned the *same* lifetime by region inference ... | ||
let (_d, d1); | ||
|
||
d1 = D_Child(1); | ||
// ... we store a reference to `d1` within `_d` ... | ||
_d = D_Child(&d1); //~ ERROR `d1` does not live long enough | ||
|
||
// ... dropck *should* complain, because Drop of _d could (and | ||
// does) access the already dropped `d1` via the `foo` method. | ||
} | ||
|
||
fn main() { | ||
f_child(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// 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 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// Check that traits with various kinds of associated items cause | ||
// dropck to inject extra region constraints. | ||
|
||
#![allow(non_camel_case_types)] | ||
|
||
trait HasSelfMethod { fn m1(&self) { } } | ||
trait HasMethodWithSelfArg { fn m2(x: &Self) { } } | ||
trait HasType { type Something; } | ||
|
||
impl HasSelfMethod for i32 { } | ||
impl HasMethodWithSelfArg for i32 { } | ||
impl HasType for i32 { type Something = (); } | ||
|
||
impl<'a,T> HasSelfMethod for &'a T { } | ||
impl<'a,T> HasMethodWithSelfArg for &'a T { } | ||
impl<'a,T> HasType for &'a T { type Something = (); } | ||
|
||
// e.g. `impl_drop!(Send, D_Send)` expands to: | ||
// ```rust | ||
// struct D_Send<T:Send>(T); | ||
// impl<T:Send> Drop for D_Send<T> { fn drop(&mut self) { } } | ||
// ``` | ||
macro_rules! impl_drop { | ||
($Bound:ident, $Id:ident) => { | ||
struct $Id<T:$Bound>(T); | ||
impl <T:$Bound> Drop for $Id<T> { fn drop(&mut self) { } } | ||
} | ||
} | ||
|
||
impl_drop!{HasSelfMethod, D_HasSelfMethod} | ||
impl_drop!{HasMethodWithSelfArg, D_HasMethodWithSelfArg} | ||
impl_drop!{HasType, D_HasType} | ||
|
||
fn f_sm() { | ||
let (_d, d1); | ||
d1 = D_HasSelfMethod(1); | ||
_d = D_HasSelfMethod(&d1); //~ ERROR `d1` does not live long enough | ||
} | ||
fn f_mwsa() { | ||
let (_d, d1); | ||
d1 = D_HasMethodWithSelfArg(1); | ||
_d = D_HasMethodWithSelfArg(&d1); //~ ERROR `d1` does not live long enough | ||
} | ||
fn f_t() { | ||
let (_d, d1); | ||
d1 = D_HasType(1); | ||
_d = D_HasType(&d1); //~ ERROR `d1` does not live long enough | ||
} | ||
|
||
fn main() { | ||
f_sm(); | ||
f_mwsa(); | ||
f_t(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
// 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 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// Check that item-less traits do not cause dropck to inject extra | ||
// region constraints. | ||
|
||
#![allow(non_camel_case_types)] | ||
|
||
trait UserDefined { } | ||
|
||
impl UserDefined for i32 { } | ||
impl<'a, T> UserDefined for &'a T { } | ||
|
||
// e.g. `impl_drop!(Send, D_Send)` expands to: | ||
// ```rust | ||
// struct D_Send<T:Send>(T); | ||
// impl<T:Send> Drop for D_Send<T> { fn drop(&mut self) { } } | ||
// ``` | ||
macro_rules! impl_drop { | ||
($Bound:ident, $Id:ident) => { | ||
struct $Id<T:$Bound>(T); | ||
impl <T:$Bound> Drop for $Id<T> { fn drop(&mut self) { } } | ||
} | ||
} | ||
|
||
impl_drop!{Send, D_Send} | ||
impl_drop!{Sized, D_Sized} | ||
|
||
// See note below regarding Issue 24895 | ||
// impl_drop!{Copy, D_Copy} | ||
|
||
impl_drop!{Sync, D_Sync} | ||
impl_drop!{UserDefined, D_UserDefined} | ||
|
||
macro_rules! body { | ||
($id:ident) => { { | ||
// `_d` and `d1` are assigned the *same* lifetime by region inference ... | ||
let (_d, d1); | ||
|
||
d1 = $id(1); | ||
// ... we store a reference to `d1` within `_d` ... | ||
_d = $id(&d1); | ||
|
||
// ... a *conservative* dropck will thus complain, because it | ||
// thinks Drop of _d could access the already dropped `d1`. | ||
} } | ||
} | ||
|
||
fn f_send() { body!(D_Send) } | ||
fn f_sized() { body!(D_Sized) } | ||
fn f_sync() { body!(D_Sync) } | ||
|
||
// Issue 24895: Copy: Clone implies `impl<T:Copy> Drop for ...` can | ||
// access a user-defined clone() method, which causes this test case | ||
// to fail. | ||
// | ||
// If 24895 is resolved by removing the `Copy: Clone` relationship, | ||
// then this definition and the call below should be uncommented. If | ||
// it is resolved by deciding to keep the `Copy: Clone` relationship, | ||
// then this comment and the associated bits of code can all be | ||
// removed. | ||
|
||
// fn f_copy() { body!(D_Copy) } | ||
|
||
fn f_userdefined() { body!(D_UserDefined) } | ||
|
||
fn main() { | ||
f_send(); | ||
f_sized(); | ||
// See note above regarding Issue 24895. | ||
// f_copy(); | ||
f_sync(); | ||
f_userdefined(); | ||
} |
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.
Should this be some sort of O(log n)/O(1) lookup data structure, instead of making this loop theoretically O(n^2)?
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.
(i.e. how big is n?)
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.
yeah I had a HashSet here originally when it was traversing every predicate. But when I switched it to being focused on just the set of traits, I became somewhat confident that
n
would not get too large -- since I assume you would be unlikely to have a long chain of parent traits where every trait defined no items.