Record/tuple patterns and new pattern match compiler and coverage checker

[Kmeakin]

• Adds record and tuple patterns.
• This required rewriting the pattern match compiler and coverage checker, following the algorithms outlined in Compiling pattern matching to good decision trees and Warnings for pattern matching, respectively
• The new algorithms can be extended in the future to support more pattern matching features, such as or-patterns and pattern guards.

Future work

• I have not done any optimization: there are a lot of allocations that could probably be eliminated.
• The coverage checker simply reports that a match is in-exhaustive, but does not explain which patterns are missing. A more sophisticated algorithm for calculating missing patterns in described in the second half of "Warnings for pattern matching"
• The implementation of shift is inefficient: we may want to add a new Weaken term kind instead
• The match compiler produces ugly code: it inserts extra let bindings in the body of default branches, instead of binding the default branch to a variable:

match x {
    0 => 0,
    y => y + 1,
}

will become

match x {
    0 => 0,
    _ => let y = x; y + 1,
}

[wezm]

I'll leave a full review of this to when Brendan returns from leave. Just noting one small issue with links in docs.

[brendanzab]

Still need to look deeper into this to try to understand the algorithm… there’s a bit to take in but it’s looking neat!

[brendanzab]

We could maybe pull out the rust upgrade into a separate PR if it helps clean things up.

[Kmeakin]

We could maybe pull out the rust upgrade into a separate PR if it helps clean things up.

That was the intention, not sure why github included those commits in the PR. Fixed it now.

[brendanzab]

I’m… not really understanding what’s going on with the Option<Option<StringId>>s for the then and else branches?

[Kmeakin]

In cases of the form

match x {
    true => ...,
    _ => ...,
}

or

match x {
    false => ...,
    _ => ...,
}

We have to push _ onto local_names, or else we get confusing bugs in distillation output. Alternatively, we could only distill matches to if-then-else syntax if they are of the form

match x {
    true => ...,
    false => ...,
}

[brendanzab]

Sorry for the slowness - taking a bit of time to get into this one

[brendanzab]

I’ve been looking at this more in depth locally. Doing a bit of messing around to understand it a bit more and tweaking the implementation a bit. While it’s big, I think it would be better to squash it into a single commit as it kind of only works all together.

One nit-pick: could we use the actual the actual type names rather than Self in impls and type definitions etc? I prefer to only use those when necessary in trait definitions.

[Kmeakin]

One nit-pick: could we use the actual the actual type names rather than Self in impls and type definitions etc? I prefer to only use those when necessary in trait definitions.

I think the preferred Rust style is to use Self wherever possible: https://rust-lang.github.io/rust-clippy/master/#use_self. Personally I prefer Self because:

• Usually less typing: most type names are more than 4 letters. Also allows you to avoid repeating lifetimes/type parameters
• One less piece of information you need to keep track off when writing methods/impls. Never do something yourself that the compiler can do for you!
• Type names may change frequently during development. Self means you don't have to update all the occurrences of the type name, and keeps diffs smaller

[brendanzab]

Why is it ok to duplicate these without updating scrut?

[Kmeakin]

A wildcard pattern leaves the scrutinee unchanged: it does not project further into it. It is duplicated ctor.arity() times because the algorithm relies on the matrix always being rectangular.

[brendanzab]

Oh, was more wondering about the type of the scrutinee - does it need to be updated for each pattern (seeing as it seems like this is based on the arity of the constructor) or is that unecessary?

[Kmeakin]

No, updating the type is also unnecessary. The scrutinee is left completely unchanged by wildcard patterns because wildcard patterns match anything: they don't inspect or transform the scrutinee in any way

[brendanzab]

I’ve looked into merging this, trying to do some cleaning up (see: Kmeakin/fathom@pattern-matching...brendanzab:fathom:pattern-matching-cleanups), but I don’t think it is ready yet. Some of the pattern matching compilation is self-contained and can be iterated on for improved clarity, but my big fear is how it impacts the handling of local bindings in the elaborator. The freshen_scrutinee and insert_extra_let methods are of particular concern, and I’m a little worried about taking on this burden going forward. Related to #488, it does make me think we might need a better approach to managing local bindings in the elaborator - eg. regarding adding/removing bindings, or with weakening.