[generator] Special cases for match guard when analyzing interior types in generators #75213

dingxiangfei2009 · 2020-08-06T08:29:42Z

This proposes one of ways to fix the mentioned issue. One cause of #72651 is that the interior type analysis misses out types of match pattern locals. Those locals are manifested as temporary borrows in the scopes of match arm guards. If uses of these locals appear after yield points, the borrows from them were not considered live across the yield points. However, this is not the case since the borrowing always happens at the very beginning of the match guard.

This calls for special treatment to analysis of types appearing in the match guard. Those borrows are recorded as the HIR tree is walked by InteriorVisitor and their uses are recorded whenever a yield point is crossed.

rust-highfive · 2020-08-06T08:29:45Z

r? @eddyb

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

dingxiangfei2009 · 2020-08-09T06:42:53Z

@eddyb I just know that reviewers are randomly assigned. Would you mind suggesting another reviewer if you find the current assignment is not suitable?

eddyb · 2020-08-16T17:19:48Z

Sorry! I should've asked someone to remove me from the list of reviewers, as I'm really behind.

r? @matthewjasper

tmandry · 2020-08-18T21:39:56Z

src/test/ui/generator/yielding-in-match-guards.rs

@@ -0,0 +1,29 @@
+// check-pass


This MVCE no longer ICEs for me, whereas this one does: #72651 (comment). No need to leave credit, just an issue number is enough.

(Feel free to also include this one if you can confirm it did ICE at one point, though).

The test case has been updated to refer to the said MVCE.

@tmandry Forgot to mention this. This test case compiles if the async functions are not used in fn main. As you can see, as soon as async function g is called in main, broken MIR ICE can be reproduced. I only found this out recently.

Ah, in that case let's put both test cases in!

bors · 2020-08-30T17:55:34Z

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

dingxiangfei2009 · 2020-08-30T18:10:03Z

Hi @matthewjasper would you mind a comment or feedback on this change?

tmandry · 2020-09-01T01:41:35Z

@rust-lang/wg-async-foundations has a meeting tomorrow focused on how this code works; after that would be a natural time for one of us to review this change.

crlf0710 · 2020-09-18T11:29:45Z

r? @tmandry

tmandry · 2020-09-22T22:28:23Z

compiler/rustc_typeck/src/check/generator_interior.rs

+ fn visit_arm(&mut self, arm: &'tcx Arm<'tcx>) {
+ if arm.guard.is_some() {
+ self.nested_scope_of_guards.push(<_>::default());
+ self.arm_has_guard = true;


It seems like we'd only want to set this while visiting the guard if expression, not the pattern.

tmandry · 2020-09-22T22:42:50Z

compiler/rustc_typeck/src/check/generator_interior.rs

+ "variable should be placed in scope earlier"
+ );
+ }
+ self.arm_has_guard = false;


We should reset back to the original value, which won't always be false (if there were a match nested inside the guard). Here's a (very contrived) example..

match x { Some(y) if match y.len() { 0 => false, l if l % 2 == 0 => true, _ => false } => (), _ => (), }

In fact, I'm tempted to say this flag is redundant and we should remove it. We can query current_scope_of_guards every time, and only find a way to optimize that if it's slow.

tmandry · 2020-09-22T22:48:29Z

compiler/rustc_typeck/src/check/generator_interior.rs

 let scope = self.region_scope_tree.var_scope(pat.hir_id.local_id);
 let ty = self.fcx.typeck_results.borrow().pat_ty(pat);
