Skip to content

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 47 additions & 35 deletions src/librustc_typeck/check/dropck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

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)?

Copy link
Member

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?)

Copy link
Member Author

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumption is what you mean by

This also should remove the unsoundness of #24895, but at the cost of injecting the dropck constraints in places where they would not be necessary if we did not have Copy: Clone.

right? Do you know of circumstances where the assumption is false?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Self as an input? I haven't tried yet; I could easily believe that today, no such drop-safety hole can be injected by associated constants.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has no //~ ERRORs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well that's clearly wrong. :)

Copy link
Member

Choose a reason for hiding this comment

The 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();
}
64 changes: 64 additions & 0 deletions src/test/compile-fail/issue-24805-dropck-trait-has-items.rs
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();
}
81 changes: 81 additions & 0 deletions src/test/run-pass/issue-24805-dropck-itemless.rs
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();
}