-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Treat generators as if they have an arbitrary destructor #49943
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
r? @Zoxc |
(as noted in the comments in code, in principle we could do something smarter here, in terms of recursively analyzing the locals of the generator to see if we can prove e.g. that none of them have destructors. But that is complex and this is simple.) |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This looks correct to me. We model generators as a struct of their upvars with a destructor which may access these upvars. The I'd like someone who knows how dropck works to glance over it though. I'm just going to assume Niko is such a person. |
ty::TyGenerator(def_id, substs, _) => { | ||
// Note that the interior types are ignored here. | ||
// Any type reachable inside the interior must also be reachable | ||
// through the upvars. |
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 comment should be kept in some form. It serves as justification for why we can get away with only reporting the upvars instead of actually calculating all the types which may get dropped inside.
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.
(okay, addressed this in followup commit)
I think the request to see what the output looks like under NLL will be satisfied by the eventual (hopefully soon) landing of PR #49900, so I won't put that change into this PR. |
// because any side-effects from dropping `_interior` can | ||
// only take place through references with lifetimes | ||
// derived from lifetimes attached to the upvars, and we | ||
// *do* traverse the uvpars here. |
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.
Typo, uv-pars. Looks good otherwise.
@pnkfelix I'd just like you to run the test locally with NLL and post the errors in a comment here. |
@Zoxc oh okay I can certainly do that. |
dd63992
to
856637a
Compare
@Zoxc here is the current behavior on AST-borrowck:
NLL (
|
There's already a test for this at https://github.com/rust-lang/rust/blob/master/src/test/ui/generator/dropck.rs, that was commented out for some reason. With this PR, that test should give an error. |
let new_constraints: Vec<ty::subst::Kind<'tcx>> = | ||
substs.upvar_tys(def_id, tcx).map(|t| t.into()).collect(); | ||
|
||
let mut constraint = substs |
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.
https://github.com/rust-lang/rust/pull/45337/files#r145093242
It might be a better idea to treat generators like trait object and have them always be dtorck. If generators are "supposed to be" captured by animpl Trait
or trait object, this shouldn't be a problem in practice..
There's no reason to have both outlives
and dtorck constraints together, the outlives
constraint is always stricter than the is-destruction-safe
constraint.
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.
Are you saying @arielb1 that we only need to invoke constraint.outlives.extend(new_contraints)
, and not also include dtorck_constraint_for_ty
?
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.
Sure just return new_constraints
as outlives constrains, same as trait objects.
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.
r=me with arielb1's comment fixed
Ping from triage, @pnkfelix ! Will you have time to address the review feedback in the near future? |
@shepmaster yep, I'll do it either today or tomorrow. (I was out on PTO last week.) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…cals' dtors. This is meant to address rust-lang#49918. Review feedback: put back comment justifying skipping interior traversal. Review feedback: dropck generators like trait objects: all their upvars must outlive the generator itself, so just create a DtorckConstraint saying so.
@bors r=nikomatsakis |
📌 Commit f12d7a5 has been approved by |
Treat generators as if they have an arbitrary destructor Conservatively assume dropping a generator touches its upvars, via locals' destructors. Fix #49918
☀️ Test successful - status-appveyor, status-travis |
Conservatively assume dropping a generator touches its upvars, via locals' destructors.
Fix #49918