Generator bugfixes #47845

Zoxc · 2018-01-29T08:50:55Z

petrochenkov · 2018-01-29T09:45:35Z

I haven't reported this as an issue yet, but immovable generators can't be used in return/yield/break expressions due to static not being added to fn ident_can_begin_expr. Could you fix this as well?

nikomatsakis

Mostly looks good. The last commit confused me a bit though. Left some requests for comments and other nits; let me know and will re-review.

nikomatsakis · 2018-01-29T18:58:36Z

src/librustc/middle/region.rs

@@ -467,9 +467,10 @@ impl<'tcx> Visitor<'tcx> for ExprLocatorVisitor {
    }

    fn visit_pat(&mut self, pat: &'tcx Pat) {
+        intravisit::walk_pat(self, pat);


What is motivating these changes? Presumably this is fixing a bug? If so, it'd be nice to have a test.

This makes the visitation match the visitation in RegionResolutionVisitor. I don't think this matters for patterns. It might mess up generators inside patterns somehow though.

nikomatsakis · 2018-01-29T19:02:55Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

@@ -533,15 +533,15 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> {
                    }
                }
                ty::TyGenerator(def_id, substs, _) => {
-                    // Try upvars first. `field_tys` requires final optimized MIR.
-                    if let Some(ty) = substs.upvar_tys(def_id, tcx).nth(field.index()) {
+                    // Try upvars first. `field_tys` requires the final optimized MIR


Nit: this comment seems slightly out of date, right? Maybe "Try pre-transform fields first (e.g., upvars, current state). The remaining fields (contained in field_tys) require the final optimized MIR.

I updated the comment.

nikomatsakis · 2018-01-29T19:04:35Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

-                    // Try upvars first. `field_tys` requires final optimized MIR.
-                    if let Some(ty) = substs.upvar_tys(def_id, tcx).nth(field.index()) {
+                    // Try upvars first. `field_tys` requires the final optimized MIR
+                    if let Some(ty) = substs.pre_transforms_tys(def_id, tcx).nth(field.index()) {
                        return Ok(ty);
                    }

                    return match substs.field_tys(def_id, tcx).nth(field.index()) {


That said, this code is basically dead code, right? That is, we don't run the type-checker after optimization? I guess it's here just in case we do, as a kind of sanity check? (But in that case, i'm not sure that the full set of type checks would pass...)

When I wrote it, only the InstCombine pass broke type checking. I disabled that and ran MIR type checking later anyway, for debugging purposes.

nikomatsakis · 2018-01-29T19:06:45Z

src/librustc_mir/dataflow/impls/borrowed_locals.rs

+use rustc::mir::visit::Visitor;
+use dataflow::BitDenotation;
+
+#[derive(Copy, Clone)]


Can we add a comment explaining what this analysis does and its overall purpose?

I added a comment here.

nikomatsakis · 2018-01-29T19:08:54Z

src/librustc_mir/transform/generator.rs

@@ -406,9 +418,9 @@ fn locals_live_across_suspend_points<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
            live_locals.union(&ignored.0);

            if !movable {
-                // For immovable generators we consider borrowed locals to always be live.
+                // For immovable generators we consider locals to always be live once borrowed.
                // This effectively makes those locals use just the storage liveness.


I'd appreciate a code example here, or somewhere =), explaining the meaning of these bits in a more intuitive way

I added some more comments. Let me know if that is good enough.

nikomatsakis · 2018-01-29T19:10:11Z

src/test/run-pass/generator/too-live-local-in-immovable-gen.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(generators)]


Can you add a comment explaining what this test is testing? My guess is that it's a local variable that wouldn't have to be considered live (because it is borrowed only after the yield)?

Or maybe I'm just confused.

I added a comment here.

…ust-lang#47482, rust-lang#46476

…ors. Fixes rust-lang#47736

nikomatsakis · 2018-01-31T19:24:26Z

@bors r+

bors · 2018-01-31T19:24:27Z

📌 Commit 6c66e11 has been approved by nikomatsakis

bors · 2018-02-03T17:28:14Z

⌛ Testing commit 6c66e11 with merge 3d292b7...

@nikomatsakis

Generator bugfixes r? @nikomatsakis

bors · 2018-02-03T20:23:14Z

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

rust-highfive assigned nikomatsakis Jan 29, 2018

Zoxc added 4 commits January 29, 2018 10:02

Make pattern visiting consistent

ad058cf

Make yield_in_scope_for_expr work with patterns. Fixes rust-lang#47758

dd3fa07

Make error reporting work on generator upvars. Fixes rust-lang#47793, r…

7a6f68c

…ust-lang#47805

Require yield types to be sized

77bc26f

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2018

Zoxc force-pushed the gen-fixes branch 2 times, most recently from 23addfb to e6ec6a5 Compare January 29, 2018 10:43

nikomatsakis reviewed Jan 29, 2018

View reviewed changes

Zoxc added 3 commits January 30, 2018 11:40

Allow access of the state field before the generator transform. Fixes r…

4325c63

…ust-lang#47482, rust-lang#46476

Force locals to be live after they are borrowed for immovable generat…

5647356

…ors. Fixes rust-lang#47736

The static keyword can now begin expressions

6c66e11

Zoxc force-pushed the gen-fixes branch from e6ec6a5 to 6c66e11 Compare January 30, 2018 11:52

nikomatsakis added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018

bors added a commit that referenced this pull request Feb 3, 2018

Auto merge of #47845 - Zoxc:gen-fixes, r=nikomatsakis

3d292b7

Generator bugfixes r? @nikomatsakis

bors mentioned this pull request Feb 3, 2018

Clarify shared file handler behavior of File::try_clone. #47958

Merged

bors merged commit 6c66e11 into rust-lang:master Feb 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generator bugfixes #47845

Generator bugfixes #47845

Zoxc commented Jan 29, 2018

petrochenkov commented Jan 29, 2018 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis Jan 30, 2018

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis Jan 29, 2018

nikomatsakis Jan 29, 2018

Zoxc Jan 30, 2018

nikomatsakis commented Jan 31, 2018

bors commented Jan 31, 2018

bors commented Feb 3, 2018

bors commented Feb 3, 2018

Generator bugfixes #47845

Generator bugfixes #47845

Conversation

Zoxc commented Jan 29, 2018

petrochenkov commented Jan 29, 2018 • edited Loading

nikomatsakis left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jan 31, 2018

bors commented Jan 31, 2018

bors commented Feb 3, 2018

bors commented Feb 3, 2018

petrochenkov commented Jan 29, 2018 •

edited

Loading