Nonparametric dropck

[pnkfelix]

Summary

Revise the Drop Check (dropck) part of Rust's static analyses in two ways. In the context of this RFC, these revisions are respectively named cannot-assume-parametricity and unguarded-escape-hatch.

1. cannot-assume-parametricity (CAP): Make dropck analysis stop relying on parametricity of type-parameters.
2. unguarded-escape-hatch (UGEH): Add an attribute (with some name starting with "unsafe") that a library designer can attach to a drop implementation that will allow a destructor to side-step the dropck's constraints (unsafely).

current RFC text

original draft

[pnkfelix]

CC #1210

[pnkfelix]

this RFC cross-cuts both the lang and libs teams, so I added both labels for now (it is mostly a lang issue, but the libs team may have opinions on some of the claims made about the utility of the escape hatch (aka UGEH)).

[pnkfelix]

cc @nikomatsakis @aturon

[arielb1]

@pnkfelix

Does this really cause no regressions across crates.io? Nobody implemented their own arena?

On the ?Special note: ?Special is actually much more general than requiring all transitively called functions to be pureparametric, because ?Special allows calling arbitrary functions that don't take T as a parameter (e.g. by casting the problematic *const T to an *const (), or even an handcrafted *const TraitObject if you need to pass the size/align).

[pnkfelix]

@arielb1 0 regressions is what we observed.

Caveat: I'm not 100% familiar with how much coverage crater provides. In particular, I imagine its possible that people have implemented their own arena types, but no code touched by crater is using them to construct cyclic internally-linking structures.

(In other words, the errors injected by this change will occur at the usage site where one attempts to create references that violate the new region relation; I do not expect to see errors at the sites that declare the destructors themselves.)

[aturon]

cc @pythonesque

[pythonesque]

I'm pretty sure this would cause regressions in some of my own libraries. Is there an easy way to test against the new code?

(I'm not in principle against some sort of unsafe escape hatch, but it's unclear to me that there's a way to replicate the existing functionality without it, and the RFC text suggests that any such escape hatch would be temporary).

Personally, if it comes down to breaking the ability to have safe cycles within a TypedArena vs. specialization, I'd take the former every time, but this is obviously something subjective.

[nikomatsakis]

@pythonesque I think the plan was that the escape hatch would be unstable, but not temporary. That is, an escape hatch is definitely needed unless we adopt some other replacement. Can you point at the code you think might be broken by this change?

[pythonesque]

@nikomatsakis Unstable is fine. https://github.com/pythonesque/hm is something that I suspect is relying on the current behavior. If that still works I'm pretty okay with this change (note that it might need to be updated for other reasons since I've been busy with other work).

I'd add that in general, I am okay with any resolution to this provided that there is a way to work around it. What wasn't clear to me from the RFC (but what you seem to be indicating) is that there will continue to be a way to work around this in the future.

[nikomatsakis]

@pythonesque

What wasn't clear to me from the RFC (but what you seem to be indicating) is that there will continue to be a way to work around this in the future.

Yes. This use case has to continue working.

What's in doubt is whether there will be a way to support such a pattern without using unsafe code for the destructor (in this case, an unsafe attribute). I think all things being equal, everyone would prefer that (though even if this pattern is considered safe, you'd still have to use explicit annotation to get the current "parametric" behavior). However, I confess to some doubts as to whether some form of annotation carries its weight: if we can find something that also addresses other use cases, it would feel better. Otherwise I feel like this may fall on the "just use unsafe code" side of the line. But we don't have to settle it right now. (And even if we stabilized an attribute, we could always deprecate it later in favor of a safe alternative.)

[cristicbz]

I would also expect generated capnproto code to break (as well as flatbuffers). Both rely on arena allocation. I'm roping them in cc @dwrensha @arbitrary-cat . I'm also working on an arena based protobuf implementation which I'm pretty sure would be broken by this (a nested message is stored in the same arena with an Option<&'a Bar> in the parent message).

I understand that specialisation is important and that this change can be considered a 'bugfix' (due to the number of bugs caused by the current behaviour) but removing support for arenas on stable is a significant breaking change that would make me sad.

Requiring unsafe code could be reasonable, given that arenas are likely to have plenty of that anyway.

[RalfJung]

Am I missing something here, or is this RFC proposing to make it impossible to implement container data types (that can contain pointers to themselves, like the Concrete example) outside of libstd? I think that'd be a very bad idea. It would restrict the entire Rust universe to the containers provided in libstd, and it'd even make experimenting with new containers on crates.io before adding them to libstd impossible. I mean, libstd contains lots of useful containers, but there'll always me niches where some specialized container, too obscure to be in libstd, is the only viable option.

[pnkfelix]

