Duplicate variable checking in patterns

[joshqwang]

Causes Hazel to give an error message on patterns that contain duplicate variables, including labels in tuples.

Added a new get_duplicate_bindings function in Term.re.
Changed Duplicate type in Self.re to DuplicateLabel so it isn't confused with the new DuplicateVar type.

[codecov]

Codecov Report

❌ Patch coverage is 67.56757% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.17%. Comparing base (322405a) to head (0dd89fc).
⚠️ Report is 98 commits behind head on dev.

Files with missing lines	Patch %	Lines
src/language/statics/Info.re	40.00%	6 Missing ⚠️
src/language/statics/Self.re	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1752      +/-   ##
==========================================
+ Coverage   49.52%   50.17%   +0.65%     
==========================================
  Files         241      231      -10     
  Lines       26676    25513    -1163     
==========================================
- Hits        13210    12801     -409     
+ Misses      13466    12712     -754     

Files with missing lines	Coverage Δ
src/language/statics/Elaborator.re	81.67% <ø> (-0.39%)	⬇️
src/language/statics/Statics.re	86.99% <100.00%> (-0.32%)	⬇️
src/language/term/Pat.re	59.09% <100.00%> (-2.20%)	⬇️
src/language/statics/Info.re	45.42% <40.00%> (-3.31%)	⬇️
src/language/statics/Self.re	34.22% <0.00%> (-0.71%)	⬇️

... and 90 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Negabinary]

I'm getting a crash when I try typing

let (x,x

[joshqwang]

Should be fixed now.

[Negabinary]

I'm a little confused how the changes in statics.re work - specifically I don't understand how you managed to reuse the duplicate-label list to also do duplicate variables. Even though I don't understand it somehow almost every example I use to try and break it works including:

let (x, x=?) = ? in No error
let (x=(x, y)) = ? in No error

The one weird behaviour I could come up with is

let (x, x=(x, y)) = ? in

which puts the errors on the first and second x instead of the first and third.

Aside from that one issue in Statics.re I'm happy with the rest of this pr; I appreciate the bonus cleanup in elaborator/pattern!

[joshqwang]

I intended initially to have duplicate variable bindings and duplicate label bindings share the same list of duplicates.

In an earlier commit, let (x, x=?) = ? in produced a DuplicateVar error and a DuplicateLabel error.

But, when I did the code cleanup in Term.re, I ended up breaking that functionality, since the function I deleted and the Term.bindings function had more differences than I realized.

I'm a little unsure what correct behavior should be here. I have a version on my machine that fixed that functionality.

But, in these case, should the labels have no error on them?

[MaxCarroll0]

I'm a little unsure what correct behavior should be here. I have a version on my machine that fixed that functionality.
...
But, in these case, should the labels have no error on them?

The x = e pattern doesn't actually bind x to anything, so the left hand side (x) should never be in a duplicate binding error (so, as you say, it should not be a duplicate label in any of your examples: there are not two labels with name x, rather a label & a variable).

And, if the right hand side (e) has duplicate variables they should be errors (e.g. the 3rd x in the 3rd example is correctly detected as an error). BUT, the 2nd x in the 2nd example should not be a duplicate variable error (only one variable x is used, the other x is a label, not a variable).

It seems like you do indeed need to distinguish duplicate labels and duplicate variables?

[joshqwang]

Hopefully should be working now.

[MaxCarroll0]

Looks good to me

[cyrus-]

lgtm other than needing a few more tests

[dm0n3y]

@cyrus- ready for review

[dm0n3y]

@cyrus- resolved merge conflicts, checked that josh's example code above works, should be good to merge

[disconcision]

is it?

[dm0n3y]

no, at least not wrt dev, but there was a benign dev bug that was passing in duplicate expression-level labels into the pattern duplicates argument. benign bc the pattern duplicates arg would then later be overwritten. fixed that here while reviewing.

[disconcision]

should be a fn in listutil that does this

[dm0n3y]

i think you mean the dedup fn? that returns entire list deduplicated, this returns just the duplicates.