-
Notifications
You must be signed in to change notification settings - Fork 13.3k
implement RFC 1238: nonparametric dropck. #28861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
bors
merged 16 commits into
rust-lang:master
from
pnkfelix:fsk-nonparam-dropck-issue28498
Oct 10, 2015
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
9868df2
Non-parametric dropck; instead trust an unsafe attribute (RFC 1238).
pnkfelix d778e57
Add RFC 1238's `unsafe_destructor_blind_to_params` (UGEH) where needed.
pnkfelix eea299b
run-pass tests for RFC 1238.
pnkfelix 83077be
compile-fail tests.
pnkfelix 5708f44
shorten URLs to placate `make tidy`.
pnkfelix 7a4743f
review comment: use RFC example for compile-fail/issue28498-reject-ex…
pnkfelix 92da3f9
review comment: reduce the `is_adt_dtorck` method to just a check for…
pnkfelix e2e261f
Added tests illustrating when and when not to use the UGEH attribute …
pnkfelix 7eda5b5
Added tests illustrating when and when not to use the UGEH attribute …
pnkfelix 73f35cf
Added tests illustrating when and when not to use the UGEH attribute …
pnkfelix 9ed5faa
Document the new more conservative dropck rule and the escape hatch.
pnkfelix 61f8def
placate check-pretty via comment rearrangement.
pnkfelix b6a4f03
revise cfail test, removing ugeh attribute that was erroneously cut-a…
pnkfelix e1aba75
review comment: point out that the dropck analysis is now trivial.
pnkfelix 34076bc
Added the param-blindness attribute to `Rc` and `Arc`.
pnkfelix a445f23
review comment: further refinement of comment above `fn is_adt_dtorck`.
pnkfelix File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,13 +115,162 @@ 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: fmt::Display>(T, &'static str); | ||
|
||
impl<T: fmt::Display> Drop for Inspector<T> { | ||
fn drop(&mut self) { | ||
println!("Inspector(_, {}) knows when *not* to inspect.", self.1); | ||
} | ||
} | ||
|
||
fn main() { | ||
let (inspector, days): (Inspector<&u8>, Box<u8>); | ||
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 and trivial; it forces all | ||
borrowed data in a value to outlive that value, which is certainly sound. | ||
|
||
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 | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: pointless use of |
||
struct Inspector<T>(T, &'static str, Box<for <'r> fn(&'r T) -> String>); | ||
|
||
impl<T> Drop for Inspector<T> { | ||
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: fmt::Display>(T, &'static str); | ||
|
||
impl<T: fmt::Display> Drop for Inspector<T> { | ||
fn drop(&mut self) { | ||
// There is a hidden call to `<T as Display>::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. | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't really call the current analysis conservative (even though it is), given that it just doesn't care about the destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... it cares that the Drop impl exists, and is not attempting any sort of parametricity analysis of how the type parameters are used in the type structure.
To me, that consists of a more conservative analysis.
Are you arguing that I should be also pointing out that it is now a trivial analysis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a better way to put it - we require the programmer to specify whether a destructor is safe to call given only destruction-safety.