dropck and new scoping rules for safe destruction #21972
Conversation
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
|
This PR is for @nikomatsakis to look at. It is marked "no checkin" because it includes a |
|
(also, I would not object if someone wants me to separate out the removal of |
|
modulo that nit, the parametricity commit looks good to me. I had some doubts for a bit but assuaged them by building and thinking about it. I think the key point is that when the "type has dtor of interest" flag is false, we are basically inlining what the default drop glue will do, since we have decided that the actions of the destructor itself are immaterial. One key point that worried me for a bit is that any type which carries borrowed data explicitly must have a region parameter, and hence is always a "dtor of interest" with the stricter rules. |
pnkfelix
force-pushed
the
new-dtor-semantics-6
branch
3 times, most recently
from
c84b47e
to
509a378
Compare
(My fix to for-loops (21984) did not deal with similar problems in if-let expressions, so those binding shifts stay.)
immediately surrounding a node that is a terminating_scope (e.g. statements, looping forms) during which the destructors run (the destructors for temporaries from the execution of that node, that is). Introduced DestructionScopeData newtype wrapper around ast::NodeId, to preserve invariant that FreeRegion and ScopeChain::BlockScope carry destruction scopes (rather than arbitrary CodeExtents). Insert DestructionScope and block Remainder into enclosing CodeExtents hierarchy. Add more doc for DestructionScope, complete with ASCII art. Switch to constructing DestructionScope rather than Misc in a number of places, mostly related to `ty::ReFree` creation, and use destruction-scopes of node-ids at various calls to liberate_late_bound_regions. middle::resolve_lifetime: Map BlockScope to DestructionScope in `fn resolve_free_lifetime`. Add the InnermostDeclaringBlock and InnermostEnclosingExpr enums that are my attempt to clarify the region::Context structure, and that later commmts build upon. Improve the debug output for `CodeExtent` attached to `ty::Region::ReScope`. Loosened an assertion in `rustc_trans::trans::cleanup` to account for `DestructionScope`. (Perhaps this should just be switched entirely over to `DestructionScope`, rather than allowing for either `Misc` or `DestructionScope`.) ---- Even though the DestructionScope is new, this particular commit should not actually change the semantics of any current code.
Largely adapted from pcwalton's original branch, with following notable modifications: Use `regionck::type_must_outlive` to generate `SafeDestructor` constraints. (this plugged some soundness holes in the analysis). Avoid exponential time blowup on compile-fail/huge-struct.rs by keeping the breadcrumbs until end of traversal. Avoid premature return from regionck::visit_expr. Factored drop-checking code out into dropck module. Added `SafeDestructor` to enum `SubregionOrigin` (for error reporting). ---- Since this imposes restrictions on the lifetimes used in types with destructors, this is a (wait for it) [breaking-change]
pnkfelix
force-pushed
the
new-dtor-semantics-6
branch
9 times, most recently
from
85627d5
to
1a2954b
Compare
| let &ty::ParamBounds { | ||
| ref region_bounds, builtin_bounds, ref trait_bounds, | ||
| ref projection_bounds, | ||
| } = &t.bounds; |
There was a problem hiding this comment.
Just to establish context (post rebasing) for his note above, nikomatsakis had a note here (in the original commit), as follows:
the bounds are reflected in the predicates already, so you don't have to look at them.
pnkfelix
force-pushed
the
new-dtor-semantics-6
branch
from
9bc49c0
to
2c9d81b
Compare
|
⌛ Testing commit 2c9d81b with merge fc6123d... |
|
💔 Test failed - auto-win-32-nopt-t |
|
@bors: retry |
This is a resurrection and heavy revision/expansion of a PR that pcwalton did to resolve #8861. The most relevant, user-visible semantic change is this: #[unsafe_destructor] is gone. Instead, if a type expression for some value has a destructor, then any lifetimes referenced within that type expression must strictly outlive the scope of the value. See discussion on rust-lang/rfcs#769
|
Argh, bors, you used my old description for your merge message! |
Hey @barosl is this related to barosl/homu#51 , or is it a distinct issue that deserves a separate ticket? (My old description said "... (Having said that, fixing the commit message is not worth cancelling the build. But if this build fails, then I will open a fresh PR to work around these issues with bors.) |
|
This is a big deal! 🚀 |
|
Beautiful tests. |
This is a resurrection and heavy revision/expansion of a PR that pcwalton did to resolve #8861.
The most relevant, user-visible semantic change is this: if a type expression for some value has a destructor, then any lifetimes referenced within that type expression must strictly outlive the scope of the value.
See discussion on rust-lang/rfcs#769, especially for the discussion of the meaning of "strictly outlive."
A previous version of this pull request also removed the
#[unsafe_destructor]attribute, as well as the analysis that would reject genericDropimpls that were not annotated with that attribute. For the short term, however, we are landing just the new, stricter, believed-to-be sound analysis; there is still work remaining on #8861 before we could be comfortable removing the#[unsafe_destructor]attribute.[breaking-change]