Immovable generators by Zoxc · Pull Request #45337 · rust-lang/rust

Zoxc · 2017-10-17T05:22:04Z

This adds support for immovable generators which allow you to borrow local values inside generator across suspension points. These are declared using a static keyword:

letmut generator = static || {let local = &Vec::new();yield;
local.push(0i8);};
generator.resume();// ERROR moving the generator after it has resumed would invalidate the interior reference// drop(generator);

Region inference is no longer affected by the types stored in generators so the regions inside should be similar to other code (and unaffected by the presence of yield expressions). The borrow checker is extended to pick up the slack so interior references still result in errors for movable generators. This fixes#44197, #45259 and #45093.

This PR depends on PR #44917 (immovable types), I suggest potential reviewers ignore the first commit as it adds immovable types.

rust-highfive · 2017-10-17T05:22:07Z

r? @arielb1

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

arielb1 · 2017-10-17T09:44:22Z

Can't you replicate the zip loop that the TyTuple branch does?

I had to deal with the Binder, which needs an intermediate type. I could create one though.

You could implement Relate for &'tcx Slice<Ty<'tcx>> using the same algorithm as Substs.

I changed this to use an intermediate type to make it clear the logic only applies to TyGeneratorWitness.

arielb1 · 2017-10-17T10:08:42Z

This doesn't feel right. If the generator captures an upvar through a mutable reference, the generator doesn't have dropck constraints:
#![feature(generators, generator_trait)]use std::cell::RefCell;use std::ops::Generator;fnstatic_alloc<'a,T>(t:T) -> &'amutT{// this is an unsafe function, but it is sound (not running destructors// is safe), can be implemented using a static dropless arena of// `ManuallyDrop` data, and was planned to be added to the standard// library at some point.unsafe{use std::mem;letmut b = Box::new(t);let r = &mut*(&mut*b as*mutT); mem::forget(b); r }}fnmain(){let(cell,mut gen); cell = Box::new(RefCell::new(0));let ref_ = static_alloc(Some(cell.borrow_mut()));// the upvar is the non-dropck `&mut Option<Ref<'a, i32>>`. gen = || {// but the generator can use it to drop a `Ref<'a, i32>`.let _d = ref_.take();yield;}; gen.resume();// drops the RefCell and then the Ref, leading to use-after-free}

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 an impl Trait or trait object, this shouldn't be a problem in practice..

