diff --git a/text/0000-sound-generic-drop.md b/text/0000-sound-generic-drop.md new file mode 100644 index 00000000000..34fbab639e6 --- /dev/null +++ b/text/0000-sound-generic-drop.md @@ -0,0 +1,1156 @@ +- Start Date: 2013-08-29 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary + +Remove `#[unsafe_destructor]` from the Rust language. Make it safe +for developers to implement `Drop` on type- and lifetime-parameterized +structs and enum (i.e. "Generic Drop") by imposing new rules on code +where such types occur, to ensure that the drop implementation cannot +possibly read or write data via a reference of type `&'a Data` where +`'a` could have possibly expired before the drop code runs. + +Note: This RFC is describing a feature that has been long in the +making; in particular it was previously sketched in Rust [Issue #8861] +"New Destructor Semantics" (the source of the tongue-in-cheek "Start +Date" given above), and has a [prototype implementation] that is being +prepared to land. The purpose of this RFC is two-fold: + + 1. standalone documentation of the (admittedly conservative) rules + imposed by the new destructor semantics, and + + 2. elicit community feedback on the rules, both in the form they will + take for 1.0 (which is relatively constrained) and the form they + might take in the future (which allows for hypothetical language + extensions). + +[Issue #8861]: https://github.com/rust-lang/rust/issues/8861 + +[prototype implementation]: https://github.com/pnkfelix/rust/tree/77afdb70a1d4d5a20069f12412bfeda3ccd145bf + +# Motivation + +Part of Rust's design is rich use of Resource Acquisition Is +Initialization (RAII) patterns, which requires destructors: code +attached to certain types that runs only when a value of the type goes +out of scope or is otherwise deallocated. In Rust, the `Drop` trait is +used for this purpose. + +Currently (as of Rust 1.0 alpha), a developer cannot implement `Drop` +on a type- or lifetime-parametric type (e.g. `struct Sneetch<'a>` or +`enum Zax`) without attaching the `#[unsafe_destructor]` attribute +to it. The reason this attribute is required is that the current +implementation allows for such destructors to inject unsoundness +accidentally (e.g. reads from or writes to deallocated memory, +accessing data when its representation invariants are no longer +valid). + +Furthermore, while some destructors can be implemented with no danger +of unsoundness, regardless of `T` (assuming that any `Drop` +implementation attached to `T` is itself sound), as soon as one wants +to interact with borrowed data within the `fn drop` code (e.g. access +a field `&'a StarOffMachine` from a value of type `Sneetch<'a>` ), +there is currently no way to enforce a rule that `'a` *strictly* +*outlive* the value itself. This is a huge gap in the language as it +stands: as soon as a developer attaches `#[unsafe_destructor]` to such +a type, it is imposing a subtle and *unchecked* restriction on clients +of that type that they will not ever allow the borrowed data to expire +first. + +## Lifetime parameterization: the Sneetch example +[The Sneetch example]: #lifetime-parameterization-the-sneetch-example + +If today Sylvester writes: + +```rust +// opt-in to the unsoundness! +#![feature(unsafe_destructor)] + +pub mod mcbean { + use std::cell::Cell; + + pub struct StarOffMachine { + usable: bool, + dollars: Cell, + } + + impl Drop for StarOffMachine { + fn drop(&mut self) { + let contents = self.dollars.get(); + println!("Dropping a machine; sending {} dollars to Sylvester.", + contents); + self.dollars.set(0); + self.usable = false; + } + } + + impl StarOffMachine { + pub fn new() -> StarOffMachine { + StarOffMachine { usable: true, dollars: Cell::new(0) } + } + pub fn remove_star(&self, s: &mut Sneetch) { + assert!(self.usable, + "No different than a read of a dangling pointer."); + self.dollars.set(self.dollars.get() + 10); + s.has_star = false; + } + } + + pub struct Sneetch<'a> { + name: &'static str, + has_star: bool, + machine: Cell>, + } + + impl<'a> Sneetch<'a> { + pub fn new(name: &'static str) -> Sneetch<'a> { + Sneetch { + name: name, + has_star: true, + machine: Cell::new(None) + } + } + + pub fn find_machine(&self, m: &'a StarOffMachine) { + self.machine.set(Some(m)); + } + } + + #[unsafe_destructor] + impl<'a> Drop for Sneetch<'a> { + fn drop(&mut self) { + if let Some(m) = self.machine.get() { + println!("{} says ``before I die, I want to join my \ + plain-bellied brethren.''", self.name); + m.remove_star(self); + } + } + } +} + +fn unwary_client() { + use mcbean::{Sneetch, StarOffMachine}; + let (s1, m, s2, s3); // (accommodate PR 21657) + s1 = Sneetch::new("Sneetch One"); + m = StarOffMachine::new(); + s2 = Sneetch::new("Sneetch Two"); + s3 = Sneetch::new("Sneetch Zee"); + + s1.find_machine(&m); + s2.find_machine(&m); + s3.find_machine(&m); +} + +fn main() { + unwary_client(); +} +``` + +This compiles today; if you run it, it prints the following: + +``` +Sneetch Zee says ``before I die, I want to join my plain-bellied brethren.'' +Sneetch Two says ``before I die, I want to join my plain-bellied brethren.'' +Dropping a machine; sending 20 dollars to Sylvester. +Sneetch One says ``before I die, I want to join my plain-bellied brethren.'' +thread '
' panicked at 'No different than a read of a dangling pointer.', :27 +``` + +Explanation: In Sylvester's code, the `Drop` implementation for +`Sneetch` invokes a method on the borrowed reference in the field +`machine`. This implies there is an implicit restriction on an value +`s` of type `Sneetch<'a>`: the lifetime `'a` must *strictly outlive* +`s`. + +(The example encodes this constraint in a dynamically-checked manner +via an explicit `usable` boolean flag that is only set to false in the +machine's own destructor; it is important to keep in mind that this is +just a method to illustrate the violation in a semi-reliable manner: +Using a machine after `usable` is set to false by its `fn drop` code +is analogous to dereferencing a `*mut T` that has been deallocated, or +similar soundness violations.) + +Sylvester's API does not encode the constraint "`'a` must strictly +outlive the `Sneetch<'a>`" explicitly; Rust currently has no way of +expressing the constraint that one lifetime be strictly greater than +another lifetime or type (the form `'a:'b` only formally says that +`'a` must live *at least* as long as `'b`). + +Thus, client code like that in `unwary_client` can inadvertantly set +up scenarios where Sylvester's code may break, and Sylvester might be +completely unaware of the vulnerability. + +## Type parameterization: the problem of trait bounds +[The Zook example]: #type-parameterization-the-problem-of-trait-bounds + +One might think that all instances of this problem can +be identified by the use of a lifetime-parametric `Drop` implementation, +such as `impl<'a> Drop for Sneetch<'a> { ..> }` + +However, consider this trait and struct: + +```rust +trait Button { fn push(&self); } +struct Zook { button: B, } +#[unsafe_destructor] +impl Drop for Zook { + fn drop(&mut self) { self.button.push(); } +} +``` +In this case, it is not obvious that there is anything wrong here. + +But if we continue the example: +```rust +struct Bomb { usable: bool } +impl Drop for Bomb { fn drop(&mut self) { self.usable = false; } } +impl Bomb { fn activate(&self) { assert!(self.usable) } } + +enum B<'a> { HarmlessButton, BigRedButton(&'a Bomb) } +impl<'a> Button for B<'a> { + fn push(&self) { + if let B::BigRedButton(borrowed) = *self { + borrowed.activate(); + } + } +} + +fn main() { + let (mut zook, ticking); + zook = Zook { button: B::HarmlessButton }; + ticking = Bomb { usable: true }; + zook.button = B::BigRedButton(&ticking); +} +``` +Within the `zook` there is a hidden reference to borrowed data, +`ticking`, that is assigned the same lifetime as `zook` but that +will be dropped before `zook` is. + +(These examples may seem contrived; see [Appendix A] for a far less +contrived example, that also illustrates how the use of borrowed data +can lie hidden behind type parameters.) + +## The proposal + +This RFC is proposes to fix this scenario, by having the compiler +ensure that types with destructors are only employed in contexts where +either any borrowed data with lifetime `'a` within the type either +strictly outlives the value of that type, or such borrowed data is +provably not accessible from any `Drop` implementation via a reference +of type `&'a`/`&'a mut`. This is the "Drop-Check" (aka `dropck`) rule. + +# Detailed design + +## The Drop-Check Rule +[The Drop-Check Rule]: #the-drop-check-rule + +The Motivation section alluded to the compiler enforcing a new rule. +Here is a more formal statement of that rule: + +Let `v` be some value (either temporary or named); if the type of `v` owns data of type `D`, +where `D` has a lifetime- or type-parametric `Drop` implementation and +either: + + * (A.) the `Drop impl` for `D` instantiates `D` at `'a` + directly, i.e. `D<'a>`, or, + + * (B.) the `Drop impl` for `D` has some type parameter with a + trait bound `T` where `T` is a trait that has at least + one method, + +then `'a` must strictly outlive the scope of `v`. + +(Note: This rule is using two phrases that deserve further +elaboration and that are discussed further in sections that +follow: ["the type owns data of type `D`"][type-ownership] +and ["must strictly outlive"][strictly-outlives].) + +(Note: When encountering a `D` of the form `Box`, we +conservatively assume that such a type has a `Drop` implementation +parametric in `'b`.) + +This rule allows much sound existing code to compile without complaint +from `rustc`. This is largely due to the fact that many `Drop` +implementations enjoy near-complete parametricity: They tend to not +impose any bounds at all on their type parameters, and thus the rule +does not apply to them. + +At the same time, this rule catches the cases where a destructor could +possibly reference borrowed data via a reference of type `&'a _` or +`&'a mut_`. Here is why: + +Condition (A.) ensures that a type like `Sneetch<'a>` +from [the Sneetch example] will only be +assigned to an expression `s` where `'a` strictly outlives `s`. + +Condition (B.) catches cases like `Zook>` from +[the Zook example], where the destructor's interaction with borrowed +data is hidden behind a method call in the `fn drop`. + +## Near-complete parametricity suffices + +### Noncopy types + +All non-`Copy` type parameters are (still) assumed to have a +destructor. Thus, one would be correct in noting that even a type +`T` with no bounds may still have one hidden method attached; namely, +its `Drop` implementation. + +However, the drop implementation for `T` can only be called when +running the destructor for value `v` if either: + + 1. the type of `v` owns data of type `T`, or + + 2. the destructor of `v` constructs an instance of `T`. + +In the first case, the Drop-Check rule ensures that `T` must satisfy +either Condition (A.) or (B.). In this second case, the freshly +constructed instance of `T` will only be able to access either +borrowed data from `v` itself (and thus such data will already have +lifetime that strictly outlives `v`) or data created during the +execution of the destructor. + +### `Any` instances + +All types implementing `Any` is forced to outlive `'static`. So one +should not be able to hide borrowed data behind the `Any` trait, and +therefore it is okay for the analysis to treat `Any` like a black box +whose destructor is safe to run (at least with respect to not +accessing borrowed data). + +## Strictly outlives +[strictly-outlives]: #strictly-outlives + +There is a notion of "strictly outlives" within the compiler +internals. (This RFC is not adding such a notion to the language +itself; expressing "'a strictly outlives 'b" as an API constraint is +not a strict necessity at this time.) + +The heart of the idea is this: we approximate the notion of "strictly +outlives" by the following rule: if a value `U` needs to strictly +outlive another value `V` with code extent `S`, we could just say that +`U` needs to live at least as long as the parent scope of `S`. + +There are likely to be sound generalizations of the model given here +(and we will likely need to consider such to adopt future extensions +like Single-Entry-Multiple-Exit (SEME) regions, but that is out of +scope for this RFC). + +In terms of its impact on the language, the main change has already +landed in the compiler; see [Rust PR 21657], which added +`CodeExtent::Remainder`, for more direct details on the implications +of that change written in a user-oriented fashion. + +[Rust PR 21657]: https://github.com/rust-lang/rust/pull/21657 + +One important detail of the strictly-outlives relationship +that comes in part from [Rust PR 21657]: +All bindings introduced by a single `let` statement +are modeled as having the *same* lifetime. +In an example like +```rust +let a; +let b; +let (c, d); +... +``` +`a` strictly outlives `b`, and `b` strictly outlives both `c` and `d`. +However, `c` and `d` are modeled as having the same lifetime; neither +one strictly outlives the other. +(Of course, during code execution, one of them will be dropped before +the other; the point is that when `rustc` builds its internal +model of the lifetimes of data, it approximates and assigns them +both the same lifetime.) This is an important detail, +because there are situations where one *must* assign the same +lifetime to two distinct bindings in order to allow them to +mutually refer to each other's data. + +For more details on this "strictly outlives" model, see [Appendix B]. + +## When does one type own another +[type-ownership]: #when-does-one-type-own-another + +The definition of the Drop-Check Rule used the phrase +"if the type owns data of type `D`". + +This criteria is based on recursive descent of the +structure of an input type `E`. + + * If `E` itself has a Drop implementation that satisfies either + condition (A.) or (B.) then add, for all relevant `'a`, + the constraint that `'a` must outlive the scope of + the value that caused the recursive descent. + + * Otherwise, if we have previously seen `E` during the descent + then skip it (i.e. we assume a type has no destructor of interest + until we see evidence saying otherwise). + This check prevents infinite-looping when we + encounter recursive references to a type, which can arise + in e.g. `Option>`. + + * Otherwise, if `E` is a struct (or tuple), for each of the struct's + fields, recurse on the field's type (i.e., a struct owns its + fields). + + * Otherwise, if `E` is an enum, for each of the enum's variants, + and for each field of each variant, recurse on the field's type + (i.e., an enum owns its fields). + + * Otherwise, if `E` is of the form `& T`, `&mut T`, `* T`, or `fn (T, ...) -> T`, + then skip this `E` + (i.e., references, native pointers, and bare functions do not own + the types they refer to). + + * Otherwise, recurse on any immediate type substructure of `E`. + (i.e., an instantiation of a polymorphic type `Poly` is + assumed to own `T_1` and `T_2`; note that structs and enums *do + not* fall into this category, as they are handled up above; but + this does cover cases like `Box+'a>`). + +### Phantom Data + +The above definition for type-ownership is (believed to be) sound for +pure Rust programs that do not use `unsafe`, but it does not suffice +for several important types without some tweaks. + +In particular, consider the implementation of `Vec`: +as of "Rust 1.0 alpha": +```rust +pub struct Vec { + ptr: NonZero<*mut T>, + len: uint, + cap: uint, +} +``` + +According to the above definition, `Vec` does not own `T`. +This is clearly wrong. + +However, it generalizing the rule to say that `*mut T` owns `T` would +be too conservative, since there are cases where one wants to use +`*mut T` to model references to state that are not owned. + +Therefore, we need some sort of marker, so that types like `Vec` +can express that values of that type own instances of `T`. +The `PhantomData` marker proposed by [RFC 738] ("Support variance +for type parameters") is a good match for this. +This RFC assumes that either [RFC 738] will be accepted, +or if necessary, this RFC will be amended so that it +itself adds the concept of `PhantomData` to the language. +Therefore, as an additional special case to the criteria above +for when the type `E` owns data of type `D`, we include: + + * If `E` is `PhantomData`, then recurse on `T`. + +[RFC 738]: https://github.com/rust-lang/rfcs/pull/738 + +## Examples of changes imposed by the Drop-Check Rule + +### Some cyclic structure is still allowed +[Cyclic structure still allowed]: #some-cyclic-structure-is-still-allowed + +Earlier versions of the Drop-Check rule were quite conservative, to +the point where cyclic data would be disallowed in many contexts. +The Drop-Check rule presented in this RFC was crafted to try +to keep many existing useful patterns working. + +In particular, cyclic structure is still allowed in many +contexts. Here is one concrete example: + +```rust +use std::cell::Cell; + +#[derive(Show)] +struct C<'a> { + v: Vec>>>, +} + +impl<'a> C<'a> { + fn new() -> C<'a> { + C { v: Vec::new() } + } +} + +fn f() { + let (mut c1, mut c2, mut c3); + c1 = C::new(); + c2 = C::new(); + c3 = C::new(); + + c1.v.push(Cell::new(None)); + c1.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + + c1.v[0].set(Some(&c2)); + c1.v[1].set(Some(&c3)); + c2.v[0].set(Some(&c2)); + c2.v[1].set(Some(&c3)); + c3.v[0].set(Some(&c1)); + c3.v[1].set(Some(&c2)); +} +``` + +In this code, each of the nodes { `c1`, `c2`, `c3` } contains a +reference to the two other nodes, and those references are stored in a +`Vec`. Note that all of the bindings are introduced by a single +let-statement; this is to accommodate the region inference system +which wants to assign a single code extent to the `'a` lifetime, as +discussed in the [strictly-outlives] section. + +Even though `Vec` itself is defined as implementing `Drop`, +it puts no bounds on `T`, and therefore that `Drop` implementation is +ignored by the Drop-Check rule. + +### Directly mixing cycles and `Drop` is rejected + +[The Sneetch example] illustrates a scenario were borrowed data is +dropped while there is still an outstanding borrow that will be +accessed by a destructor. In that particular example, one can easily +reorder the bindings to ensure that the `StarOffMachine` outlives all +of the sneetches. + +But there are other examples that have no such resolution. In +particular, graph-structured data where the destructor for each node +accesses the neighboring nodes in the graph; this simply cannot be +done soundly, because when there are cycles, there is no legal order in which to drop the nodes. + +(At least, we cannot do it soundly without imperatively removing a +node from the graph as the node is dropped; but we are not going to +attempt to support verifying such an invariant as part of this RFC; to +my knowledge it is not likely to be feasible with type-checking based +static analyses). + +In any case, we can easily show some code that will now start to be +rejected due to the Drop-Check rule: we take the same `C<'a>` example +of cyclic structure given above, but we now attach a `Drop` +implementation to `C<'a>`: + +```rust +use std::cell::Cell; + +#[derive(Show)] +struct C<'a> { + v: Vec>>>, +} + +impl<'a> C<'a> { + fn new() -> C<'a> { + C { v: Vec::new() } + } +} + +// (THIS IS NEW) +impl<'a> Drop for C<'a> { + fn drop(&mut self) { } +} + +fn f() { + let (mut c1, mut c2, mut c3); + c1 = C::new(); + c2 = C::new(); + c3 = C::new(); + + c1.v.push(Cell::new(None)); + c1.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + + c1.v[0].set(Some(&c2)); + c1.v[1].set(Some(&c3)); + c2.v[0].set(Some(&c2)); + c2.v[1].set(Some(&c3)); + c3.v[0].set(Some(&c1)); + c3.v[1].set(Some(&c2)); +} +``` + +Now the addition of `impl<'a> Drop for C<'a>` changes +the results entirely; + +The Drop-Check rule sees the newly added `impl<'a> Drop for C<'a>`, +which means that for every value of type `C<'a>`, `'a` must strictly +outlive the value. But in the binding +`let (mut c1, mut c2, mut c3)` , all three bindings are assigned +the same type `C<'scope_of_c1_c2_and_c3>`, where +`'scope_of_c1_c2_and_c3` does not strictly outlive any of the three. +Therefore this code will be rejected. + +(Note: it is irrelevant that the `Drop` implementation is a no-op +above. The analysis does not care what the contents of that code are; +it solely cares about the public API presented by the type to its +clients. After all, the `Drop` implementation for `C<'a>` could be +rewritten tomorrow to contain code that accesses the neighboring +nodes. + +### Some temporaries need to be given names + +Due to the way that `rustc` implements the [strictly-outlives] +relation in terms of code-extents, the analysis does not know in an +expression like `foo().bar().quux()` in what order the temporary +values `foo()` and `foo().bar()` will be dropped. + +Therefore, the Drop-Check rule sometimes forces one to rewrite the +code so that it is apparent to the compiler that the value from +`foo()` will definitely outlive the value from `foo().bar()`. + +Thus, on occasion one is forced to rewrite: +```rust +let q = foo().bar().quux(); +... +``` + +as: +```rust +let foo = foo(); +let q = foo.bar().quux() +... +``` + +or even sometimes as: +```rust +let foo = foo(); +let bar = foo.bar(); +let q = bar.quux(); +... +``` +depending on the types involved. + +In practice, pnkfelix saw this arise most often +with code like this: + +```rust +for line in old_io::stdin().lock().lines() { + ... +} +``` + +Here, the result of `stdin()` is a `StdinReader`, which holds a +`RaceBox` in a `Mutex` behind an `Arc`. The result of the `lock()` +method is a `StdinReaderGuard<'a>`, which owns a `MutexGuard<'a, +RaceBox>`. The `MutexGuard` has a `Drop` implementation that is +parametric in `'a`; thus, the Drop-Check rule insists that the +lifetime assigned to `'a` strictly outlive the `MutexGuard`. + +So, under this RFC, we rewrite the code like so: +```rust +let stdin = old_io::stdin(); +for line in stdin.lock().lines() { + ... +} +``` + +(pnkfelix acknowledges that this rewrite is unfortunate. Potential +future work would be to further revise the code extent system so that +the compiler knows that the temporary from `stdin()` will outlive the +temporary from `stdin().lock()`. However, such a change to the +code extents could have unexpected fallout, analogous to the +fallout that was associated with [Rust PR 21657].) + +### Mixing acyclic structure and `Drop` is sometimes rejected + +This is an example of sound code, accepted today, that is +unfortunately rejected by the Drop-Check rule (at least in pnkfelix's +prototype): + +```rust +#![feature(unsafe_destructor)] + +use std::cell::Cell; + +#[derive(Show)] +struct C<'a> { + f: Cell>>, +} + +impl<'a> C<'a> { + fn new() -> C<'a> { + C { f: Cell::new(None), } + } +} + +// force dropck to care about C<'a> +#[unsafe_destructor] +impl<'a> Drop for C<'a> { + fn drop(&mut self) { } +} + +fn f() { + let c2; + let mut c1; + + c1 = C::new(); + c2 = C::new(); + + c1.f.set(Some(&c2)); +} + +fn main() { + f(); +} +``` + +In principle this should work, since `c1` and `c2` are assigned to +distinct code extents, and `c1` will be dropped before `c2`. However, +in the prototype, the region inference system is determining that the +lifetime `'a` in `&'a C<'a>` (from the `c1.f.set(Some(&c2));` +statement) needs to cover the whole block, rather than just the block +remainder extent that is actually covered by the `let c2;`. + +(This may just be a bug somewhere in the prototype, but for the time +being pnkfelix is going to assume that it will be a bug that this RFC +is forced to live with indefinitely.) + +## Unsound APIs need to be revised or removed entirely +[Unsound APIs]: #unsound-apis-that-need-to-be-revised-or-removed-entirely + +While the Drop-Check rule is designed to ensure that safe Rust code is +sound in its use of destructors, it cannot assure us that unsafe code +is sound. It is the responsibility of the author of unsafe code to +ensure it does not perform unsound actions; thus, we need to audit our +own API's to ensure that the standard library is not providing +functionality that circumvents the Drop-Check rule. + +The most obvious instance of this is the `arena` crate: in particular: +one can use an instance of `arena::Arena` to create cyclic graph +structure where each node's destructor accesses (via `&_` references) +its neighboring nodes. + +Here is a version of our running `C<'a>` example +(where we now do something interesting the destructor for `C<'a>`) +that demonstrates the problem: + +Example: +```rust +extern crate arena; + +use std::cell::Cell; + +#[derive(Show)] +struct C<'a> { + name: &'static str, + v: Vec>>>, + usable: bool, +} + +impl<'a> Drop for C<'a> { + fn drop(&mut self) { + println!("dropping {}", self.name); + for neighbor in self.v.iter().map(|v|v.get()) { + if let Some(neighbor) = neighbor { + println!(" {} checking neighbor {}", + self.name, neighbor.name); + assert!(neighbor.usable); + } + } + println!("done dropping {}", self.name); + self.usable = false; + + } +} + +impl<'a> C<'a> { + fn new(name: &'static str) -> C<'a> { + C { name: name, v: Vec::new(), usable: true } + } +} + +fn f() { + use arena::Arena; + let arena = Arena::new(); + let (c1, c2, c3); + + c1 = arena.alloc(|| C::new("c1")); + c2 = arena.alloc(|| C::new("c2")); + c3 = arena.alloc(|| C::new("c3")); + + c1.v.push(Cell::new(None)); + c1.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c2.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + c3.v.push(Cell::new(None)); + + c1.v[0].set(Some(c2)); + c1.v[1].set(Some(c3)); + c2.v[0].set(Some(c2)); + c2.v[1].set(Some(c3)); + c3.v[0].set(Some(c1)); + c3.v[1].set(Some(c2)); +} +``` + +Calling `f()` results in the following printout: +``` +dropping c3 + c3 checking neighbor c1 + c3 checking neighbor c2 +done dropping c3 +dropping c1 + c1 checking neighbor c2 + c1 checking neighbor c3 +thread '
' panicked at 'assertion failed: neighbor.usable', ../src/test/compile-fail/dropck_untyped_arena_cycle.rs:19 +``` + +This is unsound. It should not be possible to express such a +scenario without using `unsafe` code. + +This RFC suggests that we revise the `Arena` API by adding a phantom +lifetime parameter to its type, and bound the values the arena +allocates by that phantom lifetime, like so: +```rust +pub struct Arena<'longer_than_self> { + _invariant: marker::InvariantLifetime<'longer_than_self>, + ... +} + +impl<'longer_than_self> Arena<'longer_than_self> { + pub fn alloc(&self, op: F) -> &mut T + where F: FnOnce() -> T { + ... + } +} +``` +Admittedly, this is a severe limitation, since it forces the data +allocated by the Arena to store only references to data that strictly +outlives the arena, regardless of whether the allocated data itself +even has a destructor. (I.e., `Arena` would become much weaker than +`TypedArena` when attempting to work with cyclic structures). +(pnkfelix knows of no way to fix this without adding further extensions +to the language, e.g. some way to express "this type's destructor accesses +none of its borrowed data", which is out of scope for this RFC.) + +Alternatively, we could just deprecate the `Arena` API, (which is not +marked as stable anyway. + +The example given here can be adapted to other kinds of backing +storage structures, in order to double-check whether the API is likely +to be sound or not. For example, the `arena::TypedArena` type +appears to be sound (as long as it carries `PhantomData` just like +`Vec` does). In particular, when one ports the above example to use +`TypedArena` instead of `Arena`, it is statically rejected by `rustc`. + +## The final goal: remove #[unsafe_destructor] + +Once all of the above pieces have landed, lifetime- and +type-parameterized `Drop` will be safe, and thus we will be able to +remove `#[unsafe_destructor]`! + +# Drawbacks + +* The Drop-Check rule is a little complex, and does disallow some + sound code that would compile today. + +* The change proposed in this RFC places restrictions on uses of types + with attached destructors, but provides no way for a type `Foo<'a>` to + state as part of its public interface that its drop implementation + will not read from any borrowed data of lifetime `'a`. (Extending the + language with such a feature is potential future work, but is out of + scope for this RFC.) + +* Some useful interfaces are going to be disallowed by this RFC. + For example, the RFC recommends that the current `arena::Arena` + be revised or simply deprecated, due to its unsoundness. + (If desired, we could add an `UnsafeArena` that continues + to support the current `Arena` API with the caveat that its users need to + *manually* enforce the constraint that the destructors do not access + data that has been already dropped. But again, that decision is out + of scope for this RFC.) + +# Alternatives + +We considered simpler versions of [the Drop-Check rule]; in +particular, an earlier version of it simply said that if the type of +`v` owns any type `D` that implements `Drop`, then for any lifetime +`'a` that `D` refers to, `'a` must strictly outlive the scope of `v`, +because the destructor for `D` might hypothetically access borrowed +data of lifetime `'a`. + + * This rule is simpler in the sense that it more obviously sound. + + * But this rule disallowed far more code; e.g. the [Cyclic structure + still allowed] example was rejected under this more naive rule, + because `C<'a>` owns D = `Vec>>>`, and this + particular D refers to `'a`. + +---- + +Sticking with the current `#[unsafe_destructor]` approach to lifetime- +and type-parametric types that implement `Drop` is not really tenable; +we need to do something (and we have been planning to do something +like this RFC for over a year). + +# Unresolved questions + +* Is the Drop-Check rule provably sound? pnkfelix has based his + argument on informal reasoning about parametricity, but it would be + good to put forth a more formal argument. (And in the meantime, + pnkfelix invites the reader to try to find holes in the rule, + preferably with concrete examples that can be fed into the + prototype.) + +* How much can covariance help with some of the lifetime issues? + + See in particular [Rust Issue 21198] "new scoping rules for safe + dtors may benefit from variance on type params" + +[Rust Issue 21198]: https://github.com/rust-lang/rust/issues/21198 + + Before adding Condition (B.) to [the Drop-Check Rule], it seemed + like enabling covariance in more standard library types was going to + be very important for landing this work. And even now, it is + possible that covariance could still play an important role. + But nonetheless, there are some API's whose current form is fundamentally + incompatible with covariance; e.g. the current `TypedArena` API + is fundamentally invariant with respect to `T`. + +# Appendices + +## Appendix A: Why and when would Drop read from borrowed data +[Appendix A]: #appendix-a-why-and-when-would-drop-read-from-borrowed-data + +Here is a story, about two developers, Julia and Kurt, and the code +they hacked on. + +Julia inherited some code, and it is misbehaving. It appears like +key/value entries that the code inserts into the standard library's +`HashMap` are not always retrievable from the map. Julia's current +hypothesis is that something is causing the keys' computed hash codes +to change dynamically, sometime after the entries have been inserted +into the map (but it is not obvious when or if this change occurs, nor +what its source might be). Julia thinks this hypothesis is plausible, +but does not want to audit all of the key variants for possible causes +of hash code corruption until after she has hard evidence confirming +the hypothesis. + +Julia writes some code that walks a hash map's internals and checks +that all of the keys produce a hash code that is consistent with their +location in the map. However, since it is not clear when the keys' +hash codes are changing, it is not clear where in the overall code +base she should add such checks. (The hash map is sufficiently large +that she cannot simply add calls to do this consistency check +everywhere.) + +However, there is one spot in the control flow that is a clear +contender: if the check is run right before the hash map is dropped, +then that would surely be sometime after the hypothesized corruption +had occurred. In other words, a destructor for the hash map seems +like a good place to start; Julia could make her own local copy of the +hash map library and add this check to a `impl Drop for +HashMap { ... }` implementation. + +In this new destructor code, Julia needs to invoke the hash-code +method on `K`. So she adds the bound `where K: Eq + Hash` to her +`HashMap` and its `Drop` implementation, along with the corresponding +code to walk the table's entries and check that the hash codes for all +the keys matches their position in the table. + +Using this, Julia manages confirms her hypothesis (yay). And since it +was a reasonable amount of effort to do this experiment, she puts this +variation of `HashMap` up on `crates.io`, calling it the +`CheckedHashMap` type. + +Sometime later, Kurt pulls a copy of `CheckHashMap` off of +`crates.io`, and he happens to write some code that looks like this: + +```rust +fn main() { + #[derive(PartialEq, Eq, Hash, Debug)] + struct Key<'a> { name: &'a str } + + { + let (key, mut map, name) : (Key, CheckedHashMap<&Key, String>, String); + name = format!("k1"); + map = CheckedHashMap::new(); + key = Key { name: &*name }; + map.map.insert(&key, format!("Value for k1")); + } +} +``` + +And, kaboom: when the map goes out of scope, the destructor for +`CheckedHashMap` attempts to compute a hashcode on a reference to +`key` that may not still be valid, and even if `key` is still valid, +it holds a reference to a slice of name that likewise may not still be +valid. + +This illustrates a case where one might legitimately mix destructor +code with borrowed data. (Is this example any less contrived than +[the Sneetch example]? That is in the eye of the beholder.) + +## Appendix B: strictly-outlives details +[Appendix B]: #appendix-b-strictly-outlives-details + +The rest of this section gets into some low-level details of parts of +how `rustc` is implemented, largely because the changes described here +do have an impact on what results the `rustc` region inference system +produces (or fails to produce). It serves mostly to explain (1.) why +[Rust PR 21657] was implemented, and (2.) why one may sometimes see +indecipherable region-inference errors. + +### Review: Code Extents + +(Nothing here is meant to be new; its just providing context for the +next subsection.) + +Every Rust expression evaluates to a value `V` that is either placed +into some location with an associated lifetime such as `'l`, or `V` is +associated with a block of code that statically delimits the `V`'s +runtime extent (i.e. we know from the function's text where `V` will +be dropped). In the `rustc` source, the blocks of code are sometimes +called "scopes" and sometimes "code extents"; I will try to stick to +the latter term here, since the word "scope" is terribly overloaded. + +Currently, the code extents in Rust are arranged into a tree hierarchy +structured similarly to the abstract syntax tree; for any given code +extent, the compiler can ask for its parent in this hierarchy. + +Every Rust expression `E` has an associated "terminating extent" +somewhere in its chain of parent code extents; temporary values +created during the execution of `E` are stored at stack locations +managed by `E`'s terminating extent. When we hit the end of the +terminating extent, all such temporaries are dropped. + +An example of a terminating extent: in a let-statement like: +```rust +let = ; +``` +the terminating extent of `` is the let-statement itself. So in +an example like: +```rust +let a1 = input.f().g();` +... +``` +there is a temporary value returned from `input.f()`, and it will live +until the end of the let statement, but not into the subsequent code +represented by `...`. (The value resulting from `input.f().g()`, on +the other hand, will be stored in `a1` and lives until the end of the +block enclosing the let statement.) + +(It is not important to this RFC to know the full set of rules +dictating which parent expressions are deemed terminating extents; we +just will assume that these things do exist.) + +For any given code extent `S`, the parent code extent `P` of `S`, if +it exists, potentially holds bits of code that will execute after `S` +is done. Any cleanup code for any values assigned to `P` will only +run after we have finished with *all* code associated with `S`. + +### A problem with 1.0 alpha code extents + +So, with the above established, we have a hint at how to express that +a lifetime `'a` needs to strictly outlive a particular code extent `S`: +simply say that `'a` needs to live at least long as `P`. + +However, this is a little too simplistic, at least for the Rust +compiler circa Rust 1.0 alpha. The main problem is that all the +bindings established by let statements in a block are assigned the +same code extent. + +This, combined with our simplistic definition, yields real problems. +For example, in: + +```rust +{ + use std::fmt; + #[derive(Debug)] struct DropLoud(&'static str, T); + impl Drop for DropLoud { + fn drop(&mut self) { println!("dropping {}:{:?}", self.0, self.1); } + } + + let c1 = DropLoud("c1", 1); + let c2 = DropLoud("c2", &c1); +} +``` + +In principle, the code above is legal: `c2` will be dropped before +`c1` is, and thus it is okay that `c2` holds a borrowed reference to +`c1` that will be read when `c2` is dropped (indirectly via the +`fmt::Debug` implementation. + +However, with the structure of code extents as of Rust 1.0 alpha, `c1` +and `c2` are both given the same code extent: that of the block +itself. Thus in that context, this definition of "strictly outlives" +indicates that `c1` does *not* strictly outlive `c2`, because `c1` +does not live at least as long as the parent of the block; it only +lives until the end of the block itself. + +This illustrates why "All the bindings established by let statements +in a block are assigned the same code extent" is a problem + +### Block Remainder Code Extents + +The solution proposed here (motivated by experience with the +prototype) is to introduce finer-grained code extents. This solution +is essentially [Rust PR 21657], which has already landed in `rustc`. +(That is in part why this is merely an appendix, rather than part of +the body of the RFC itself.) + +The code extents remain in a tree-hierarchy, but there are now extra +entries in the tree, which provide the foundation for a more precise +"strictly outlives" relation. + +We introduce a new code extent, called a "block remainder" extent, for +every let statement in a block, representing the suffix of the block +covered by the bindings in that let statement. + +For example, given `{ let (a, b) = EXPR_1; let c = EXPR_2; ... }`, +which previously had a code extent structure like: +``` +{ let (a, b) = EXPR_1; let c = EXPR_2; ... } + +----+ +----+ + +------------------+ +-------------+ ++------------------------------------------+ +``` +so the parent extent of each let statement was the whole block. + +But under the new rules, there are two new block remainder extents +introduced, with this structure: + +``` +{ let (a, b) = EXPR_1; let c = EXPR_2; ... } + +----+ +----+ + +------------------+ +-------------+ + +-------------------+ <-- new: block remainder 2 + +------------------------------------------+ <-- new: block remainder 1 ++---------------------------------------------+ +``` + +The first let-statement introduces a block remainder extent that +covers the lifetime for `a` and `b`. The second let-statement +introduces a block remainder extent that covers the lifetime for `c`. + +Each let-statement continues to be the terminating extent for its +initializer expression. But now, the parent of the extent of the +second let statement is a block remainder extent ("block remainder +2"), and, importantly, the parent of block remainder 2 is another +block remainder extent ("block remainder 1"). This way, we precisely +represent the lifetimes of the named values bound by each let +statement, and know that `a` and `b` both strictly outlive `c` +as well as the temporary values created during evaluation of +`EXPR_2`. +Likewise, `c` strictly outlives the bindings and temporaries created +in the `...` that follows it. + +### Why stop at let-statements? + +This RFC does *not* propose that we attempt to go further and track +the order of destruction of the values bound by a *single* let +statement. + +Such an experiment could be made part of future work, but for now, we +just continue to assign `a` and `b` to the same scope; the compiler +does not attempt to reason about what order they will be dropped in, +and thus we cannot for example reference data borrowed from `a` in any +destructor code for `b`. + +The main reason that we do not want to attempt to produce even finer +grain scopes, at least not right now, is that there are scenarios +where it is *important* to be able to assign the same region to two +distinct pieces of data; in particular, this often arises when one +wants to build cyclic structure, as discussed in +[Cyclic structure still allowed].