implement RFC 1238: nonparametric dropck. #28861

pnkfelix · 2015-10-06T12:18:07Z

implement RFC 1238: nonparametric dropck.

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.

rust-highfive · 2015-10-06T12:18:20Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2015-10-06T12:18:25Z

r? @arielb1

pnkfelix · 2015-10-06T14:26:25Z

hmm, weird about the travis failure, I thought I ran make check but maybe I only ran make check-rpass check-cfail on this go-round.

pnkfelix · 2015-10-06T14:40:11Z

@arielb1 responding to your IRC comments here, since you logged out of IRC AFAICT:

arielb1 wrote:

pnkfelix: I don't like your test
to gh28861
it exploits a soundness bug

preventing that and dropck-specialization is different
from what the test is trying to show
I would have a trivial container
with a destructor

I infer based on your response that your problem here is with compile-fail/issue28498-reject-ex1.rs? Would the original example from the RFC be a suitable "trivial container" to illustrate this?

(I ask because I view compile-fail/issue28498-reject-ex1.rs as an expansion of such a trivial container, so I am a little surprised to see you react so strongly to it. But switching to the example from the RFC is certainly easy; I just wanted to better understand the problem you have, which I assume is that it may confuse other people reading the commits about how truly simple-minded the new dropck is...)

Update: On re-reading compile-fail/issue28498-reject-ex1.rs, I now retract my claim that it is merely an "expansion" of a trivial container. I have come around to @arielb1 's POV that it really is demonstrating something different, and I should just go back to the example from the RFC.

I would also add a test for the interaction of UGEH with param bounds

Good idea; will do.

pnkfelix: I don't like the "unreachable" parametricity-checking code

Yeah that had a "bad code smell" to me as well. The reason I left it in:

other work has led me to think that the unsafe_destructor_blind_to_params attribute is not fine-grain enough (it essentially tags all type formals as parametric, when I think we actually want to specify parametricity for individual type parameters for some cases, if not now then in the near future),
and I was/am not sure if a finer-grain attribute will end up needing the other code, so I didn't want to immediately delete it.

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

arielb1 · 2015-10-06T15:43:16Z

src/librustc/middle/ty/util.rs

@@ -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


This whole method should be !self.has_attr(dtor_method, "unsafe_destructor_blind_to_params") - unless you have some other plan.

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.

arielb1 · 2015-10-06T15:46:23Z

I would prefer to either have a test that UGEH works with a "real" bound - like my fmt::Debug example, but made non-UB - or to still have the parametricity check. Documenting the unsafety rules somewhere would be nice (i.e. not using lifetime parameters, not using trait bounds, not calling function pointers accessible from self whose parameters contain a type parameter).

pnkfelix · 2015-10-08T11:45:10Z

Okay I agree with both of @arielb1's points in that last comment:

I need more tests to illustrate some of the other scenarios that arise here, and
I need to improve the documentation -- not sure where that should go, maybe in a PR on the rustonomicon doc.

… the attribute.

…w.r.t. a trait bound.

…w.r.t. a lifetime param.

…w.r.t. types in negative position.

pnkfelix · 2015-10-08T13:36:41Z

@arielb1 do you want to review the new tests and docs, or shall I hand that off to someone else?

bstrie · 2015-10-08T17:59:31Z

@pnkfelix does this close #26656?

arielb1 · 2015-10-08T19:33:13Z

@bstrie

It essentially works around it.

arielb1 · 2015-10-08T19:38:08Z

src/doc/nomicon/dropck.md

+
+The precise rules that govern drop checking may be less restrictive in
+the future.
+


I wouldn't really call the current analysis conservative (even though it is), given that it just doesn't care about the destructor.

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

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.

arielb1 · 2015-10-08T19:48:44Z

@gankro: could you take a look at the docs?

arielb1 · 2015-10-08T20:03:18Z

src/librustc/middle/ty/util.rs

-    /// 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.


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.

hmm, well that's better than the previous use of "alive", which I thought was too easy to misinterpret.

I wrote it before the "outlives" reform.

(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.

arielb1 · 2015-10-08T20:21:53Z

A point I want to document, not necessarily in this PR:

In most cases, when a method is called, its receiver is required to outlive the call.
If this was required by drop glue (or destructors), it would not be possible to create circular reference graphs.
Therefore, we have the (weaker) notion of "destruction-safety". Types without destructors are destruction-safe when their contents are.
To allow for library-defined containers, it is possible to create types with destructors that are destruction-safe when their contents are.
However, these types must be careful as they can be destroyed with their contents not being alive.

eefriedman · 2015-10-08T21:48:52Z

Is there some logic behind the specific types in the standard library you chose to mark #[unsafe_destructor_blind_to_params]? For example, VecDeque is marked, but HashSet isn't.

eefriedman · 2015-10-08T21:56:10Z

Nevermind, didn't see the comment on pnkfelix@d778e57 .

Would it make sense to mark Rc/Arc?

…nd-pasted into it.

pnkfelix · 2015-10-09T12:10:01Z

@eefriedman marking Rc / Arc seems sound, but I'd prefer not to expand the scope of marked types without seeing a motivating example...

Update: I constructed an example. :) I'll add it as a test and put the marker on Rc (and probably Arc too).

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

arielb1 · 2015-10-09T20:42:03Z

@bors r+

bors · 2015-10-09T20:42:04Z

📌 Commit a445f23 has been approved by arielb1