This gives the following error.:
error[E0597]: `ref_` does not live long enough --> dtorck.rs:27:18 | 25 | gen = || { | -- capture occurs here 26 | // but the generator can use it to drop a `Ref<'a, i32>`. 27 | let _d = ref_.take(); | ^^^^ does not live long enough ... 32 | } | - borrowed value dropped before borrower | = note: values in a scope are dropped in the opposite order they are created error: aborting due to previous error
Note that upvars are still checked. Even if we didn't ignore the interior, would higher-ranked regions have any effect on dtorck?

Does this still give the error with your PR? the only upvar should be ref_, which should have an empty dtorck constraint. Maybe I just need to check this PR out to see that this holds?

That error is from this PR.

Could you make this into a test?

arielb1 · 2017-10-17T10:09:47Z

TyFnPtr skips binders blindly in the next 5 lines, so sure.

Ok. Removed this FIXME.

arielb1 · 2017-10-17T10:16:16Z

// It is either a Box or some already borrowed local.
nit: it can also be a borrow from something based on an upvar, e.g. &fcx.tcx.

I was thinking of MIR locals here.

(*fcx).tcx is not an already borrowed MIR local, it's from the place where fcx was allocated from, which can be everywhere (e.g. it could be a field of an arena or Vec, which are not a Box).
I just want to avoid comments that lie to avoid rumors spreading.

I changed this comment.

arielb1 · 2017-10-17T10:18:51Z

You could also only promote locals that are borrowed across a yield if you don't want to promote everything.

I'd need a liveness analysis which properly handles borrows then. I'm waiting on NLL before I do that. My plan is to have a pass which optimizes Storage* instructions based on NLL and then let the generator transformation only use the dataflow analysis on those instructions.

arielb1 · 2017-10-17T10:22:45Z

not a tuple anymore.

arielb1 · 2017-10-17T10:43:35Z

Maybe add a comment to the following effect:
The types in the generator interior contain lifetimes local to the generator itself, which should not be exposed outside of the generator. Therefore, we replace these lifetimes with existentially-bound lifetimes, which reflect the exact value of the lifetimes not being known by users.
These lifetimes are used in auto trait impl checking (for example, if a Sync generator contains an &'α T, we need to check whether &'α T: Sync), so knowledge of the exact relationships between them isn't particularly important.
Note that lifetimes in the type of a local that is currently dead need not be alive. It's up to the generator to ensure that captured lifetimes that are actually used are alive.

Note that lifetimes in the type of a local that is currently dead need not be alive. It's up to the generator to ensure that captured lifetimes that are actually used are alive.
I'm not sure what you mean by this. I added your other paragraphs as comments.

arielb1 · 2017-10-17T10:55:50Z

Can you add a test that static generators actually work - that you can yield *b and that it stays valid?

You could do yield *b in movable generator, given NLL. Did you mean I should use the reference after the yield so the test still works with NLL?

That's one thing. Another thing is that you should have a test that actually runs the generator and asserts that the references have the same value between resumes, to quickly catch if some change horribly breaks immovable generator codegen.

I changed this test.

arielb1 · 2017-10-17T10:56:38Z

Could you also add an rpass test for the "vexpr lifetime" case (aka #44197)?

I also need to think of more tests to add here.

I added a test case for #44197.

bors · 2017-10-21T09:37:43Z

☔ The latest upstream changes (presumably #45381) made this pull request unmergeable. Please resolve the merge conflicts.

carols10cents · 2017-10-30T15:57:03Z

Triage ping @Zoxc -- looks like there are merge conflicts again and some test failures!

Zoxc · 2017-10-30T16:40:02Z

This PR is blocked on landing #44917.

bors · 2017-11-05T19:20:47Z

☔ The latest upstream changes (presumably #45072) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2017-11-08T09:44:34Z

☔ The latest upstream changes (presumably #45735) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2017-11-15T10:32:45Z

☔ The latest upstream changes (presumably #45944) made this pull request unmergeable. Please resolve the merge conflicts.

kennytm · 2017-11-22T10:58:26Z

triage: still blocked by #44917.

nikomatsakis · 2018-01-19T19:58:58Z

As an aside, It doesn't seem to me that creating a "static generator" has to be unsafe, but I don't mind that it is. Am I missing something though? That is, is it UB to create a static generator under some conditions (even if you never invoke next?).

I would have thought that the proof burden would be:

Once you invoke next the first time,
all further calls must use the same pointer.

arielb1 · 2018-01-19T22:52:15Z

@nikomatsakis

In the generator's contents signature, all regions are replaced with bound regions. This means a generator type would be something of the form:

⊢ AutoTrait auto trait
⊢ ∀'a,'b. {Foo1<'a>, Foo2<'b>}: AutoTrait
----------------
⊢ Generator#1234<'x, 'y, T, may_contain=∃'a,'b. {Foo1<'a>, Foo2<'b>}, return_type=T): AutoTrait

Basically, the generator contains some of the interior times for some set of substituted lifetimes, so if an ~~OIBIT~~auto trait holds for every substitution of the lifetimes, then it holds for the generator.

Note that we lose all information about relations between the lifetimes - we might have had impl Sync for Foo1<'static>, and the generator might have only ever stored a Foo1<'static>, but we don't try to handle that. I think this should be not that bad in practice because the generator's contents only get used for proving auto traits, which should not have weird lifetime constraints.

arielb1 · 2018-01-19T22:56:02Z

It doesn't seem to me that creating a "static generator" has to be unsafe,

ATM static generators use the "standard" safe Generator trait. This means that static generators, once created, are a safety hazard - moving them and calling resume again would cause UB.

The above is probably a bad idea - it is very easy to make an unsafe API using them - and we should create an new ImmovableGenerator trait with an unsafe resume method.

nikomatsakis · 2018-01-19T23:26:50Z

@arielb1 oh, I hadn't even looked at the trait part of this PR, I just assumed we would have an ImmobileGenerator trait =)

I think that's what we want, yeah.

Zoxc · 2018-01-22T06:08:18Z

Am I missing something though? That is, is it UB to create a static generator under some conditions (even if you never invoke next?).

It is always safe to create a generator.

I would have thought that the proof burden would be:
Once you invoke next the first time,
all further calls must use the same pointer.

You have to ensure that once you call resume the generator cannot move in memory. You cannot move it between resume calls.

nikomatsakis · 2018-01-22T17:00:36Z

It is always safe to create a generator.

By this I guess you mean a non-static generator?

You have to ensure that once you call resume the generator cannot move in memory. You cannot move it between resume calls.

Right, and for this reason I think that the resume method ought to be unsafe.

But I think we can tweak this in follow-up work. I'm pretty happy with this PR.

r=me once rebased.

…ws across suspension points to borrowck. Fixesrust-lang#44197, rust-lang#45259 and rust-lang#45093.

Zoxc · 2018-01-23T07:38:49Z

@bors r=nikomatsakis

bors · 2018-01-23T07:38:50Z

📌 Commit 55c6c88 has been approved by nikomatsakis

bors · 2018-01-23T22:50:10Z

⌛ Testing commit 55c6c88 with merge fdecb05...

Immovable generators This adds support for immovable generators which allow you to borrow local values inside generator across suspension points. These are declared using a `static` keyword: ```rust let mut generator = static || { let local = &Vec::new(); yield; local.push(0i8); }; generator.resume(); // ERROR moving the generator after it has resumed would invalidate the interior reference // drop(generator); ``` Region inference is no longer affected by the types stored in generators so the regions inside should be similar to other code (and unaffected by the presence of `yield` expressions). The borrow checker is extended to pick up the slack so interior references still result in errors for movable generators. This fixes#44197, #45259 and #45093. This PR depends on [PR #44917 (immovable types)](#44917), I suggest potential reviewers ignore the first commit as it adds immovable types.

bors · 2018-01-24T01:43:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing fdecb05 to master...

rust-highfive assigned arielb1 Oct 17, 2017

Zoxc force-pushed the gen-static branch 2 times, most recently from c512527 to 0ffc23aCompare October 17, 2017 05:52

arielb1 reviewed Oct 17, 2017
View reviewed changes

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 17, 2017

Zoxc force-pushed the gen-static branch from 155c6fd to 0a62415Compare October 21, 2017 11:48

Zoxc force-pushed the gen-static branch 3 times, most recently from 398c882 to 233b261Compare November 5, 2017 18:58

Zoxc force-pushed the gen-static branch from 233b261 to 6cc9891Compare November 5, 2017 20:31

Zoxc force-pushed the gen-static branch from 6cc9891 to 42ddbdaCompare November 14, 2017 14:24

carols10cents added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2017

kennytm removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 6, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2018

Zoxc force-pushed the gen-static branch from 3e27a93 to 8dcc5c4Compare January 23, 2018 02:45

Zoxc added 6 commits January 23, 2018 05:10

Adds support for immovable generators. Move checking of invalid borro…
ccf0d83
…ws across suspension points to borrowck. Fixesrust-lang#44197, rust-lang#45259 and rust-lang#45093.

Make immovable generators unsafe
c166ef9

Use delay_span_bug for things that should be errors
9b57e60

Fix yield-while-local-borrowed.rs test
bae2988

Add dropck test
410d27b

Port borrows across yield check to MIR borrowck
55c6c88

Zoxc force-pushed the gen-static branch from 8dcc5c4 to 55c6c88Compare January 23, 2018 04:11

bors merged commit 55c6c88 into rust-lang:masterJan 24, 2018

bors mentioned this pull request Jan 24, 2018
Refactor diverging and numeric fallback. #46714
Merged

andreytkachenko mentioned this pull request Jan 24, 2018
Generator state borrowings should have same as generator lifetime #47654
Closed

topecongiro mentioned this pull request Jan 26, 2018
Add a support for immovable generators rust-lang/rustfmt#2397
Closed

Zoxc mentioned this pull request Jan 27, 2018
Borrow error when yielding with nested generators #45093
Closed

andreytkachenko mentioned this pull request Feb 1, 2018
Consider add support of recently merged immovable generators alexcrichton/futures-await#58
Closed

ehuss mentioned this pull request Nov 6, 2018
[WIP] Implement (possibly?) the entire Rust grammar. rust-lang/wg-grammar#13
Closed

pnkfelix mentioned this pull request Nov 12, 2018
Do not check outlives when explicit bound contains escaping regions #53736
Closed

Zoxc deleted the gen-static branch August 14, 2019 08:15

Uh oh!

Uh oh!

Conversation

Zoxc commented Oct 17, 2017

Uh oh!

rust-highfive commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Oct 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Oct 21, 2017

Uh oh!

carols10cents commented Oct 30, 2017

Uh oh!

Zoxc commented Oct 30, 2017

Uh oh!

bors commented Nov 5, 2017

Uh oh!

bors commented Nov 8, 2017

arielb1Oct 18, 2017•
edited
Loading

arielb1Oct 17, 2017•
edited
Loading

arielb1Oct 17, 2017•
edited
Loading