From 9868df2fd5d9364a1a1d8b22847c7b442a77a88b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 16 Jul 2015 14:56:03 +0200 Subject: [PATCH 01/16] Non-parametric dropck; instead trust an unsafe attribute (RFC 1238). Implement cannot-assume-parametricity (CAP) from RFC 1238, and add the UGEH attribute. ---- Note that we check for the attribute attached to the dtor method, not the Drop impl. (This is just to match the specification of RFC and the tests; I am not wedded to this approach.) --- src/doc/reference.md | 14 ++++++ src/librustc/middle/ty/util.rs | 18 ++++++++ src/librustc_typeck/check/dropck.rs | 71 +++++++++++++++++++---------- src/libsyntax/feature_gate.rs | 9 ++++ 4 files changed, 87 insertions(+), 25 deletions(-) diff --git a/src/doc/reference.md b/src/doc/reference.md index 2fce37eccae..5be180007f9 100644 --- a/src/doc/reference.md +++ b/src/doc/reference.md @@ -2022,6 +2022,20 @@ macro scope. - `simd` - on certain tuple structs, derive the arithmetic operators, which lower to the target's SIMD instructions, if any; the `simd` feature gate is necessary to use this attribute. +- `unsafe_destructor_blind_to_params` - on `Drop::drop` method, asserts that the + destructor code (and all potential specializations of that code) will + never attempt to read from nor write to any references with lifetimes + that come in via generic parameters. This is a constraint we cannot + currently express via the type system, and therefore we rely on the + programmer to assert that it holds. Adding this to a Drop impl causes + the associated destructor to be considered "uninteresting" by the + Drop-Check rule, and thus it can help sidestep data ordering + constraints that would otherwise be introduced by the Drop-Check + rule. Such sidestepping of the constraints, if done incorrectly, can + lead to undefined behavior (in the form of reading or writing to data + outside of its dynamic extent), and thus this attribute has the word + "unsafe" in its name. To use this, the + `unsafe_destructor_blind_to_params` feature gate must be enabled. - `unsafe_no_drop_flag` - on structs, remove the flag that prevents destructors from being run twice. Destructors might be run multiple times on the same object with this attribute. To use this, the `unsafe_no_drop_flag` feature diff --git a/src/librustc/middle/ty/util.rs b/src/librustc/middle/ty/util.rs index b546438f392..43757df3d3d 100644 --- a/src/librustc/middle/ty/util.rs +++ b/src/librustc/middle/ty/util.rs @@ -578,6 +578,16 @@ pub fn is_adt_dtorck(&self, adt: ty::AdtDef<'tcx>) -> bool { }); let generics = adt.type_scheme(self).generics; + // RFC 1238: if the destructor method is tagged with the + // attribute `unsafe_destructor_blind_to_params`, then the + // compiler is being instructed to *assume* that the + // destructor will not access borrowed data via a type + // parameter, even if such data is otherwise reachable. + if self.has_attr(dtor_method, "unsafe_destructor_blind_to_params") { + debug!("typ: {:?} assumed blind and thus is dtorck-safe", adt); + return false; + } + // In `impl<'a> Drop ...`, we automatically assume // `'a` is meaningful and thus represents a bound // through which we could reach borrowed data. @@ -592,6 +602,14 @@ pub fn is_adt_dtorck(&self, adt: ty::AdtDef<'tcx>) -> bool { return true; } + // RFC 1238: *any* type parameter at all makes this a dtor of + // interest (i.e. cannot-assume-parametricity from RFC 1238.) + if generics.has_type_params(subst::TypeSpace) { + debug!("typ: {:?} has interesting dtor due to type params", + adt); + return true; + } + let mut seen_items = Vec::new(); let mut items_to_inspect = vec![impl_did]; while let Some(item_def_id) = items_to_inspect.pop() { diff --git a/src/librustc_typeck/check/dropck.rs b/src/librustc_typeck/check/dropck.rs index 870a81e510e..941fa5f9910 100644 --- a/src/librustc_typeck/check/dropck.rs +++ b/src/librustc_typeck/check/dropck.rs @@ -217,19 +217,16 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>( /// /// ---- /// -/// The Drop Check Rule is the following: +/// The simplified (*) Drop Check Rule is the following: /// /// Let `v` be some value (either temporary or named) and 'a be some /// lifetime (scope). If the type of `v` owns data of type `D`, where /// -/// * (1.) `D` has a lifetime- or type-parametric Drop implementation, and -/// * (2.) the structure of `D` can reach a reference of type `&'a _`, and -/// * (3.) 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, +/// * (1.) `D` has a lifetime- or type-parametric Drop implementation, +/// (where that `Drop` implementation does not opt-out of +/// this check via the `unsafe_destructor_blind_to_params` +/// attribute), and +/// * (2.) the structure of `D` can reach a reference of type `&'a _`, /// /// then 'a must strictly outlive the scope of v. /// @@ -237,6 +234,35 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>( /// /// This function is meant to by applied to the type for every /// expression in the program. +/// +/// ---- +/// +/// (*) The qualifier "simplified" is attached to the above +/// definition of the Drop Check Rule, because it is a simplification +/// of the original Drop Check rule, which attempted to prove that +/// some `Drop` implementations could not possibly access data even if +/// it was technically reachable, due to parametricity. +/// +/// However, (1.) parametricity on its own turned out to be a +/// necessary but insufficient condition, and (2.) future changes to +/// the language are expected to make it impossible to ensure that a +/// `Drop` implementation is actually parametric with respect to any +/// particular type parameter. (In particular, impl specialization is +/// expected to break the needed parametricity property beyond +/// repair.) +/// +/// Therefore we have scaled back Drop-Check to a more conservative +/// rule that does not attempt to deduce whether a `Drop` +/// implementation could not possible access data of a given lifetime; +/// instead Drop-Check now simply assumes that if a destructor has +/// access (direct or indirect) to a lifetime parameter, then that +/// lifetime must be forced to outlive that destructor's dynamic +/// extent. We then provide the `unsafe_destructor_blind_to_params` +/// attribute as a way for destructor implementations to opt-out of +/// this conservative assumption (and thus assume the obligation of +/// ensuring that they do not access data nor invoke methods of +/// values that have been previously dropped). +/// pub fn check_safety_of_destructor_if_necessary<'a, 'tcx>(rcx: &mut Rcx<'a, 'tcx>, typ: ty::Ty<'tcx>, span: Span, @@ -356,13 +382,18 @@ fn iterate_over_potentially_unsafe_regions_in_type<'a, 'b, 'tcx>( // borrowed data reachable via `typ` must outlive the parent // of `scope`. This is handled below. // - // However, there is an important special case: by - // parametricity, any generic type parameters have *no* trait - // bounds in the Drop impl can not be used in any way (apart - // from being dropped), and thus we can treat data borrowed - // via such type parameters remains unreachable. + // However, there is an important special case: for any Drop + // impl that is tagged as "blind" to their parameters, + // we assume that data borrowed via such type parameters + // remains unreachable via that Drop impl. + // + // For example, consider: + // + // ```rust + // #[unsafe_destructor_blind_to_params] + // impl Drop for Vec { ... } + // ``` // - // For example, consider `impl Drop for Vec { ... }`, // which does have to be able to drop instances of `T`, but // otherwise cannot read data from `T`. // @@ -370,16 +401,6 @@ fn iterate_over_potentially_unsafe_regions_in_type<'a, 'b, 'tcx>( // unbounded type parameter `T`, we must resume the recursive // analysis on `T` (since it would be ignored by // type_must_outlive). - // - // FIXME (pnkfelix): Long term, we could be smart and actually - // feed which generic parameters can be ignored *into* `fn - // type_must_outlive` (or some generalization thereof). But - // for the short term, it probably covers most cases of - // interest to just special case Drop impls where: (1.) there - // are no generic lifetime parameters and (2.) *all* generic - // type parameters are unbounded. If both conditions hold, we - // simply skip the `type_must_outlive` call entirely (but - // resume the recursive checking of the type-substructure). if has_dtor_of_interest(tcx, ty) { debug!("iterate_over_potentially_unsafe_regions_in_type \ {}ty: {} - is a dtorck type!", diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 18c6d74d62e..e364716bdf2 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -136,6 +136,10 @@ // switch to Accepted; see RFC 320) ("unsafe_no_drop_flag", "1.0.0", None, Active), + // Allows using the unsafe_destructor_blind_to_params attribute + // (Needs an RFC link) + ("unsafe_destructor_blind_to_params", "1.3.0", Some(28498), Active), + // Allows the use of custom attributes; RFC 572 ("custom_attribute", "1.0.0", None, Active), @@ -339,6 +343,11 @@ enum Status { ("unsafe_no_drop_flag", Whitelisted, Gated("unsafe_no_drop_flag", "unsafe_no_drop_flag has unstable semantics \ and may be removed in the future")), + ("unsafe_destructor_blind_to_params", + Normal, + Gated("unsafe_destructor_blind_to_params", + "unsafe_destructor_blind_to_params has unstable semantics \ + and may be removed in the future")), ("unwind", Whitelisted, Gated("unwind_attributes", "#[unwind] is experimental")), // used in resolve From d778e57bf61b2284f33d28486f2ac63faa0422cf Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 17 Jul 2015 16:12:35 +0200 Subject: [PATCH 02/16] Add RFC 1238's `unsafe_destructor_blind_to_params` (UGEH) where needed. I needed it in `RawVec`, `Vec`, and `TypedArena` for `rustc` to bootstrap; but of course that alone was not sufficient for `make check`. Later I added `unsafe_destructor_blind_to_params` to collections, in particular `LinkedList` and `RawTable` (the backing representation for `HashMap` and `HashSet`), to get the regression tests exercising cyclic structure from PR #27185 building. ---- Note that the feature is `dropck_parametricity` (which is not the same as the attribute's name). We will almost certainly vary our strategy here in the future, so it makes some sense to have a not-as-ugly name for the feature gate. (The attribute name was deliberately selected to be ugly looking.) --- src/liballoc/lib.rs | 5 +++++ src/liballoc/raw_vec.rs | 1 + src/libarena/lib.rs | 7 +++++++ src/libcollections/btree/node.rs | 3 +++ src/libcollections/lib.rs | 6 ++++++ src/libcollections/linked_list.rs | 1 + src/libcollections/vec.rs | 1 + src/libcollections/vec_deque.rs | 1 + src/libstd/collections/hash/table.rs | 1 + src/libstd/lib.rs | 6 ++++++ src/libsyntax/feature_gate.rs | 8 ++++---- src/test/run-pass/issue-24805-dropck-itemless.rs | 7 ++++++- 12 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/liballoc/lib.rs b/src/liballoc/lib.rs index 98c729aaba4..8ecc78a231e 100644 --- a/src/liballoc/lib.rs +++ b/src/liballoc/lib.rs @@ -94,6 +94,11 @@ #![feature(unboxed_closures)] #![feature(unique)] #![feature(unsafe_no_drop_flag, filling_drop)] +// SNAP 1af31d4 +#![allow(unused_features)] +// SNAP 1af31d4 +#![allow(unused_attributes)] +#![feature(dropck_parametricity)] #![feature(unsize)] #![feature(core_slice_ext)] #![feature(core_str_ext)] diff --git a/src/liballoc/raw_vec.rs b/src/liballoc/raw_vec.rs index dd2db6fab08..49d37698154 100644 --- a/src/liballoc/raw_vec.rs +++ b/src/liballoc/raw_vec.rs @@ -445,6 +445,7 @@ pub fn unsafe_no_drop_flag_needs_drop(&self) -> bool { } impl Drop for RawVec { + #[unsafe_destructor_blind_to_params] /// Frees the memory owned by the RawVec *without* trying to Drop its contents. fn drop(&mut self) { let elem_size = mem::size_of::(); diff --git a/src/libarena/lib.rs b/src/libarena/lib.rs index 57e82720e8b..62463ecabbf 100644 --- a/src/libarena/lib.rs +++ b/src/libarena/lib.rs @@ -38,8 +38,14 @@ #![feature(ptr_as_ref)] #![feature(raw)] #![feature(staged_api)] +#![feature(dropck_parametricity)] #![cfg_attr(test, feature(test))] +// SNAP 1af31d4 +#![allow(unused_features)] +// SNAP 1af31d4 +#![allow(unused_attributes)] + extern crate alloc; use std::cell::{Cell, RefCell}; @@ -510,6 +516,7 @@ fn grow(&self) { } impl Drop for TypedArena { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { unsafe { // Determine how much was filled. diff --git a/src/libcollections/btree/node.rs b/src/libcollections/btree/node.rs index 5ac8a698e98..bde0d0e6b5f 100644 --- a/src/libcollections/btree/node.rs +++ b/src/libcollections/btree/node.rs @@ -275,12 +275,14 @@ fn next_back(&mut self) -> Option { } impl Drop for RawItems { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { for _ in self {} } } impl Drop for Node { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { if self.keys.is_null() || (unsafe { self.keys.get() as *const K as usize == mem::POST_DROP_USIZE }) @@ -1419,6 +1421,7 @@ fn next_edge_back(&mut self) -> Option> { } impl Drop for MoveTraversalImpl { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { // We need to cleanup the stored values manually, as the RawItems destructor would run // after our deallocation. diff --git a/src/libcollections/lib.rs b/src/libcollections/lib.rs index 03ee8ba31b1..4292c200fbe 100644 --- a/src/libcollections/lib.rs +++ b/src/libcollections/lib.rs @@ -32,6 +32,11 @@ #![allow(trivial_casts)] #![cfg_attr(test, allow(deprecated))] // rand +// SNAP 1af31d4 +#![allow(unused_features)] +// SNAP 1af31d4 +#![allow(unused_attributes)] + #![feature(alloc)] #![feature(box_patterns)] #![feature(box_syntax)] @@ -59,6 +64,7 @@ #![feature(unboxed_closures)] #![feature(unicode)] #![feature(unique)] +#![feature(dropck_parametricity)] #![feature(unsafe_no_drop_flag, filling_drop)] #![feature(decode_utf16)] #![feature(utf8_error)] diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index 891e8b7b2c9..fca7d3b26fc 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -655,6 +655,7 @@ pub fn split_off(&mut self, at: usize) -> LinkedList { #[stable(feature = "rust1", since = "1.0.0")] impl Drop for LinkedList { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { // Dissolve the linked_list in a loop. // Just dropping the list_head can lead to stack exhaustion diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index bcde523307c..d374c0959f3 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -1385,6 +1385,7 @@ fn cmp(&self, other: &Vec) -> Ordering { #[stable(feature = "rust1", since = "1.0.0")] impl Drop for Vec { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { // NOTE: this is currently abusing the fact that ZSTs can't impl Drop. // Or rather, that impl'ing Drop makes them not zero-sized. This is diff --git a/src/libcollections/vec_deque.rs b/src/libcollections/vec_deque.rs index f7efe9a38df..d438c27a96f 100644 --- a/src/libcollections/vec_deque.rs +++ b/src/libcollections/vec_deque.rs @@ -64,6 +64,7 @@ fn clone(&self) -> VecDeque { #[stable(feature = "rust1", since = "1.0.0")] impl Drop for VecDeque { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { self.clear(); // RawVec handles deallocation diff --git a/src/libstd/collections/hash/table.rs b/src/libstd/collections/hash/table.rs index 32541353a0a..e8796dd10b4 100644 --- a/src/libstd/collections/hash/table.rs +++ b/src/libstd/collections/hash/table.rs @@ -999,6 +999,7 @@ fn clone(&self) -> RawTable { } impl Drop for RawTable { + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { if self.capacity == 0 || self.capacity == mem::POST_DROP_USIZE { return; diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index 10c7190ca0c..9af766ad2af 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -199,6 +199,11 @@ test(no_crate_inject, attr(deny(warnings))), test(attr(allow(dead_code, deprecated, unused_variables, unused_mut))))] +// SNAP 1af31d4 +#![allow(unused_features)] +// SNAP 1af31d4 +#![allow(unused_attributes)] + #![feature(alloc)] #![feature(allow_internal_unstable)] #![feature(associated_consts)] @@ -241,6 +246,7 @@ #![feature(unboxed_closures)] #![feature(unicode)] #![feature(unique)] +#![feature(dropck_parametricity)] #![feature(unsafe_no_drop_flag, filling_drop)] #![feature(decode_utf16)] #![feature(unwind_attributes)] diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index e364716bdf2..e134e5617dc 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -136,9 +136,9 @@ // switch to Accepted; see RFC 320) ("unsafe_no_drop_flag", "1.0.0", None, Active), - // Allows using the unsafe_destructor_blind_to_params attribute - // (Needs an RFC link) - ("unsafe_destructor_blind_to_params", "1.3.0", Some(28498), Active), + // Allows using the unsafe_destructor_blind_to_params attribute; + // RFC 1238 + ("dropck_parametricity", "1.3.0", Some(28498), Active), // Allows the use of custom attributes; RFC 572 ("custom_attribute", "1.0.0", None, Active), @@ -345,7 +345,7 @@ enum Status { and may be removed in the future")), ("unsafe_destructor_blind_to_params", Normal, - Gated("unsafe_destructor_blind_to_params", + Gated("dropck_parametricity", "unsafe_destructor_blind_to_params has unstable semantics \ and may be removed in the future")), ("unwind", Whitelisted, Gated("unwind_attributes", "#[unwind] is experimental")), diff --git a/src/test/run-pass/issue-24805-dropck-itemless.rs b/src/test/run-pass/issue-24805-dropck-itemless.rs index 4512bcc2000..9fa48202298 100644 --- a/src/test/run-pass/issue-24805-dropck-itemless.rs +++ b/src/test/run-pass/issue-24805-dropck-itemless.rs @@ -13,6 +13,8 @@ #![allow(non_camel_case_types)] +#![feature(dropck_parametricity)] + trait UserDefined { } impl UserDefined for i32 { } @@ -26,7 +28,10 @@ impl<'a, T> UserDefined for &'a T { } macro_rules! impl_drop { ($Bound:ident, $Id:ident) => { struct $Id(T); - impl Drop for $Id { fn drop(&mut self) { } } + impl Drop for $Id { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { } + } } } From eea299beaaf5d4a735101a1209d774a2faf01343 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Oct 2015 13:48:55 +0200 Subject: [PATCH 03/16] run-pass tests for RFC 1238. Illustrates cases that worked before and must continue to work, and a case that shows how to use the `unsafe_destructor_blind_to_params` attribute (aka "the UGEH attribute") to work around cannot-assume-parametricity. --- src/test/run-pass/issue28498-must-work-ex1.rs | 26 ++++++++++++++ src/test/run-pass/issue28498-must-work-ex2.rs | 29 +++++++++++++++ src/test/run-pass/issue28498-ugeh-ex1.rs | 35 +++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 src/test/run-pass/issue28498-must-work-ex1.rs create mode 100644 src/test/run-pass/issue28498-must-work-ex2.rs create mode 100644 src/test/run-pass/issue28498-ugeh-ex1.rs diff --git a/src/test/run-pass/issue28498-must-work-ex1.rs b/src/test/run-pass/issue28498-must-work-ex1.rs new file mode 100644 index 00000000000..0c2aafec431 --- /dev/null +++ b/src/test/run-pass/issue28498-must-work-ex1.rs @@ -0,0 +1,26 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Example taken from RFC 1238 text + +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-must-continue-to-work + +use std::cell::Cell; + +struct Concrete<'a>(u32, Cell>>); + +fn main() { + let mut data = Vec::new(); + data.push(Concrete(0, Cell::new(None))); + data.push(Concrete(0, Cell::new(None))); + + data[0].1.set(Some(&data[1])); + data[1].1.set(Some(&data[0])); +} diff --git a/src/test/run-pass/issue28498-must-work-ex2.rs b/src/test/run-pass/issue28498-must-work-ex2.rs new file mode 100644 index 00000000000..5ab2631213f --- /dev/null +++ b/src/test/run-pass/issue28498-must-work-ex2.rs @@ -0,0 +1,29 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Example taken from RFC 1238 text + +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-must-continue-to-work + +use std::cell::Cell; + +struct Concrete<'a>(u32, Cell>>); + +struct Foo { data: Vec } + +fn main() { + let mut foo = Foo { data: Vec::new() }; + foo.data.push(Concrete(0, Cell::new(None))); + foo.data.push(Concrete(0, Cell::new(None))); + + foo.data[0].1.set(Some(&foo.data[1])); + foo.data[1].1.set(Some(&foo.data[0])); +} + diff --git a/src/test/run-pass/issue28498-ugeh-ex1.rs b/src/test/run-pass/issue28498-ugeh-ex1.rs new file mode 100644 index 00000000000..2df86e49b0f --- /dev/null +++ b/src/test/run-pass/issue28498-ugeh-ex1.rs @@ -0,0 +1,35 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Example taken from RFC 1238 text + +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#example-of-the-unguarded-escape-hatch + +#![feature(dropck_parametricity)] +use std::cell::Cell; + +struct Concrete<'a>(u32, Cell>>); + +struct Foo { data: Vec } + +impl Drop for Foo { + #[unsafe_destructor_blind_to_params] // This is the UGEH attribute + fn drop(&mut self) { } +} + +fn main() { + let mut foo = Foo { data: Vec::new() }; + foo.data.push(Concrete(0, Cell::new(None))); + foo.data.push(Concrete(0, Cell::new(None))); + + foo.data[0].1.set(Some(&foo.data[1])); + foo.data[1].1.set(Some(&foo.data[0])); +} + From 83077bee9352cea17758b26aa467022b2fbdc599 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Oct 2015 13:56:36 +0200 Subject: [PATCH 04/16] compile-fail tests. One just checks that we are feature-gating the UGEH attribute (as usual for attributes associated with unstable features). The other is adapted from the RFC 1238 text, except that it has been extended somewhat to actually *illustrate* the scenario that we are trying to prevent, namely observing the state of data, from safe code, after the destructor for that data has been executed. --- .../compile-fail/feature-gate-dropck-ugeh.rs | 39 ++++++++++ .../compile-fail/issue28498-reject-ex1.rs | 74 +++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 src/test/compile-fail/feature-gate-dropck-ugeh.rs create mode 100644 src/test/compile-fail/issue28498-reject-ex1.rs diff --git a/src/test/compile-fail/feature-gate-dropck-ugeh.rs b/src/test/compile-fail/feature-gate-dropck-ugeh.rs new file mode 100644 index 00000000000..a7349e36007 --- /dev/null +++ b/src/test/compile-fail/feature-gate-dropck-ugeh.rs @@ -0,0 +1,39 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Ensure that attempts to use the unsafe attribute are feature-gated. + +// Example adapted from RFC 1238 text (just left out the feature gate). + +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#example-of-the-unguarded-escape-hatch + +// #![feature(dropck_parametricity)] + +use std::cell::Cell; + +struct Concrete<'a>(u32, Cell>>); + +struct Foo { data: Vec } + +impl Drop for Foo { + #[unsafe_destructor_blind_to_params] // This is the UGEH attribute + //~^ ERROR unsafe_destructor_blind_to_params has unstable semantics + fn drop(&mut self) { } +} + +fn main() { + let mut foo = Foo { data: Vec::new() }; + foo.data.push(Concrete(0, Cell::new(None))); + foo.data.push(Concrete(0, Cell::new(None))); + + foo.data[0].1.set(Some(&foo.data[1])); + foo.data[1].1.set(Some(&foo.data[0])); +} + diff --git a/src/test/compile-fail/issue28498-reject-ex1.rs b/src/test/compile-fail/issue28498-reject-ex1.rs new file mode 100644 index 00000000000..1de20411f3a --- /dev/null +++ b/src/test/compile-fail/issue28498-reject-ex1.rs @@ -0,0 +1,74 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Example taken from RFC 1238 text + +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-will-start-to-be-rejected + +// Compare against test/run-pass/issue28498-must-work-ex2.rs + +use std::cell::Cell; + +#[derive(Copy, Clone, Debug)] +enum Validity { Valid, Invalid } +use self::Validity::{Valid, Invalid}; + +struct Abstract { + id: u32, + nbor: Cell>, + valid: Validity, + observe: fn(&Cell>) -> (u32, Validity), +} + +#[derive(Copy, Clone)] +struct Neighbor<'a>(&'a Abstract>); + +fn observe(c: &Cell>) -> (u32, Validity) { + let r = c.get().unwrap().0; + (r.id, r.valid) +} + +impl<'a> Abstract> { + fn new(id: u32) -> Self { + Abstract { + id: id, + nbor: Cell::new(None), + valid: Valid, + observe: observe + } + } +} + +struct Foo { + data: Vec, +} + +impl Drop for Abstract { + fn drop(&mut self) { + let (nbor_id, nbor_valid) = (self.observe)(&self.nbor); + println!("dropping element {} ({:?}), observed neighbor {} ({:?})", + self.id, + self.valid, + nbor_id, + nbor_valid); + self.valid = Invalid; + } +} + +fn main() { + let mut foo: Foo> = Foo { data: Vec::new() }; + foo.data.push(Abstract::new(0)); + foo.data.push(Abstract::new(1)); + + foo.data[0].nbor.set(Some(Neighbor(&foo.data[1]))); + //~^ ERROR `foo.data` does not live long enough + foo.data[1].nbor.set(Some(Neighbor(&foo.data[0]))); + //~^ ERROR `foo.data` does not live long enough +} From 5708f44b368ae8cb5a4fdb1df222b2adef1c3bd0 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Oct 2015 16:51:20 +0200 Subject: [PATCH 05/16] shorten URLs to placate `make tidy`. --- src/test/compile-fail/feature-gate-dropck-ugeh.rs | 3 ++- src/test/run-pass/issue28498-must-work-ex1.rs | 3 ++- src/test/run-pass/issue28498-must-work-ex2.rs | 3 ++- src/test/run-pass/issue28498-ugeh-ex1.rs | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test/compile-fail/feature-gate-dropck-ugeh.rs b/src/test/compile-fail/feature-gate-dropck-ugeh.rs index a7349e36007..aa479a987a3 100644 --- a/src/test/compile-fail/feature-gate-dropck-ugeh.rs +++ b/src/test/compile-fail/feature-gate-dropck-ugeh.rs @@ -12,7 +12,8 @@ // Example adapted from RFC 1238 text (just left out the feature gate). -// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#example-of-the-unguarded-escape-hatch +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md +// #example-of-the-unguarded-escape-hatch // #![feature(dropck_parametricity)] diff --git a/src/test/run-pass/issue28498-must-work-ex1.rs b/src/test/run-pass/issue28498-must-work-ex1.rs index 0c2aafec431..83a978d57b9 100644 --- a/src/test/run-pass/issue28498-must-work-ex1.rs +++ b/src/test/run-pass/issue28498-must-work-ex1.rs @@ -10,7 +10,8 @@ // Example taken from RFC 1238 text -// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-must-continue-to-work +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md +// #examples-of-code-that-must-continue-to-work use std::cell::Cell; diff --git a/src/test/run-pass/issue28498-must-work-ex2.rs b/src/test/run-pass/issue28498-must-work-ex2.rs index 5ab2631213f..93652a1b612 100644 --- a/src/test/run-pass/issue28498-must-work-ex2.rs +++ b/src/test/run-pass/issue28498-must-work-ex2.rs @@ -10,7 +10,8 @@ // Example taken from RFC 1238 text -// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-must-continue-to-work +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md +// #examples-of-code-that-must-continue-to-work use std::cell::Cell; diff --git a/src/test/run-pass/issue28498-ugeh-ex1.rs b/src/test/run-pass/issue28498-ugeh-ex1.rs index 2df86e49b0f..5a6da819039 100644 --- a/src/test/run-pass/issue28498-ugeh-ex1.rs +++ b/src/test/run-pass/issue28498-ugeh-ex1.rs @@ -10,7 +10,8 @@ // Example taken from RFC 1238 text -// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#example-of-the-unguarded-escape-hatch +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md +// #example-of-the-unguarded-escape-hatch #![feature(dropck_parametricity)] use std::cell::Cell; From 7a4743fab02ae9e1cb8523f91997b3f6d8d02518 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Oct 2015 16:54:10 +0200 Subject: [PATCH 06/16] review comment: use RFC example for compile-fail/issue28498-reject-ex1.rs (It is not *exactly* the text from the RFC, but the only thing it adds is a call to a no-op function that is just an attempt to make it clear where the potential for impl specialization comes from.) --- .../compile-fail/issue28498-reject-ex1.rs | 64 ++++++------------- 1 file changed, 19 insertions(+), 45 deletions(-) diff --git a/src/test/compile-fail/issue28498-reject-ex1.rs b/src/test/compile-fail/issue28498-reject-ex1.rs index 1de20411f3a..cee7c57c201 100644 --- a/src/test/compile-fail/issue28498-reject-ex1.rs +++ b/src/test/compile-fail/issue28498-reject-ex1.rs @@ -10,65 +10,39 @@ // Example taken from RFC 1238 text -// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md#examples-of-code-that-will-start-to-be-rejected +// https://github.com/rust-lang/rfcs/blob/master/text/1238-nonparametric-dropck.md +// #examples-of-code-that-will-start-to-be-rejected // Compare against test/run-pass/issue28498-must-work-ex2.rs use std::cell::Cell; -#[derive(Copy, Clone, Debug)] -enum Validity { Valid, Invalid } -use self::Validity::{Valid, Invalid}; +struct Concrete<'a>(u32, Cell>>); -struct Abstract { - id: u32, - nbor: Cell>, - valid: Validity, - observe: fn(&Cell>) -> (u32, Validity), +struct Foo { data: Vec } + +fn potentially_specialized_wrt_t(t: &T) { + // Hypothetical code that does one thing for generic T and then is + // specialized for T == Concrete (and the specialized form can + // then access a reference held in concrete tuple). + // + // (We don't have specialization yet, but we want to allow for it + // in the future.) } -#[derive(Copy, Clone)] -struct Neighbor<'a>(&'a Abstract>); - -fn observe(c: &Cell>) -> (u32, Validity) { - let r = c.get().unwrap().0; - (r.id, r.valid) -} - -impl<'a> Abstract> { - fn new(id: u32) -> Self { - Abstract { - id: id, - nbor: Cell::new(None), - valid: Valid, - observe: observe - } - } -} - -struct Foo { - data: Vec, -} - -impl Drop for Abstract { +impl Drop for Foo { fn drop(&mut self) { - let (nbor_id, nbor_valid) = (self.observe)(&self.nbor); - println!("dropping element {} ({:?}), observed neighbor {} ({:?})", - self.id, - self.valid, - nbor_id, - nbor_valid); - self.valid = Invalid; + potentially_specialized_wrt_t(&self.data[0]) } } fn main() { - let mut foo: Foo> = Foo { data: Vec::new() }; - foo.data.push(Abstract::new(0)); - foo.data.push(Abstract::new(1)); + let mut foo = Foo { data: Vec::new() }; + foo.data.push(Concrete(0, Cell::new(None))); + foo.data.push(Concrete(0, Cell::new(None))); - foo.data[0].nbor.set(Some(Neighbor(&foo.data[1]))); + foo.data[0].1.set(Some(&foo.data[1])); //~^ ERROR `foo.data` does not live long enough - foo.data[1].nbor.set(Some(Neighbor(&foo.data[0]))); + foo.data[1].1.set(Some(&foo.data[0])); //~^ ERROR `foo.data` does not live long enough } From 92da3f9464085bcf73221291bed20d410dee4a32 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 7 Oct 2015 13:17:12 +0200 Subject: [PATCH 07/16] review comment: reduce the `is_adt_dtorck` method to just a check for the attribute. --- src/librustc/middle/ty/util.rs | 95 ++++------------------------------ 1 file changed, 9 insertions(+), 86 deletions(-) diff --git a/src/librustc/middle/ty/util.rs b/src/librustc/middle/ty/util.rs index 43757df3d3d..3d1eb66287f 100644 --- a/src/librustc/middle/ty/util.rs +++ b/src/librustc/middle/ty/util.rs @@ -566,102 +566,25 @@ macro_rules! hash { ($e:expr) => { $e.hash(state) } } } } - /// Returns true if this ADT is a dtorck type, i.e. whether it being - /// safe for destruction requires it to be alive + /// Returns true if this ADT is a dtorck type, i.e. whether it + /// being safe for destruction requires all borrowed pointers + /// reachable by it to have lifetimes strictly greater than self. pub fn is_adt_dtorck(&self, adt: ty::AdtDef<'tcx>) -> bool { let dtor_method = match adt.destructor() { Some(dtor) => dtor, None => return false }; - let impl_did = self.impl_of_method(dtor_method).unwrap_or_else(|| { - self.sess.bug(&format!("no Drop impl for the dtor of `{:?}`", adt)) - }); - let generics = adt.type_scheme(self).generics; // RFC 1238: if the destructor method is tagged with the // attribute `unsafe_destructor_blind_to_params`, then the // compiler is being instructed to *assume* that the - // destructor will not access borrowed data via a type - // parameter, even if such data is otherwise reachable. - if self.has_attr(dtor_method, "unsafe_destructor_blind_to_params") { - debug!("typ: {:?} assumed blind and thus is dtorck-safe", adt); - return false; - } - - // In `impl<'a> Drop ...`, we automatically assume - // `'a` is meaningful and thus represents a bound - // through which we could reach borrowed data. + // destructor will not access borrowed data, + // even if such data is otherwise reachable. // - // FIXME (pnkfelix): In the future it would be good to - // extend the language to allow the user to express, - // in the impl signature, that a lifetime is not - // actually used (something like `where 'a: ?Live`). - if generics.has_region_params(subst::TypeSpace) { - debug!("typ: {:?} has interesting dtor due to region params", - adt); - return true; - } - - // RFC 1238: *any* type parameter at all makes this a dtor of - // interest (i.e. cannot-assume-parametricity from RFC 1238.) - if generics.has_type_params(subst::TypeSpace) { - debug!("typ: {:?} has interesting dtor due to type params", - adt); - return true; - } - - let mut seen_items = Vec::new(); - let mut items_to_inspect = vec![impl_did]; - while let Some(item_def_id) = items_to_inspect.pop() { - if seen_items.contains(&item_def_id) { - continue; - } - - for pred in self.lookup_predicates(item_def_id).predicates { - let result = match pred { - ty::Predicate::Equate(..) | - ty::Predicate::RegionOutlives(..) | - ty::Predicate::TypeOutlives(..) | - ty::Predicate::WellFormed(..) | - ty::Predicate::ObjectSafe(..) | - ty::Predicate::Projection(..) => { - // For now, assume all these where-clauses - // may give drop implementation capabilty - // to access borrowed data. - true - } - - ty::Predicate::Trait(ty::Binder(ref t_pred)) => { - let def_id = t_pred.trait_ref.def_id; - if self.trait_items(def_id).len() != 0 { - // If trait has items, assume it adds - // 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 { - debug!("typ: {:?} has interesting dtor due to generic preds, e.g. {:?}", - adt, pred); - return true; - } - } - - seen_items.push(item_def_id); - } - - debug!("typ: {:?} is dtorck-safe", adt); - false + // Such access can be in plain sight (e.g. dereferencing + // `*foo.0` of `Foo<'a>(&'a u32)`) or indirectly hidden + // (e.g. calling `foo.0.clone()` of `Foo`). + return !self.has_attr(dtor_method, "unsafe_destructor_blind_to_params"); } } From e2e261f376b4d9dad7e9dd891da4865afd613303 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 8 Oct 2015 14:12:26 +0200 Subject: [PATCH 08/16] Added tests illustrating when and when not to use the UGEH attribute w.r.t. a trait bound. --- .../issue28498-reject-trait-bound.rs | 50 ++++++++++++++++++ .../issue28498-ugeh-with-trait-bound.rs | 51 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 src/test/compile-fail/issue28498-reject-trait-bound.rs create mode 100644 src/test/run-pass/issue28498-ugeh-with-trait-bound.rs diff --git a/src/test/compile-fail/issue28498-reject-trait-bound.rs b/src/test/compile-fail/issue28498-reject-trait-bound.rs new file mode 100644 index 00000000000..3904d68ba19 --- /dev/null +++ b/src/test/compile-fail/issue28498-reject-trait-bound.rs @@ -0,0 +1,50 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate that having a trait bound causes dropck to reject code +// that might indirectly access previously dropped value. +// +// Compare with run-pass/issue28498-ugeh-with-trait-bound.rs + +use std::fmt; + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo(u32, T); + +impl Drop for Foo { + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is unsound, + // because we access `T` fmt method when we pass `self.1` + // below, and thus potentially read from borrowed data. + println!("Dropping Foo({}, {:?})", self.0, self.1); + } +} + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped); + //~^ ERROR `last_dropped` does not live long enough + foo1 = Foo(1, &first_dropped); + //~^ ERROR `first_dropped` does not live long enough + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} diff --git a/src/test/run-pass/issue28498-ugeh-with-trait-bound.rs b/src/test/run-pass/issue28498-ugeh-with-trait-bound.rs new file mode 100644 index 00000000000..2e9633b3a21 --- /dev/null +++ b/src/test/run-pass/issue28498-ugeh-with-trait-bound.rs @@ -0,0 +1,51 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate the use of the unguarded escape hatch with a trait bound +// to assert that destructor will not access any dead data. +// +// Compare with compile-fail/issue28498-reject-trait-bound.rs + +#![feature(dropck_parametricity)] + +use std::fmt; + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo(u32, T); + +impl Drop for Foo { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is sound, + // because destructor never accesses the `Debug::fmt` method + // of `T`, despite having it available. + println!("Dropping Foo({}, _)", self.0); + } +} + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped); + foo1 = Foo(1, &first_dropped); + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} From 7eda5b508f14a262ffd6aabee631b5e39ce45874 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 8 Oct 2015 14:19:37 +0200 Subject: [PATCH 09/16] Added tests illustrating when and when not to use the UGEH attribute w.r.t. a lifetime param. --- .../issue28498-reject-lifetime-param.rs | 48 +++++++++++++++++++ .../issue28498-ugeh-with-lifetime-param.rs | 48 +++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 src/test/compile-fail/issue28498-reject-lifetime-param.rs create mode 100644 src/test/run-pass/issue28498-ugeh-with-lifetime-param.rs diff --git a/src/test/compile-fail/issue28498-reject-lifetime-param.rs b/src/test/compile-fail/issue28498-reject-lifetime-param.rs new file mode 100644 index 00000000000..92028c7a818 --- /dev/null +++ b/src/test/compile-fail/issue28498-reject-lifetime-param.rs @@ -0,0 +1,48 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate that having a lifetime param causes dropck to reject code +// that might indirectly access previously dropped value. +// +// Compare with run-pass/issue28498-ugeh-with-lifetime-param.rs + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo<'a>(u32, &'a ScribbleOnDrop); + +impl<'a> Drop for Foo<'a> { + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is unsound, + // because destructor accesses borrowed data in `self.1` + // and we must force that to strictly outlive `self`. + println!("Dropping Foo({}, {:?})", self.0, self.1); + } +} + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped); + //~^ ERROR `last_dropped` does not live long enough + foo1 = Foo(1, &first_dropped); + //~^ ERROR `first_dropped` does not live long enough + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} diff --git a/src/test/run-pass/issue28498-ugeh-with-lifetime-param.rs b/src/test/run-pass/issue28498-ugeh-with-lifetime-param.rs new file mode 100644 index 00000000000..19be2568e4f --- /dev/null +++ b/src/test/run-pass/issue28498-ugeh-with-lifetime-param.rs @@ -0,0 +1,48 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate the use of the unguarded escape hatch with a lifetime param +// to assert that destructor will not access any dead data. +// +// Compare with compile-fail/issue28498-reject-lifetime-param.rs + +#![feature(dropck_parametricity)] + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo<'a>(u32, &'a ScribbleOnDrop); + +impl<'a> Drop for Foo<'a> { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is sound, + // because destructor never accesses `self.1`. + println!("Dropping Foo({}, _)", self.0); + } +} + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped); + foo1 = Foo(1, &first_dropped); + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} From 73f35cfb240667518c8636fb8c8ca2a9679c4f2b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 8 Oct 2015 14:32:36 +0200 Subject: [PATCH 10/16] Added tests illustrating when and when not to use the UGEH attribute w.r.t. types in negative position. --- .../issue28498-reject-passed-to-fn.rs | 51 +++++++++++++++++ .../issue28498-ugeh-with-passed-to-fn.rs | 56 +++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 src/test/compile-fail/issue28498-reject-passed-to-fn.rs create mode 100644 src/test/run-pass/issue28498-ugeh-with-passed-to-fn.rs diff --git a/src/test/compile-fail/issue28498-reject-passed-to-fn.rs b/src/test/compile-fail/issue28498-reject-passed-to-fn.rs new file mode 100644 index 00000000000..89e6d68567d --- /dev/null +++ b/src/test/compile-fail/issue28498-reject-passed-to-fn.rs @@ -0,0 +1,51 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate that a type param in negative position causes dropck to reject code +// that might indirectly access previously dropped value. +// +// Compare with run-pass/issue28498-ugeh-with-passed-to-fn.rs + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo(u32, T, Box fn(&'r T) -> String>); + +impl Drop for Foo { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is unsound, + // because we pass `T` to the callback in `self.2` + // below, and thus potentially read from borrowed data. + println!("Dropping Foo({}, {})", self.0, (self.2)(&self.1)); + } +} + +fn callback(s: & &ScribbleOnDrop) -> String { format!("{:?}", s) } + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped, Box::new(callback)); + //~^ ERROR `last_dropped` does not live long enough + foo1 = Foo(1, &first_dropped, Box::new(callback)); + //~^ ERROR `first_dropped` does not live long enough + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} diff --git a/src/test/run-pass/issue28498-ugeh-with-passed-to-fn.rs b/src/test/run-pass/issue28498-ugeh-with-passed-to-fn.rs new file mode 100644 index 00000000000..bb430ea3e33 --- /dev/null +++ b/src/test/run-pass/issue28498-ugeh-with-passed-to-fn.rs @@ -0,0 +1,56 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Demonstrate the use of the unguarded escape hatch with a type param in negative position +// to assert that destructor will not access any dead data. +// +// Compare with compile-fail/issue28498-reject-lifetime-param.rs + +// Demonstrate that a type param in negative position causes dropck to reject code +// that might indirectly access previously dropped value. +// +// Compare with run-pass/issue28498-ugeh-with-passed-to-fn.rs + +#![feature(dropck_parametricity)] + +#[derive(Debug)] +struct ScribbleOnDrop(String); + +impl Drop for ScribbleOnDrop { + fn drop(&mut self) { + self.0 = format!("DROPPED"); + } +} + +struct Foo(u32, T, Box fn(&'r T) -> String>); + +impl Drop for Foo { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { + // Use of `unsafe_destructor_blind_to_params` is sound, + // because destructor never passes a `self.1` to the callback + // (in `self.2`) despite having it available. + println!("Dropping Foo({}, _)", self.0); + } +} + +fn callback(s: & &ScribbleOnDrop) -> String { format!("{:?}", s) } + +fn main() { + let (last_dropped, foo0); + let (foo1, first_dropped); + + last_dropped = ScribbleOnDrop(format!("last")); + first_dropped = ScribbleOnDrop(format!("first")); + foo0 = Foo(0, &last_dropped, Box::new(callback)); + foo1 = Foo(1, &first_dropped, Box::new(callback)); + + println!("foo0.1: {:?} foo1.1: {:?}", foo0.1, foo1.1); +} From 9ed5faa9d58e37b9c8181fc92349866e7b9598b4 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 8 Oct 2015 15:29:09 +0200 Subject: [PATCH 11/16] Document the new more conservative dropck rule and the escape hatch. --- src/doc/nomicon/dropck.md | 157 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 152 insertions(+), 5 deletions(-) diff --git a/src/doc/nomicon/dropck.md b/src/doc/nomicon/dropck.md index 98d269ff762..5b83433ba3e 100644 --- a/src/doc/nomicon/dropck.md +++ b/src/doc/nomicon/dropck.md @@ -115,13 +115,160 @@ section: **For a generic type to soundly implement drop, its generics arguments must strictly outlive it.** -This rule is sufficient but not necessary to satisfy the drop checker. That is, -if your type obeys this rule then it's definitely sound to drop. However -there are special cases where you can fail to satisfy this, but still -successfully pass the borrow checker. These are the precise rules that are -currently up in the air. +Obeying this rule is (usually) necessary to satisfy the borrow +checker; obeying it is sufficient but not necessary to be +sound. That is, if your type obeys this rule then it's definitely +sound to drop. + +The reason that it is not always necessary to satisfy the above rule +is that some Drop implementations will not access borrowed data even +though their type gives them the capability for such access. + +For example, this variant of the above `Inspector` example will never +accessed borrowed data: + +```rust,ignore +struct Inspector<'a>(&'a u8, &'static str); + +impl<'a> Drop for Inspector<'a> { + fn drop(&mut self) { + println!("Inspector(_, {}) knows when *not* to inspect.", self.1); + } +} + +fn main() { + let (inspector, days); + days = Box::new(1); + inspector = Inspector(&days, "gadget"); + // Let's say `days` happens to get dropped first. + // Even when Inspector is dropped, its destructor will not access the + // borrowed `days`. +} +``` + +Likewise, this variant will also never access borrowed data: + +```rust,ignore +use std::fmt; + +struct Inspector(T, &'static str); + +impl Drop for Inspector { + fn drop(&mut self) { + println!("Inspector(_, {}) knows when *not* to inspect.", self.1); + } +} + +fn main() { + let (inspector, days): (Inspector<&u8>, Box); + days = Box::new(1); + inspector = Inspector(&days, "gadget"); + // Let's say `days` happens to get dropped first. + // Even when Inspector is dropped, its destructor will not access the + // borrowed `days`. +} +``` + +However, *both* of the above variants are rejected by the borrow +checker during the analysis of `fn main`, saying that `days` does not +live long enough. + +The reason is that the borrow checking analysis of `main` does not +know about the internals of each Inspector's Drop implementation. As +far as the borrow checker knows while it is analyzing `main`, the body +of an inspector's destructor might access that borrowed data. + +Therefore, the drop checker forces all borrowed data in a value to +strictly outlive that value. + +# An Escape Hatch + +The precise rules that govern drop checking may be less restrictive in +the future. + +The current analysis is deliberately conservative; forcing all +borrowed data in a value to outlive that value is certainly sound. + +Future versions of the language may improve its precision (i.e. to +reduce the number of cases where sound code is rejected as unsafe). + +In the meantime, there is an unstable attribute that one can use to +assert (unsafely) that a generic type's destructor is *guaranteed* to +not access any expired data, even if its type gives it the capability +to do so. + +That attribute is called `unsafe_destructor_blind_to_params`. +To deploy it on the Inspector example from above, we would write: + +```rust,ignore +struct Inspector<'a>(&'a u8, &'static str); + +impl<'a> Drop for Inspector<'a> { + #[unsafe_destructor_blind_to_params] + fn drop(&mut self) { + println!("Inspector(_, {}) knows when *not* to inspect.", self.1); + } +} +``` + +This attribute has the word `unsafe` in it because the compiler is not +checking the implicit assertion that no potentially expired data +(e.g. `self.0` above) is accessed. + +It is sometimes obvious that no such access can occur, like the case above. +However, when dealing with a generic type parameter, such access can +occur indirectly. Examples of such indirect access are: + * invoking a callback, + * via a trait method call. + +(Future changes to the language, such as impl specialization, may add +other avenues for such indirect access.) + +Here is an example of invoking a callback: + +```rust,ignore +struct Inspector(T, &'static str, Box fn(&'r T) -> String>); + +impl Drop for Inspector { + fn drop(&mut self) { + // The `self.2` call could access a borrow e.g. if `T` is `&'a _`. + println!("Inspector({}, {}) unwittingly inspects expired data.", + (self.2)(&self.0), self.1); + } +} +``` + +Here is an example of a trait method call: + +```rust,ignore +use std::fmt; + +struct Inspector(T, &'static str); + +impl Drop for Inspector { + fn drop(&mut self) { + // There is a hidden call to `::fmt` below, which + // could access a borrow e.g. if `T` is `&'a _` + println!("Inspector({}, {}) unwittingly inspects expired data.", + self.0, self.1); + } +} +``` + +And of course, all of these accesses could be further hidden within +some other method invoked by the destructor, rather than being written +directly within it. + +In all of the above cases where the `&'a u8` is accessed in the +destructor, adding the `#[unsafe_destructor_blind_to_params]` +attribute makes the type vulnerable to misuse that the borrower +checker will not catch, inviting havoc. It is better to avoid adding +the attribute. + +# Is that all about drop checker? It turns out that when writing unsafe code, we generally don't need to worry at all about doing the right thing for the drop checker. However there is one special case that you need to worry about, which we will look at in the next section. + From 61f8def63a43631cdc36568c593043b831425e68 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 8 Oct 2015 23:02:42 +0200 Subject: [PATCH 12/16] placate check-pretty via comment rearrangement. --- src/test/run-pass/issue28498-ugeh-ex1.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/run-pass/issue28498-ugeh-ex1.rs b/src/test/run-pass/issue28498-ugeh-ex1.rs index 5a6da819039..b07831f552c 100644 --- a/src/test/run-pass/issue28498-ugeh-ex1.rs +++ b/src/test/run-pass/issue28498-ugeh-ex1.rs @@ -21,7 +21,8 @@ struct Foo { data: Vec } impl Drop for Foo { - #[unsafe_destructor_blind_to_params] // This is the UGEH attribute + // Below is the UGEH attribute + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { } } From b6a4f0393bfc76f68aa3c9a5b86dd1f7aee5c08c Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 9 Oct 2015 13:30:08 +0200 Subject: [PATCH 13/16] revise cfail test, removing ugeh attribute that was erroneously cut-and-pasted into it. --- src/test/compile-fail/issue28498-reject-passed-to-fn.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/compile-fail/issue28498-reject-passed-to-fn.rs b/src/test/compile-fail/issue28498-reject-passed-to-fn.rs index 89e6d68567d..27378b1e0be 100644 --- a/src/test/compile-fail/issue28498-reject-passed-to-fn.rs +++ b/src/test/compile-fail/issue28498-reject-passed-to-fn.rs @@ -25,7 +25,6 @@ fn drop(&mut self) { struct Foo(u32, T, Box fn(&'r T) -> String>); impl Drop for Foo { - #[unsafe_destructor_blind_to_params] fn drop(&mut self) { // Use of `unsafe_destructor_blind_to_params` is unsound, // because we pass `T` to the callback in `self.2` From e1aba75a978b7a228c2e5c413e2262e24b74141f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 9 Oct 2015 14:08:06 +0200 Subject: [PATCH 14/16] review comment: point out that the dropck analysis is now trivial. --- src/doc/nomicon/dropck.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/doc/nomicon/dropck.md b/src/doc/nomicon/dropck.md index 5b83433ba3e..7c097c91266 100644 --- a/src/doc/nomicon/dropck.md +++ b/src/doc/nomicon/dropck.md @@ -186,11 +186,13 @@ strictly outlive that value. The precise rules that govern drop checking may be less restrictive in the future. -The current analysis is deliberately conservative; forcing all -borrowed data in a value to outlive that value is certainly sound. +The current analysis is deliberately conservative and trivial; it forces all +borrowed data in a value to outlive that value, which is certainly sound. -Future versions of the language may improve its precision (i.e. to -reduce the number of cases where sound code is rejected as unsafe). +Future versions of the language may make the analysis more precise, to +reduce the number of cases where sound code is rejected as unsafe. +This would help address cases such as the two Inspectors above that +know not to inspect during destruction. In the meantime, there is an unstable attribute that one can use to assert (unsafely) that a generic type's destructor is *guaranteed* to From 34076bc0c9fb9ee718e1cebc407547eef730a080 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 9 Oct 2015 18:03:31 +0200 Subject: [PATCH 15/16] Added the param-blindness attribute to `Rc` and `Arc`. This was proven necessary after I added `Rc` and `Arc` to the rpass test `dropck_legal_cycles.rs`; see PR #28929. --- src/liballoc/arc.rs | 1 + src/liballoc/rc.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/liballoc/arc.rs b/src/liballoc/arc.rs index ceca44fc1ac..393138aca09 100644 --- a/src/liballoc/arc.rs +++ b/src/liballoc/arc.rs @@ -550,6 +550,7 @@ impl Drop for Arc { /// /// } // implicit drop /// ``` + #[unsafe_destructor_blind_to_params] #[inline] fn drop(&mut self) { // This structure has #[unsafe_no_drop_flag], so this drop glue may run diff --git a/src/liballoc/rc.rs b/src/liballoc/rc.rs index 3507f123a6f..c207996a8b6 100644 --- a/src/liballoc/rc.rs +++ b/src/liballoc/rc.rs @@ -451,6 +451,7 @@ impl Drop for Rc { /// /// } // implicit drop /// ``` + #[unsafe_destructor_blind_to_params] fn drop(&mut self) { unsafe { let ptr = *self._ptr; From a445f234a64cdbbf946af012b94dccf91447b5ff Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 9 Oct 2015 18:42:14 +0200 Subject: [PATCH 16/16] review comment: further refinement of comment above `fn is_adt_dtorck`. --- src/librustc/middle/ty/util.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/librustc/middle/ty/util.rs b/src/librustc/middle/ty/util.rs index 3d1eb66287f..a8f7a4db133 100644 --- a/src/librustc/middle/ty/util.rs +++ b/src/librustc/middle/ty/util.rs @@ -566,9 +566,18 @@ macro_rules! hash { ($e:expr) => { $e.hash(state) } } } } - /// Returns true if this ADT is a dtorck type, i.e. whether it - /// being safe for destruction requires all borrowed pointers - /// reachable by it to have lifetimes strictly greater than self. + /// Returns true if this ADT is a dtorck type. + /// + /// Invoking the destructor of a dtorck type during usual cleanup + /// (e.g. the glue emitted for stack unwinding) requires all + /// lifetimes in the type-structure of `adt` to strictly outlive + /// the adt value itself. + /// + /// If `adt` is not dtorck, then the adt's destructor can be + /// invoked even when there are lifetimes in the type-structure of + /// `adt` that do not strictly outlive the adt value itself. + /// (This allows programs to make cyclic structures without + /// resorting to unasfe means; see RFCs 769 and 1238). pub fn is_adt_dtorck(&self, adt: ty::AdtDef<'tcx>) -> bool { let dtor_method = match adt.destructor() { Some(dtor) => dtor,