Fail hashing if components are ambiguously ordered.

[ChrisPenner]

Overview

A temporary measure to prevent components with ambiguous element orderings from propagating through the ecosystem; particularly in Share.

More context on this issue: #2787

This doesn't fix the issue, but it does prevent the problematic situation caused by a single hash having multiple semantically different definitions; which would cause completely erroneous behaviour in certain cases.

I analyzed Share and no such definitions are in the ecosystem at the moment, this change should keep it that way.

@aryairani we'd talked about different approaches for implementing this without making large changes to the codebase;
you seemed hesitant to thread errors all the way through only to (hopefully) remove them later, so instead I inserted calls to error, which should have the same effect but with a slightly worse user experience.

Here's what happens if you write a component like this:

foo = do bar ()
bar = do foo ()

Uh oh, an unexpected exception brought the process down! That should never happen. Please file a bug report.

Here's a stringy rendering of the exception:

  🐞

  Hashing failed because cyclic definitions because the definitions could not be completely ordered.
  This happens when multiple definitions in a mutually recursive cycle are identical except
  for references to other elements in the same cycle.
  If all elements are identical, consider simple recursion instead of mutual recursion,
  If mutual recursion is required, you may disambiguate identical definitions by
  adding a dummy comment like:
  _ = "this is the foo definition"


  This is a Unison bug and you can report it here:

  https://github.com/unisonweb/unison/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+E253299+

  Bug reference: E253299

  If there's already an issue with this reference, you can give a 👍
  on the issue to let the team know you encountered it, and you can add
  any additional details you know of to the issue.

  CallStack (from HasCallStack):
    error, called at src/Unison/Hashing/V2/ABT.hs:42:14 in unison-hashing-v2-0.0.0-8lPXbJPlm6sDj9SGQtF5JE:Unison.Hashing.V2.ABT
    crashOnHashingFailure, called at src/Unison/UnisonFile.hs:281:17 in unison-parser-typechecker-0.0.0-E7FuwoDrRadBVo69eIX64V:Unison.UnisonFile
    typecheckedUnisonFile, called at src/Unison/FileParsers.hs:323:7 in unison-parser-typechecker-0.0.0-E7FuwoDrRadBVo69eIX64V:Unison.FileParsers

It prevents the user from adding/updating such a component, which accomplishes the goal, but in a bit of a scary way.

Implementation approach and notes

• Adds a detection step during component element ordering where we detect if multiple elements have identical ordering-hashes, meaning the element ordering is ambiguous.
• Fail hashing and hash validation if this is detected.

Interesting/controversial decisions

There's some nuance, we could choose allow components where all elements are identical, since those don't technically cause any problems, but it's somewhat annoying and difficult to check if this is the case for non-trivial components, and AFAIK there's never a good reason to write such a component since you could just recurse instead.

This also is the first introduction of the idea that hashing a component could fail; which is undesirable.
We plan to lift this restriction by introducing a complete ordering of component elements in the future, but it involves implementing a whole new component ordering algorithm. (see #2787 )

Test coverage

• Tested by running it on every multi-element component on Share.
• Transcript test

Loose ends

It would be nice to properly resolve this issue by implementing a complete element ordering algorithm.

[ChrisPenner]

This is the change that allows Share to catch these.

[aryairani]

@ChrisPenner
Heads up that one of the LSP tests is failing
unison-cli/tests/Unison/Test/LSP.hs annotationNesting

      ( "let-rec blocks",
        [here|
term = let
  x a = a && y true
  y b = b && x true
  x true && y true
|]
      )

Not sure why this would be an issue when they're not top level definitions

[ChrisPenner]

@aryairani

Not sure why this would be an issue when they're not top level definitions

Even if not top-level definitions they still need to be hashed, and AFAIK we treat top-level bindings just as one big let-rec.

I think it won't run into quite the same problems, since I'd guess that the variable names are going to be the same on every hash since they're baked in, and you can't have external references into that let-rec, so that problem doesn't apply.

The one thing would be that this implies if you load a term, then change a variable name, the hash could change (if you had a 3 element component where element ordering matters), which would be annoying, but not entirely problematic.

I'll fix this test, but I don't think we should change the behaviour.