Panicked at `DotDotPat`

[edwin0cheng]

panicked at 'internal error: entered unreachable code: `DotDotPat` requires special handling and should not be mapped to a Pat.', crates\ra_hir_def\src\body\lower.rs:669:3

Code to reproduce :

fn foo(params: &[usize]) {
    match params {
        [ps @ .., _] => {}
        _ => {}
    }
}

[JoshMcguigan]

Thanks for the minimal reproduction. I added that panic here so I'll take a look at this one.

[JoshMcguigan]

So the reason I added that panic is because before that we were mapping the ast::Pat::DotDotPat to a Pat::Wild, which wasn't valid in the tuple case, since a wild (_) takes the place of a single item while the DotDotPat can take the place of multiple items. My thinking at the time was that it was always invalid to map a DotDotPat to a Pat::Wild for this reason.

    Slice { prefix: Vec<PatId>, slice: Option<PatId>, suffix: Vec<PatId> }

I'm not too familiar with the slice pattern types. Is the slice field supposed to contain the ellipsis if there is one? If that is true, can we replace slice: Option<PatId> with slice: bool?

As I've been thinking about this I've realized we could hit this panic another way (although at least this one is not valid rust source code):

struct Bar {
	a: bool,
}
fn foo(b: Bar) {
	match b {
		Bar { a: .. } => {},
	}
}

Really I think using the .. in any pattern location where it is not valid would trigger this panic. Any thoughts on a more general solution to this?

Also, the best way I know to test this pattern lowering is to add a test for the missing match arm diagnostic. Is there a better place to put tests for this?

[flodiebold]

Any thoughts on a more general solution to this?

We should just not panic on encountering it, and instead return Pat::Missing.

Is there a better place to put tests for this?

I'd just create an infer test (e.g. in regression.rs).

If that is true, can we replace slice: Option with slice: bool?

It's possible to bind to the .. in a slice pattern (as in the original example). We should instead lower it to a wildcard.

[JoshMcguigan]

It's possible to bind to the .. in a slice pattern (as in the original example). We should instead lower it to a wildcard.

My concern about lowering .. to a Wild is it would cause the following patterns to be lowered in the same way.

fn foo(params: &[usize]) {
    match params {
        // ellipsis
        [ps @ .., _] => {}
    }
}

fn foo(params: &[usize]) {
    match params {
        // underscore
        [ps @ _, _] => {}
    }
}

I could be misunderstanding here, but I'd think lowering both _ and .. to a Wild would not provide enough information to downstream code. For example, the missing match arm diagnostic would need to know whether all slices of length > 1 are covered (as in the first example), or only slices of lenth == 2 are covered (as in the second example).

[flodiebold]

No, the first one would be lowered to (pseudocode) Slice { prefix: [], slice: Some(Wild), suffix: [Wild] }, whereas the second one would be lowered to Slice { prefix: [], slice: None, suffix: [Wild, Wild] }.

In a slice pattern, .. has exactly the meaning of a wildcard pattern for the slice -- to match any slice (regardless of length).

[JoshMcguigan]

Thanks for helping me get up to speed here! Just to be sure I understand, shouldn't the first one (with the binding) be lowered to Slice { prefix: [], slice: Some(Pat::Bind { blah }), suffix: [Wild] }, whereas a .. without the binding would lower to Slice { prefix: [], slice: Some(Wild), suffix: [Wild] }?

In this case it seems there are three valid values for the slice field (within the Pat::Slice):

• None
• SomeWithBinding(Bind)
• SomeWithoutBinding

Am I still off track here?

[flodiebold]

Yes, if by "valid" you mean "can be written in surface Rust". The HIR representation would allow any arbitrary pattern for the slice pattern, even if that can't actually be written in normal Rust.

[JoshMcguigan]

Okay, I think I am getting it, just one final question (I hope 😄):

Is it the definition of the slice field in Pat::Slice that it represents the .. (or a x @ ..)? I want to make sure I am clear on the intention of the prefix vs slice vs suffix.

[flodiebold]

Yes, it represents the "variable-length" part of the slice pattern. You have a fixed-length prefix, then optionally a variable-length middle part (that is a pattern matching a slice), and then a fixed-length suffix again.

(Although it's not actually variable-length when the pattern is matching against an array...)

[JoshMcguigan]

Got it. In that case would it not be be valid to change the definition of the slice field in hir::Pat::Slice to represent the fact that it can only be in one of the three states above (doesn't exist, exists, exists with binding)? Then any arbitrary pattern other than .. or x @ .. would be lowered as part of the prefix (most likely) or suffix.

This seems like it would make hir::Pat::Slice more type safe, since we'd enforce at compile time that the slice is the correct thing? And I think malformed source code could still be lowered since arbitrary non-ellipsis patterns would be lowered into the prefix or suffix?

This actually seems very similar to the tuple pattern to me, since if I am understanding correctly, the suffix should be empty if there is no slice? In that case, perhaps it would be even better (from a type strictness standpoint) to change hir::Pat::Slice to { pats: Vec<PatId>, slice: Option<TypeRepresentingTheThreePossibleStates, usize> }, where the usize represents the position of the .. within the pats? I guess there is an issue with this suggestion, which already plays out for tuples, which is that it doesn't really allow lowering source code containing patterns that have two .. (even though this is not valid rust).

I really appreciate you taking the time to fill me in on this. I'm not proposing these changes as something I'd necessarily go out and do right away, but talking through this really helps me understand this more fundamentally. I think this background knowledge will really help when I get around to implement match statement exhaustiveness checking for slices.

[flodiebold]

I don't really see a good reason to constrain the type of the slice pattern; at least it wouldn't make type inference any easier.

perhaps it would be even better (from a type strictness standpoint) to change hir::Pat::Slice to { pats: Vec, slice: Option<TypeRepresentingTheThreePossibleStates, usize> }, where the usize represents the position of the .. within the pats?

It would be possible, but I think it'd mostly be a microoptimization.