Skip to content
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

Always record reference to binding in match if guards #78393

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

SNCPlay42
Copy link
Contributor

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 #75213 because the binding in that test case was autorefed and we recorded that adjusted type anyway.

Fixes #78366

@rust-highfive
Copy link
Collaborator

r? @lcnr

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good to me, but not something I am too familiar with

@lcnr
Copy link
Contributor

lcnr commented Oct 26, 2020

r? @tmandry maybe

@rust-highfive rust-highfive assigned tmandry and unassigned lcnr Oct 26, 2020
tcx.mk_region(ty::RegionKind::ReErased),
ty::TypeAndMut { ty, mutbl: hir::Mutability::Not },
);
self.record(ref_ty, scope, Some(expr), expr.span, guard_borrowing_from_pattern);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only call to self.record that actually needs to ever pass true for guard_borrowing_from_pattern but I didn't want to make the recording of types any less cautious. If there are perf reasons to not want to record too many types in the generator witness we could rethink that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think you're right. We could try disabling the others in a follow-up.

let tcx = self.fcx.tcx;
let ref_ty = tcx.mk_ref(
// Use `ReErased` as `resolve_interior` is going to replace all the regions anyway.
tcx.mk_region(ty::RegionKind::ReErased),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe someone with better knowledge about lifetimes in the compiler should double-check this makes sense.

@tmandry
Copy link
Member

tmandry commented Oct 30, 2020

Looks good, thanks for this!

@bors r+

By the way, if you're interested in helping out more, I'd be delighted if you came by the wg-async-foundations stream on Zulip.

@bors
Copy link
Contributor

bors commented Oct 30, 2020

📌 Commit 8bf9abb has been approved by tmandry

@bors 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 30, 2020
@bors
Copy link
Contributor

bors commented Oct 30, 2020

⌛ Testing commit 8bf9abb with merge 8df58ae...

@bors
Copy link
Contributor

bors commented Oct 30, 2020

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 8df58ae to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 30, 2020
@bors bors merged commit 8df58ae into rust-lang:master Oct 30, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 30, 2020
@SNCPlay42 SNCPlay42 deleted the match-if-guard branch April 18, 2021 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE when using Matching, Async and Indexing
6 participants