`renameTo: duplicate rename` pattern matching bug prevents ability to add/update functions

[jcwilk]

Steps to reproduce:

Add a function with a pattern match of [a@chars, b@chars] to your scratch.u and save it. eg:

failingFunction : [Text] -> (Text, Text)
failingFunction = cases
  [a@chars, b@chars] -> (a, b)
  _ -> ("", "")

You should get output like:

@jcwilk/advent-of-code-2023/main> 

  I found and typechecked these definitions in ~/unison/scratch.u. If you do an `add` or `update`,
  here's how your codebase would change:

    ⍟ These new definitions are ok to `add`:

      failingFunction : [Text] -> (Text, Text)

  renameTo: duplicate rename

@jcwilk/advent-of-code-2023/main>

The unique bit is renameTo: duplicate rename. If I then try to add or update then it acts as if there's nothing to update, even if there are many other stale functions as well:

@jcwilk/advent-of-code-2023/main> 

  I found and typechecked these definitions in ~/unison/scratch.u. If you do an `add` or `update`,
  here's how your codebase would change:

    ⍟ These new definitions are ok to `add`:

      failingFunction : [Text] -> (Text, Text)

  renameTo: duplicate rename

@jcwilk/advent-of-code-2023/main> add

  😶

  There's nothing for me to add right now.

  Hint: I'm currently watching for definitions in .u files under the ~/unison directory. Make sure
        you've updated something there before using the `add` or `update` commands, or use `load`
        to load a file explicitly.

@jcwilk/advent-of-code-2023/main> update

  😶

  There's nothing for me to add right now.

  Hint: I'm currently watching for definitions in .u files under the ~/unison directory. Make sure
        you've updated something there before using the `add` or `update` commands, or use `load`
        to load a file explicitly.

@jcwilk/advent-of-code-2023/main>

Attaching an example scratch.u file as well:

scratch.u.zip

[aryairani]

For @jcwilk we fixed this by not saying [a@chars, b@chars] ->.

@SystemFw also ran into a version of this error with this code:

go: NatMap Nat -> [Card] -> Nat
go copies = cases
  [] -> 42
  cards +: cards -> 0

we fixed by switching to card +: cards ->

[dolio]

Personally I think code like this should be an error caught during parsing or similar. I think it's likely to be a mistake most of the time. There are two possible meanings I know of:

1. The values of the two matching variables are supposed to be checked for equality to determine whether the pattern matches
2. One of the variables shadows the other

I expect this is currently 'supposed' to mean 2. But this isn't really useful. You could just use _ for the variable you don't want to refer to, and it would be clear which one isn't referred to. Which, by the way, which one actually is in scope?

So, presumably, people aren't intentionally using it for 2, which means most of the time this happens it is accidental, and would lead to incorrect variable references, because in some places they meant to refer to the one that got shadowed. Making it an error catches these mistakes.

Since we have 'universal' equality I guess it could mean 1, but that requires extra infrastructure and I'm not sure it's really that valuable. Would it really be intentionally used more often than being the same mistake as above (which could still coincidentally pass type checking and cause confusing behavior)?

[jcwilk]

Just to throw in my $0.02 - this was totally just due to my unfamiliarity with Unison so definitely don't count me as someone who wants or needs to be able to do this in particular.

I don't have enough of a familiarity with the relevant systems to have an opinion on the right way to make it work, but it would IMO be a huge improvement if it at least failed loudly and clearly since the way it failed was very opaque and wasn't at all clear it was happening due to the contents of the scratch file. Maybe until a proper fix is prioritized and sorted it'd be better to just fail more clearly?

Just my humble uneducated vote though, I'm sure y'all know the right thing to do here if anything.

[aryairani]

yeah, catching this during parsing and giving a nice error about the duplicate identifiers sounds good.

[etorreborre]

Just to add a +1 on this issue. My pattern match was

Grep p (Either.Left p) -> ... 

Where I accidentally reused p. Unfortunately I only got to see the renameTo: duplicate rename message when trying to compile my main function and that left me really puzzled.

Then I had to binary search my whole library to find the problematic pattern match :-) (thanks to @rlmark who directed me to this issue!).