-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
implement RFC 1238: nonparametric dropck. #28861
implement RFC 1238: nonparametric dropck. #28861
Conversation
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.)
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 rust-lang#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.)
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.
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.
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
r? @arielb1 |
hmm, weird about the travis failure, I thought I ran |
@arielb1 responding to your IRC comments here, since you logged out of IRC AFAICT: arielb1 wrote:
I infer based on your response that your problem here is with
Good idea; will do.
Yeah that had a "bad code smell" to me as well. The reason I left it in:
I am in the midst of investigating the finer-grain attribute now, so that should help inform me as to whether the other code should indeed be deleted |
…1.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.)
@@ -578,6 578,16 @@ impl<'tcx> ty::ctxt<'tcx> { | |||
}); | |||
let generics = adt.type_scheme(self).generics; | |||
|
|||
// RFC 1238: if the destructor method is tagged with the |
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.
This whole method should be !self.has_attr(dtor_method, "unsafe_destructor_blind_to_params")
- unless you have some other plan.
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.
Oh, yes, this code as it stands could be reduced to that.
My intention had originally been to continue returning true
from is_adt_dtorck
if the adt has region params.
Looking now, I see that the RFC directly says in its description of UGEH that the attribute "will allow a destructor to side-step the dropck's constraints." For some reason while writing the RFC I had thought that was only talking about the type parameters (that's why there's the explicit alternative of having UGEH for lifetime parameters).
But at this point, I don't see much reason to stick with my original interpretation. This one (where UGEH causes all constraints to be ignored) is easier to explain and implement.
I would prefer to either have a test that UGEH works with a "real" bound - like my |
Okay I agree with both of @arielb1's points in that last comment:
|
…w.r.t. a trait bound.
…w.r.t. a lifetime param.
…w.r.t. types in negative position.
@arielb1 do you want to review the new tests and docs, or shall I hand that off to someone else? |
It essentially works around it. |
|
||
The precise rules that govern drop checking may be less restrictive in | ||
the future. | ||
|
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.
@gankro: could you take a look at the docs? |
/// 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. |
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.
nit: I used "alive" intentionally - phantom lifetimes are just as important as borrowed pointers here. "Greater than self" is also not entirely accurate - you can totally have a &'a mut MyTrait 'a
(or even a &'a mut TypeWithDestructor<'a>
, if you are willing to use unsafe means to actually create and destroy it), you just can't call its destructor outside 'a
.
Maybe
/// Returns whether this ADT is a dtorck type. If not, it
/// is safe to call this type's destructor even when it does
/// not actually outlive the call, as long as its contents
/// remain destruction-safe.
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.
hmm, well that's better than the previous use of "alive", which I thought was too easy to misinterpret.
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 wrote it before the "outlives" reform.
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 still have some doubts about the wording. E.g. the phrase "even when it does not actually outlive the call [to the destructor]" troubles me, probably because I am having problems choosing a consistent denotation for "it" and "outlives".)
I'll try to come up with something better that still satisfies the constraints you listed above.
A point I want to document, not necessarily in this PR:
|
Is there some logic behind the specific types in the standard library you chose to mark |
Nevermind, didn't see the comment on pnkfelix@d778e57 . Would it make sense to mark Rc/Arc? |
@eefriedman marking Update: I constructed an example. :) I'll add it as a test and put the marker on |
1. Added big comment block explaining the test framework. 2. Added tests exericising Rc and Arc. This was inspired by a comment from eefriedman on PR rust-lang#28861. 3. Made the cycle-detection not issue false-positives on acyclic dags. Doing this efficiently required revising the framework; instead of visiting all children (i.e. doing a traversal), now each test is responsible for supplying the path that will act as a witness to the cycle. Luckily for me, all of the pre-existing tests worked with a trivial path built from "always tke your first left", but new tests I added did require other input paths (i.e., "first turn right, then left". (The path representation is a bit-string and its branches are n-ary, not word phrases and binary branches as you might think from the outline above.)
This was proven necessary after I added `Rc` and `Arc` to the rpass test `dropck_legal_cycles.rs`; see PR rust-lang#28929.
Major revision to the dropck_legal_cycles test. 1. Added big comment block explaining the test framework. 2. Added tests exericising Rc and Arc. This was inspired by a comment from eefriedman on PR #28861. 3. Made the cycle-detection not issue false-positives on acyclic dags. Doing this efficiently required revising the framework; instead of visiting all children (i.e. doing a traversal), now each test is responsible for supplying the path that will act as a witness to the cycle. Luckily for me, all of the pre-existing tests worked with a trivial path built from "always tke your first left", but new tests I added did require other input paths (i.e., "first turn right, then left". (The path representation is a bit-string and its branches are n-ary, not word phrases and binary branches as you might think from the outline above.) cc PR #27185
@bors r |
📌 Commit a445f23 has been approved by |
The code is desugared into use std::cell::Cell;
struct C<'a> { val: Cell<Option<&'a C<'a>>> }
fn main() {
'd: {
let t: (C<'a>, C<'a>);
'a: {
t = (C { val: Cell::new(None) }, C { val: Cell::new(None) });
Cell::set(&t.0.val, Some(&t.1));
Cell::set(&t.1.val, Some(&t.0));
}
del t;
}
} This program is somewhat suspect, because |
…elb1 implement RFC 1238: nonparametric dropck. cc #28498 cc @nikomatsakis
A bit late to the party for that review... Easy to fix in Post. |
@gankro yeah I'll make a follow-up PR with further revisions to nomincon. |
@arielb1 I have a question: Your most recent comment above, that starts with: "The code is desugared into ..." What is the context of that note? Is that text that you think would be useful to add somewhere in the nomicon? Or are you pointing out something about one of the tests (in which case I am not able to infer what test you are referencing). My best guess is that it is an example that you want added to the nomicon, but I am not certain. |
1. Added big comment block explaining the test framework. 2. Added tests exericising Rc and Arc. This was inspired by a comment from eefriedman on PR rust-lang#28861. 3. Made the cycle-detection not issue false-positives on acyclic dags. Doing this efficiently required revising the framework; instead of visiting all children (i.e. doing a traversal), now each test is responsible for supplying the path that will act as a witness to the cycle. Luckily for me, all of the pre-existing tests worked with a trivial path built from "always tke your first left", but new tests I added did require other input paths (i.e., "first turn right, then left". (The path representation is a bit-string and its branches are n-ary, not word phrases and binary branches as you might think from the outline above.)
implement RFC 1238: nonparametric dropck.
cc #28498
cc @nikomatsakis