@RalfJung yes: the RFC is not proposing a specific long-term plan for general-purpose containers (namely, containers that can hold values with references to other values in the same container), and therefore the escape-hatch it proposed is specifically going to be unstable until we do have a long term plan.

Experimentation with such containers can use the proposed escape-hatch in the short term with the nightly release channel, but not with beta/stable.

To be clear, however: We need a long term plan. Some ideas were sketched in the Alternatives section. However, part of the point here is to avoid blocking the "impl specialization" work on such an alternative.

[nrc]

Could you either explain here exactly how specialisation breaks parametricity or link to a specific comment or something - it is not discussed in the linked RFC.

[Gankra]

@nrc In essence, parametricity says you're A-OK if you're working with a totally generic T. You can in principle call functions on T knowing nothing interesting will be done with it, since it's totally generic. However with specialization those functions can do something interesting (by specializing their implementation).

[RalfJung]

On a slightly more concrete note: One of the consequences of parametricity is that if you have a function fn foo<T>(t: T) -> T, then there is literally only one thing the function can do: Return its argument. (It would also mutate some other state, but that's irrelevant here.) In particular, the function can not dereference any pointer contained in t, since it can't even know whether there are pointers in t! This is because foo has to do "basically the same thing" for all T. After all, there's only one foo implementing all that!

However, once we have specialization, one could write a special version of this function for T = u32 that increments its argument. Or a special version for T = &mut i32 that increments the data in the pointer, and then return the pointer. Suddenly, the type of the function alone no longer guarantees that the function has to do "basically the same thing" for all T.

[pnkfelix]

(@nrc is right, the RFC itself needs the explanation)

[nrc]

I think this RFC would be easier to understand if it were more stand-alone - I feel like I need 706 open in another tab and have to flick between the two to know what the effect of this RFC will be.

[nrc]

typo: allows

[mitsuhiko]

@pnkfelix

Experimentation with such containers can use the proposed escape-hatch in the short term with the nightly release channel, but not with beta/stable.

That seems like an unwise thing to do. This is not exactly an uncommon situation to have but this would be the first time that functionality is effectively removed from stable rust as far as I understand.

[llogiq]

Given that parametricy is a necessary condition for sound drop, could the current logic be made available as a lint, so that whoever is aware of the limitation can at least use it to check for unsound impls (at least given a non-specialized impl)?

[nikomatsakis]

I remain pretty convinced we should take this step quickly, but I'd like to collect examples where annotations are required so we can analyze their usage patterns. For example, when it comes to stabilizing, I can imagine that we might institute (and stabilize) a sound but overly conservative analysis, and retain an (unstable, for now) unsafe attribute to go beyond. Looking at code that requires changes would give us some idea how useful this is, of course.

@cristicbz can you point us at any of the code you believe would be affected? (is it open source / publicly accessible?)

[cristicbz]

capnproto & flatbuffers were just guesses, since both of them use arena-like allocation. However, their 'references' may actually be indices (given that they need to serialize by simply mapping the memory). That's why I was hoping to get either @dwrensha or @arbitrary-cat to chip in. I was worried that crater might not spot it because it affects only generated code (then again, one would assume the tests of any crates which use either of the libraries would exercise the generated code as well).

My arena protobuf implementation is not open source yet I'm afraid :(. What I use is basically a version of arena::Arena without the dtor stuff (since it's only used for Copy types). A proto message containing a nested message (or a string/byte slice) is stored together with its children in a single arena:

struct Foo<'a> {
    bar: i32,
    baz: Option<&'a Baz>,    // Stored in the same arena right before the Foo instance.
}

struct Frob<'a> {
    iffy: Option<&'a str>,    // Stored in the same arena right before the Frob instance.  
}

The actual arena code seems very similar to this one (on a superficial reading of the latter). TBH, I don't fully grok the RFC, but from what I can tell, it makes it impossible to create arenas which store references to other things in the arena.

[dwrensha]

I was worried that crater might not spot it because it affects only generated code...

Yeah, I think Crater will test capnproto-rust's runtime library and its schema compiler plugin, but none of its generated code. The problem is that code generation requires that the schema compiler be installed. (Perhaps I could address this lack of coverage by having a build.rs that downloaded and installed the schema compiler (written in C++) if it's not already installed on the system?)

I am indeed curious about whether the proposed changes in the RFC break anything in capnproto-rust. It would be good if someone with access to the new branch (@pnkfelix, @nikomatsakis?) could run it on my test suites, as described by these travis configs: https://github.com/dwrensha/capnpc-rust/blob/master/.travis.yml , https://github.com/dwrensha/capnp-rpc-rust/blob/master/.travis.yml .

[nikomatsakis]

@dwrensha sorry for the delay; I'll try to whip up a build of the branch and run some tests (@pnkfelix has been doing the work there, but the commits should be on his repo; he's not around right now). Hopefully I can get to that over the weekend.

