Bool patterns

[wezm]

This PR adds initial support for boolean patterns. As a demonstration of their use I extended opentype.fathom to implement some of the version and magic number checks.

There are several things that have been deemed out of scope for this PR that will be investigated/addressed in follow up work:

• The check for sfnt version is commented out as it has a big impact on run time, although it does work.
• There are two FIXMEs attributed to Check for duplicate patterns in match #340
• When encountering a name in a pattern there is a special case to handle true and false to prevent all binding from resolving to their value. How to handle names in patterns #341 captures the alternatives to this for later resolution.

Other notes:

I'm open to better name suggestions for SplitConstBranches and its variants.

Fixes #340

[mikeday]

I'm curious why the sfnt version check has an impact when the other tests do not?

[wezm]

I'm curious why the sfnt version check has an impact when the other tests do not?

Looks like I've found what to look at next :)

[wezm]

You know what I think it was the test harness that was going slow. Now that I've updated the snapshots for the other differences it's not slow. Perhaps generating the diffs was slow for some reason.

Edit: The formats/data/opentype/aots/cmap8_font1.snap snapshot is 8421 lines. Perhaps it was diffing that, that slowed it down.

[brendanzab]

Maybe it would help reduce confusion to rename Split to ConstBranches? I was stuck in naming this originally, and it shows in some of the identifiers I picked.

It's confusing because insplit_telescope and split_const_branches I was trying to follow in an attempt to follow Rust's name for ‘uncons’, [T]::split_first. But then this collides with ‘split’ in the sense of a ‘case-split’.

Also wondering if we could make this type of split_const_branches follow [T]::split_first more closely with something like:

Option<((Const, ArcValue<'arena>), NextConstBranches<'arena>)>

using the following type:

#[derive(Clone, Debug)]
pub enum NextConstBranches<'arena> {
    Branches(ConstBranches<'arena>),
    Default(Closure<'arena>),
}

I dunno if that's better though - just an idea!

[wezm]

Ahh interesting. That explanation does connect some dots for me. Perhaps Split could also be CaseSplit but ConstBranches is does seem clearer.

Also wondering if we could make this type of split_const_branches follow [T]::split_first more closely with something like:

Option<((Const, ArcValue<'arena>), NextConstBranches<'arena>)>

This also makes more sense now. I didn't understand in the original code why the tuples were nested versus using a 3-tuple. Perhaps it's worth giving (Const, ArcValue<'arena>) a name, even if only an alias like Branch or ConstBranch to clean up the nesting a bit?

[brendanzab]

Yeah good idea about giving it a name!

[wezm]

I had a go at refactoring this to use

Option<((Const, ArcValue<'arena>), NextConstBranches<'arena>)>

but it got a bit messy. Perhaps I misunderstood something but I decided to mostly leave it alone. I did change

SplitConstBranches::Const(Const, ArcValue<'arena>, Split<'arena>)

to

SplitConstBranches::Branch(Branch<'arena>, Split<'arena>)

though, where Branch<'arena> is type Branch<'arena> = (Const, ArcValue<'arena>);

[brendanzab]

Yeah that's fair enough - I might have a go later if that's ok.

Eventually might be good to rename Split to Branches but it's probably good to take one thing at a time.

[wezm]

Yeah that's fair enough - I might have a go later if that's ok.

By all means go ahead 👍

[brendanzab]

Looking good! Thanks for the various comment fixes by the way! 😄

[brendanzab]

Looks good!

[brendanzab]

This is a bug in codespan. I'm pretty sure it was fixed but we might not have deployed it to crates.io

[wezm]

What's buggy about it?

[brendanzab]

It's missing the vertical line in the gutter

[brendanzab]

  ┌─ tests/fail/elaboration/non-exhaustive-patterns/match-duplicate.fathom:3:7
  │  
3 │ ╭ match true {
  │ │       ^^^^ patterns not covered