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

RFC2229 Add missing edge case #87996

Merged
merged 2 commits into from
Aug 20, 2021
Merged

RFC2229 Add missing edge case #87996

merged 2 commits into from
Aug 20, 2021

Conversation

roxelo
Copy link
Member

@roxelo roxelo commented Aug 13, 2021

Closes #87988

This PR fixes an ICE where a match discriminant is not being read when expected. This ICE was the result of a missing edge case which assumed that if a pattern is of type PatKind::TupleStruct(..) | PatKind::Path(..) | PatKind::Struct(..) | PatKind::Tuple(..) then a place could only be a multi variant if the place is of type kind Adt.

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Aug 13, 2021
@roxelo roxelo changed the title Add missing multi variant cases Add missing multi variant case Aug 13, 2021
@roxelo
Copy link
Member Author

roxelo commented Aug 13, 2021

r? @nikomatsakis

Copy link
Member

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

Possible edge case

compiler/rustc_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved
@roxelo roxelo changed the title Add missing multi variant case RFC2229 Add missing edge case Aug 14, 2021
@m-ou-se m-ou-se added this to Migration crater run blockers in 2021 Edition Blockers Aug 16, 2021
@@ -265,6 +265,9 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
if def.variants.len() > 1 {
needs_to_be_read = true;
}
} else {
// If it is not ty::Adt, then it should be read
needs_to_be_read = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering what cases might be missing here -- one silly example might be matching on a constant of type () or something like that, which is kind of a no-op.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not too sure either. I am going to import all the match test cases that are here https://github.com/rust-lang/rust/tree/master/src/test/ui/match and then convert them into closure test cases and see if I found anything new

@nikomatsakis
Copy link
Contributor

@bors r+

We might as well land this PR in the meantime.

@bors
Copy link
Contributor

bors commented Aug 19, 2021

📌 Commit 2e61659 has been approved by nikomatsakis

@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 Aug 19, 2021
@Mark-Simulacrum
Copy link
Member

@bors p=1 - wanted for crater in #87066

camsteffen added a commit to camsteffen/rust that referenced this pull request Aug 19, 2021
RFC2229 Add missing edge case

Closes rust-lang#87988

This PR fixes an ICE where a match discriminant is not being read when expected. This ICE was the result of a missing edge case which assumed that if a pattern is of type `PatKind::TupleStruct(..) | PatKind::Path(..) | PatKind::Struct(..) | PatKind::Tuple(..)` then a place could only be a multi variant if the place is of type kind Adt.
@bors
Copy link
Contributor

bors commented Aug 19, 2021

⌛ Testing commit 2e61659 with merge ebedfed...

@bors
Copy link
Contributor

bors commented Aug 20, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing ebedfed to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2021
@bors bors merged commit ebedfed into rust-lang:master Aug 20, 2021
2021 Edition Blockers automation moved this from Migration crater run blockers to Completed items Aug 20, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 20, 2021
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
2021 Edition Blockers
  
Completed items
Development

Successfully merging this pull request may close these issues.

ICE: error with const in 2021 closure capture
8 participants