[glaebhoerl]

I feel like the two separate motivations for the change are being mushed together, or at least, I'm having a hard time separating them. If we don't consider the prospect of specialization, could the parametricity rule for dropck be salvaged, or is it fatally flawed?

[llogiq]

@glaebhoerl If I have understood the problem correctly, parametricity is not sufficient for dropck soundness even in current Rust.

[RalfJung]

Exactly. Also see the discussion in rust-lang/rust#26656.

[glaebhoerl]

Yeah, I've seen the issues on the rust repo. The question in my mind is isn't (this is clear) or cannot be (this isn't)?

E.g. @pnkfelix commented:

We need a stronger check, that ensures that we only allow the weaker region relations on parameter types T that can be proven do not flow into any "negative positions" (e.g. function arguments). Positive positions are okay since those can only create instances of T, they cannot consume the one we have in hand.

But I don't know if this is believed to actually be feasible and workable, or if it was just theorizing.

[pnkfelix]

@glaebhoerl I think the negative-position analysis of the types is entirely feasible; it was at least the plan I was planning to follow.

But after @arielb1 pointed out that specialization (RFC PR #1210) breaks the parametricity properties that dropck needs, I opted not to invest further effort in parametricity-based solutions until after we resolve whether we are planning to support such systems in the future.

[glaebhoerl]

@pnkfelix OK, thanks for the clarification!

[nikomatsakis]

Hear ye, hear ye. This RFC is entering final comment period.

[nikomatsakis]

A quick note: I apologize that neither I nor @pnkfelix haven't gotten to investing the impact of this change on capnproto-rust. However, as the existing functionality will still be available (but unstable), it doesn't seem to be a blocking concern either way.

[glaebhoerl]

I don't think it makes sense for this to go into FCP before specialization is decided, considering @pnkfelix's last comment.

[aturon]

@glaebhoerl

This RFC by itself does not preclude using parametricity again in the future; it moves us back to a more conservative position, where an unsafe attribute it required, while we determine our long-term strategy. IMO, the more quickly we move to it, the less dependence we have around the current (unsound) rules.

[rkjnsn]

While I understand the motivation for this change and the desire to move quickly, I am concerned that not only are we potentially breaking code that currently works on stable, there will be no way to get that code to work again without switching to nightly. I don't think this should land without some way for affected code to continue to work on stable. One option would be a way (unsafely) opt into the old rules on a per-struct basis. Even if we know such an attribute will later be deprecated when we devise our long term strategy, I think it is very important that code that currently targets stable can continue to do so.

[bstrie]

@rkjnsn has a point, it may be prudent to provide a new unsafe attribute for opting in to the old behavior. I have no idea how much of an implementation burden that would be, but luckily it wouldn't need to stick around forever (and requiring an opt-in would keep new code from relying on the old behavior).

[aturon]

@rkjnsn @bstrie To make sure I understand: you're basically proposing to immediately stabilize the attribute being proposed here?

Given the results of crater (which showed zero regressions), I would be inclined to make this change with an unstable attribute and then widely advertise it, and see whether it is being used on stable Rust (in some place we cannot see). While this does indeed remove stable functionality, it's worth remembering that the functionality is unsound today, so some kind of breaking change is required; adding an attribute that might move crates back to nightly seems like a not terrible solution. And all that said, if this does turn out to be an issue during the nightly/beta period, we can move to stabilize the attribute and hope to deprecate it later.

[arielb1]

@aturon

Doesn't this break rustc irreparably?

There is some unsoundness around this functionality - the poor handling of fn - but it is not significant (using it to justify removing dropck seems to me about as bad as using the contravariance bug to justify removing all kinds of variance, except you can use transmute to work around variance issues).

[nikomatsakis]

@arielb1

Doesn't this break rustc irreparably?

What do you mean by this?

[aturon]

@arielb1

Doesn't this break rustc irreparably?

The RFC adds an unsafe annotation that provides the same capabilities as today's parametricity-based analysis. As @pnkfelix says in the RFC, this has already been prototyped and tested on crates.io (where the attribute is applied to the collections in std) with no breakage.

There is some unsoundness around this functionality - the poor handling of fn - but it is not significant (using it to justify removing dropck seems to me about as bad as using the contravariance bug to justify removing all kinds of variance, except you can use transmute to work around variance issues).

I'm not proposing that as the justification; clearly the main long-term motivation is making room for specialization. What I've been arguing is that, whatever else happens, it makes sense to move in this direction quickly to avoid reliance on the current rules and to patch the current soundness hole while we figure out our long-term strategy.

[alexcrichton]

This libs team discussed this RFC today during our triage meeting, and the conclusion was: 👍!

(as in, we're ok with the fallout here)

[nikomatsakis]

Huzzah! The language design team has decided to accept this RFC.