- self.record(ty, Some(scope), None, pat.span);
+ self.record(ty, Some(scope), None, pat.span, false);
+ if self.arm_has_guard {


We may want to check that this binding is actually introduced by the if guard, instead of by some arbitrary expression inside it.

That said, it's okay if we overapproximate the set of types inside our generator. Since it's very rare to have other bindings introduced inside a match guard, we could leave this as is. If we do that we should leave a comment explaining that this is an overapproximation.

I gave some thoughts about it. I think a better way is to use a separate and simple visitor that only walks the patterns in the arm, given that the arm has a guard. Most part of the original visit_pat will be kept the same.

tmandry · 2020-09-22T22:49:14Z

compiler/rustc_typeck/src/check/generator_interior.rs

@@ -134,6 +147,9 @@ pub fn resolve_interior<'a, 'tcx>(
 expr_count: 0,
 kind,
 prev_unresolved_span: None,
+ nested_scope_of_guards: <_>::default(),
+ current_scope_of_guards: <_>::default(),


naming suggestion: guard_bindings_set

tmandry · 2020-09-22T22:51:39Z

compiler/rustc_typeck/src/check/generator_interior.rs

@@ -134,6 +147,9 @@ pub fn resolve_interior<'a, 'tcx>(
 expr_count: 0,
 kind,
 prev_unresolved_span: None,
+ nested_scope_of_guards: <_>::default(),


naming suggestion: guard_bindings

tmandry · 2020-09-22T22:52:29Z

compiler/rustc_typeck/src/check/generator_interior.rs

+ /// such borrows can span across this yield point.
+ /// As such, we need to track these borrows and record them despite of the fact
+ /// that they may succeed the said yield point in the post-order.
+ nested_scope_of_guards: SmallVec<[SmallVec<[HirId; 4]>; 4]>,


The outer SmallVec could probably be of len 1, since nesting these would be extremely rare in practice.

tmandry · 2020-10-06T17:40:33Z

Friendly ping @dingxiangfei2009, are you still able to work on this?

dingxiangfei2009 · 2020-10-06T17:58:41Z

Sorry for keeping you waiting, @tmandry , I will apply your suggestions this morning.

tmandry · 2020-10-12T23:22:48Z

Sorry, I was on call last week but am taking a look now.

tmandry

Okay, after my comments I think this is good to go. Thanks for doing this!

tmandry · 2020-10-09T02:26:15Z

compiler/rustc_typeck/src/check/generator_interior.rs

@@ -53,7 +63,9 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> {
 yield_data.expr_and_pat_count, self.expr_count, source_span
 );

- if yield_data.expr_and_pat_count >= self.expr_count {
+ if guard_borrowing_from_pattern


nit: It may worth be restating what you said above in a comment here, i.e. that we need to record these regardless of where they appear in the post-order traversal.

tmandry · 2020-10-13T00:17:59Z

compiler/rustc_typeck/src/check/generator_interior.rs

+ self.guard_bindings.push(<_>::default());
+ }
+ self.visit_pat(&arm.pat);
+ if let Some(ref g) = arm.guard {


instead of accessing the arm's fields directly, could you destructure it so if new fields are added, this gets updated? e.g. let Arm { guard, pat } = arm;

This code is duplicating what normally goes in super_visit_arm() so it's helpful to have static checks like this.

That is a good idea. I also updated other field accesses to arm towards the end of this function. There are other fields in arm such as hir_id, but I am going to elide them for now.

If you use .. it sort of defeats the purpose of what I was suggesting. I'd rather list out all fields and assign them to _.

tmandry · 2020-10-13T00:25:11Z

compiler/rustc_typeck/src/check/generator_interior.rs

+ if arm.guard.is_some() {
+ self.guard_bindings.push(<_>::default());
+ }


This is duplicated below, I think maybe you meant to take it out?

Yes. I need to remove it.

so that it reflects the fact that borrowing these pattern locals is happening before any yield points in match guards

tmandry · 2020-10-13T19:31:19Z

@bors r+
@bors rollup=never in case there's a perf impact on async code

bors · 2020-10-13T19:31:21Z

📌 Commit 50627a3 has been approved by tmandry

bors · 2020-10-13T20:20:34Z

⌛ Testing commit 50627a3 with merge adef9da...

bors · 2020-10-13T22:13:01Z

☀️ Test successful - checks-actions, checks-azure
Approved by: tmandry
Pushing adef9da to master...

Always record reference to binding in match if guards When encountering a binding from a `match` pattern in its `if` guard when computing a generator's interior types, we must always record the type of a reference to the binding because of how `if` guards are lowered to MIR. This was missed in rust-lang#75213 because the binding in that test case was autorefed and we recorded that adjusted type anyway. Fixes rust-lang#78366

rust-highfive assigned eddyb Aug 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2020

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch from ade5e15 to 10762e9 Compare August 6, 2020 08:32

rust-highfive assigned matthewjasper and unassigned eddyb Aug 16, 2020

tmandry reviewed Aug 18, 2020

View reviewed changes

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch from 10762e9 to b4a321a Compare August 19, 2020 03:15

dingxiangfei2009 requested a review from tmandry August 19, 2020 05:43

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch 2 times, most recently from 31131dd to 9291c31 Compare August 20, 2020 06:49

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch from 9291c31 to f5e9e99 Compare August 30, 2020 18:08

rust-highfive assigned tmandry and unassigned matthewjasper Sep 18, 2020

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2020

tmandry reviewed Sep 22, 2020

View reviewed changes

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch from f5e9e99 to 29f61ac Compare October 7, 2020 04:58

dingxiangfei2009 requested a review from tmandry October 9, 2020 01:32

tmandry reviewed Oct 13, 2020

View reviewed changes

dingxiangfei2009 added 7 commits October 14, 2020 00:50

documentation fix

f9ccd39

also record the types of borrows from the pattern locals in match guards

7f5721c

so that it reflects the fact that borrowing these pattern locals is happening before any yield points in match guards

test derived from rust-lang#74961

323f079

rustfmt

66345d9

switch the test to an actual MCVE

df127b8

dedicated visitor for arm patterns

4a8ba7b

explanatory comments and fix guard binding stack

50627a3

dingxiangfei2009 force-pushed the yield-point-in-match-guard branch from 29f61ac to 50627a3 Compare October 13, 2020 16:52

dingxiangfei2009 requested a review from tmandry October 13, 2020 16:54

bors 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 Oct 13, 2020

JohnTitor mentioned this pull request Oct 13, 2020

Rollup of 10 pull requests #77904

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2020

bors merged commit adef9da into rust-lang:master Oct 13, 2020

rustbot added this to the 1.49.0 milestone Oct 13, 2020

jonas-schievink mentioned this pull request Oct 19, 2020

ICE: variable should be placed in scope earlier #78115

Closed

SNCPlay42 mentioned this pull request Oct 26, 2020

Always record reference to binding in match if guards #78393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[generator] Special cases for match guard when analyzing interior types in generators #75213

[generator] Special cases for match guard when analyzing interior types in generators #75213

dingxiangfei2009 commented Aug 6, 2020

rust-highfive commented Aug 6, 2020

dingxiangfei2009 commented Aug 9, 2020

eddyb commented Aug 16, 2020

tmandry Aug 18, 2020

dingxiangfei2009 Aug 19, 2020

dingxiangfei2009 Aug 27, 2020 •

edited

Loading

tmandry Sep 1, 2020

bors commented Aug 30, 2020

dingxiangfei2009 commented Aug 30, 2020

tmandry commented Sep 1, 2020

crlf0710 commented Sep 18, 2020

tmandry Sep 22, 2020

tmandry Sep 22, 2020

tmandry Sep 22, 2020

dingxiangfei2009 Oct 7, 2020

tmandry Sep 22, 2020

tmandry Sep 22, 2020

tmandry Sep 22, 2020

tmandry commented Oct 6, 2020

dingxiangfei2009 commented Oct 6, 2020

tmandry commented Oct 12, 2020

tmandry left a comment

tmandry Oct 9, 2020

tmandry Oct 13, 2020

dingxiangfei2009 Oct 13, 2020

tmandry Oct 13, 2020

tmandry Oct 13, 2020

dingxiangfei2009 Oct 13, 2020

tmandry commented Oct 13, 2020

bors commented Oct 13, 2020

bors commented Oct 13, 2020

bors commented Oct 13, 2020

[generator] Special cases for match guard when analyzing interior types in generators #75213

[generator] Special cases for match guard when analyzing interior types in generators #75213

Conversation

dingxiangfei2009 commented Aug 6, 2020

rust-highfive commented Aug 6, 2020

dingxiangfei2009 commented Aug 9, 2020

eddyb commented Aug 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 30, 2020

dingxiangfei2009 commented Aug 30, 2020

tmandry commented Sep 1, 2020

crlf0710 commented Sep 18, 2020

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

tmandry commented Oct 6, 2020

dingxiangfei2009 commented Oct 6, 2020

tmandry commented Oct 12, 2020

tmandry 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

tmandry commented Oct 13, 2020

bors commented Oct 13, 2020

bors commented Oct 13, 2020

bors commented Oct 13, 2020

dingxiangfei2009 Aug 27, 2020 •

edited

Loading