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

Expand potential inner Or pattern for THIR #98200

Merged
merged 3 commits into from
Aug 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion compiler/rustc_mir_build/src/thir/pattern/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,17 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> {
/// expands it.
fn push(&mut self, row: PatStack<'p, 'tcx>) {
if !row.is_empty() && row.head().is_or_pat() {
self.patterns.extend(row.expand_or_pat());
let pats = row.expand_or_pat();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to make expand_or_pat recursively expand or patterns instead?

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 couldn't make that one recursive but made a new function that is recursive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either the other use of expand_or_Pat is dead code now or it's still got that issue. Check whether it's dead by panicking inside of it instead of doing any logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be dead code because line 835, it's the only reason I couldn't replace the function altogether

Copy link
Contributor

Choose a reason for hiding this comment

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

That call may be dead after your change is what I'm thinking. Since you now expand all the nested Or patterns, line 835 may not encounter Or patterns anymore.

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 did as you said, it panicked. Newly added function is only called via fn push and that is called via line 846.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we now have one function that does the recursive work and one that doesn't. On what kind of matches did it panic? Should we call the new function at that site to reduce duplication and make it more general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other function just works like iterator right now, I tried to merge these two functions together but problem is, it's quite tricky because of line 838 if I remove expand_or_pat altogether and just call expand_and_extend compiler gives a warning unreachable pattern even though both of them should do the same thing at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

that is suspicious indeed and makes me confident that we should definitely make them work the same and figure out why they are problematic

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 I understand now why the compiler gives "unreachable pattern" err, to reach those places I need to access is_under_guard for each recursive call, but also push to the matrix has to happen after the call is_useful to trigger if v.is_empty() where the matrix has to be empty. Even if I throw all those out of the window I need to call is_useful for each v 🤷‍♂️

let mut no_inner_or = true;
for pat in pats {
if !pat.is_empty() && pat.head().is_or_pat() {
self.patterns.extend(pat.expand_or_pat());
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
no_inner_or = false;
}
}
if no_inner_or {
self.patterns.extend(row.expand_or_pat());
}
} else {
self.patterns.push(row);
}
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#[allow(unused_variables)]
#[allow(unused_parens)]
fn main() {
let x = "foo";
match x {
x @ ((("h" | "ho" | "yo" | ("dude" | "w")) | () | "nop") | ("hey" | "gg")) |
//~^ ERROR mismatched types
x @ ("black" | "pink") |
x @ ("red" | "blue") => {
}
_ => (),
}
}
11 changes: 11 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0308]: mismatched types
--> $DIR/inner-or-pat-2.rs:6:54
|
LL | match x {
| - this expression has type `&str`
LL | x @ ((("h" | "ho" | "yo" | ("dude" | "w")) | () | "nop") | ("hey" | "gg")) |
| ^^ expected `str`, found `()`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
15 changes: 15 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// run-pass

#[allow(unreachable_patterns)]
#[allow(unused_variables)]
#[allow(unused_parens)]
fn main() {
let x = "foo";

match x {
x @ ("foo" | "bar") |
(x @ "red" | (x @ "blue" | x @ "red")) => {
}
_ => (),
}
}
13 changes: 13 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat-4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#[allow(unused_variables)]
#[allow(unused_parens)]
fn main() {
let x = "foo";

match x {
x @ ("foo" | "bar") |
(x @ "red" | (x @ "blue" | "red")) => {
//~^ variable `x` is not bound in all patterns
}
_ => (),
}
}
11 changes: 11 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat-4.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0408]: variable `x` is not bound in all patterns
--> $DIR/inner-or-pat-4.rs:8:37
|
LL | (x @ "red" | (x @ "blue" | "red")) => {
| - ^^^^^ pattern doesn't bind `x`
| |
| variable not in all patterns

error: aborting due to previous error

For more information about this error, try `rustc --explain E0408`.
14 changes: 14 additions & 0 deletions src/test/ui/or-patterns/inner-or-pat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// run-pass

#[allow(unused_variables)]
#[allow(unused_parens)]
fn main() {
let x = "foo";
match x {
x @ ((("h" | "ho" | "yo" | ("dude" | "w")) | "no" | "nop") | ("hey" | "gg")) |
x @ ("black" | "pink") |
x @ ("red" | "blue") => {
}
_ => (),
}
}