arielb1 · 2015-10-09T20:55:40Z

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 t is not alive during the call to del t, but as the types involved are non-dtorck types, there is no such actual constraint (if they were dtorck types, the constraint would cause the lifetime parameters, and the borrows, to be inferred to 'd, which would cause a borrowck error).

bors · 2015-10-10T00:39:31Z

⌛ Testing commit a445f23 with merge 87cd2c0...

@nikomatsakis

…elb1 implement RFC 1238: nonparametric dropck. cc #28498 cc @nikomatsakis

bors · 2015-10-10T02:29:36Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

Gankra · 2015-10-10T02:41:45Z

A bit late to the party for that review...

Easy to fix in Post.

pnkfelix · 2015-10-10T11:31:22Z

@gankro yeah I'll make a follow-up PR with further revisions to nomincon.

pnkfelix · 2015-10-10T16:56:53Z

@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.)

pnkfelix added 4 commits October 6, 2015 14:16

run-pass tests for RFC 1238.

eea299b

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.

rust-highfive assigned nrc Oct 6, 2015

rust-highfive assigned arielb1 and unassigned nrc Oct 6, 2015

pnkfelix added 2 commits October 6, 2015 16:51

shorten URLs to placate make tidy.

5708f44

review comment: use RFC example for compile-fail/issue28498-reject-ex…

7a4743f

…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.)

arielb1 reviewed Oct 6, 2015
View reviewed changes

pnkfelix added 5 commits October 8, 2015 14:43

review comment: reduce the is_adt_dtorck method to just a check for…

92da3f9

… the attribute.

Added tests illustrating when and when not to use the UGEH attribute …

e2e261f

…w.r.t. a trait bound.

Added tests illustrating when and when not to use the UGEH attribute …

7eda5b5

…w.r.t. a lifetime param.

Added tests illustrating when and when not to use the UGEH attribute …

73f35cf

…w.r.t. types in negative position.

Document the new more conservative dropck rule and the escape hatch.

9ed5faa

arielb1 reviewed Oct 8, 2015
View reviewed changes

placate check-pretty via comment rearrangement.

61f8def

pnkfelix added 2 commits October 9, 2015 13:30

revise cfail test, removing ugeh attribute that was erroneously cut-a…

b6a4f03

…nd-pasted into it.

review comment: point out that the dropck analysis is now trivial.

e1aba75

pnkfelix mentioned this pull request Oct 9, 2015

Major revision to the dropck_legal_cycles test. #28929

Merged

pnkfelix added 2 commits October 9, 2015 18:10

Added the param-blindness attribute to Rc and Arc.

34076bc

This was proven necessary after I added `Rc` and `Arc` to the rpass test `dropck_legal_cycles.rs`; see PR rust-lang#28929.

review comment: further refinement of comment above fn is_adt_dtorck.

a445f23

bors added a commit that referenced this pull request Oct 10, 2015

Auto merge of #28861 - pnkfelix:fsk-nonparam-dropck-issue28498, r=ari…

87cd2c0

…elb1 implement RFC 1238: nonparametric dropck. cc #28498 cc @nikomatsakis

bors merged commit a445f23 into rust-lang:master Oct 10, 2015

pnkfelix mentioned this pull request Oct 16, 2015

dropck doc oversights / revision #29111

Open

apasel422 mentioned this pull request Oct 16, 2015

Add Shared pointer and have {Arc, Rc} use it #29110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement RFC 1238: nonparametric dropck. #28861

implement RFC 1238: nonparametric dropck. #28861

pnkfelix commented Oct 6, 2015

rust-highfive commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

arielb1 Oct 6, 2015

pnkfelix Oct 8, 2015

arielb1 commented Oct 6, 2015

pnkfelix commented Oct 8, 2015

pnkfelix commented Oct 8, 2015

bstrie commented Oct 8, 2015

arielb1 commented Oct 8, 2015

arielb1 Oct 8, 2015

pnkfelix Oct 8, 2015

arielb1 Oct 8, 2015

arielb1 commented Oct 8, 2015

arielb1 Oct 8, 2015

pnkfelix Oct 8, 2015

arielb1 Oct 8, 2015

pnkfelix Oct 9, 2015

arielb1 commented Oct 8, 2015

eefriedman commented Oct 8, 2015

eefriedman commented Oct 8, 2015

pnkfelix commented Oct 9, 2015

arielb1 commented Oct 9, 2015

bors commented Oct 9, 2015

arielb1 commented Oct 9, 2015

bors commented Oct 10, 2015

bors commented Oct 10, 2015

Gankra commented Oct 10, 2015

pnkfelix commented Oct 10, 2015

pnkfelix commented Oct 10, 2015


		The precise rules that govern drop checking may be less restrictive in
		the future.

implement RFC 1238: nonparametric dropck. #28861

implement RFC 1238: nonparametric dropck. #28861

Conversation

pnkfelix commented Oct 6, 2015

rust-highfive commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

pnkfelix commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Oct 6, 2015

pnkfelix commented Oct 8, 2015

pnkfelix commented Oct 8, 2015

bstrie commented Oct 8, 2015

arielb1 commented Oct 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Oct 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Oct 8, 2015

eefriedman commented Oct 8, 2015

eefriedman commented Oct 8, 2015

pnkfelix commented Oct 9, 2015

arielb1 commented Oct 9, 2015

bors commented Oct 9, 2015

arielb1 commented Oct 9, 2015

bors commented Oct 10, 2015

bors commented Oct 10, 2015

Gankra commented Oct 10, 2015

pnkfelix commented Oct 10, 2015

pnkfelix commented Oct 10, 2015