@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

@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.

@aryairani Can you write up that alternative messaging you wanted when you get a chance please? Thanks :)

@ChrisPenner Just to be clear, this error can happen even nested deep within another definition? Or when can it happen? I don't think I ever got why it doesn't just apply at the top level.

Even if not top-level definitions they still need to be hashed

Yes, but does it need to be an error if it's not a top-level definition? I guess it's the same situation, where you could have two definitions that each had this equivalent cycle inside it, and then the top level definitions' hashes would inherit the issue?

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.

So then can we make it not an error?

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 see, so instead of being unable to change the internal variable name, because you end up with a new hash, you could change it.

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

Ok

@ChrisPenner Wdyt about

scratch/main>
🐞

Sorry, you've encountered a weird situation that we are aware of and are currently working on a fix for.  I'll explain what happened and how you can work around it.

The following cyclic definitions could not be completely ordered:
  <something indicating the line number?> bar
  <something indicating the line number?> foo
  <preferably without `User "..."`>
  <if line numbers not possible, can we tell when they're nested in another definition and include that name too? I'm guessing not, but I thought I'd ask.>
This happens when multiple definitions in a mutually recursive cycle have a very similar structure.

You can work around this by restructuring them to be less similar, e.g. by adding a pure expression to distinguish them, 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.

And then if it it's practical to print the message exactly once, that'd be better than twice.

@ChrisPenner We should probably have some kind of test that captures this behavior, even if we don't like the current behavior, or at least an easy to find issue about it.

Yes, but does it need to be an error if it's not a top-level definition? I guess it's the same situation, where you could have two definitions that each had this equivalent cycle inside it, and then the top level definitions' hashes would inherit the issue?

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.

So then can we make it not an error?

I don't know for sure, I'm not familiar with that part of hashing, but since we treat top-level definitions as basically just a let-rec I don't think I'm able to distinguish the two without essentially threading that info all the way through.

I'm hesitant to spend much more time on this tbh, it's already soaked up like a full week that could've been spent on implementing the fix.

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 see, so instead of being unable to change the internal variable name, because you end up with a new hash, you could change it.

Yes, I think so, but not reliably, since it would only allow you to change it if it caused a re-ordering, which isn't always going to be the case.

<something indicating the line number?> bar
<something indicating the line number?> foo
<preferably without User "...">
<if line numbers not possible, can we tell when they're nested in another definition and include that name too? I'm guessing not, but I thought I'd ask.>

I can't get the line number without specializing the whole algorithm to be Ann or have an Annotated class;
Similarly I can't get a good variable name without adding that to the ABT.Var class; (We have a name primitive for the other Var class, do you know why there are two Var classes?)

I also can't tell whether we're inside another definition without rewriting things, which I'm hesitant to do because it may introduce other bugs, and would likely take a similar chunk of time to just implementing the fix.

I know it seems like I'm not able to budge on a lot of things that make this error better, but that's mainly because I don't think it makes sense to invest the big chunk of work it'd take to do most of these things (which would later need to be undone) when we could instead invest that work in a proper fix. Or at the very least, I don't want to delay getting this validation in place. If someone uploads a problematic term to share that will generate possibly weeks of additional work.

Even if we update the error message later I'd really like to get this merged as soon as possible to prevent such a situation.

@ChrisPenner I think the info is already threaded through in the IsTop field; or at least I think it was. I'm not sure if it's still usable? But I also don't have any concrete reason to think it's not.

@ChrisPenner

do you know why there are two Var classes?

IIRC The less powerful ABT.Var was the one from a paper, and the bigger one was extra functionality that we added.

I know it seems like I'm not able to budge on a lot of things that make this error better

@ChrisPenner It's cool. Let's just do something reasonable for tests, get CI passing, and merge it, and then create a nice ticket to track adding the tie-breaking algorithm.

@ChrisPenner I think the info is already threaded through in the IsTop field; or at least I think it was. I'm not sure if it's still usable? But I also don't have any concrete reason to think it's not.

The actual hashComponents function is parameterized over the carrier functor, so I don't have this info at that level;
I could possibly pass it in from the outer hashTermComponents function, but even then I think the IsTop info from the Let is stripped off by the time we make it to that point; so I'd need to dig around a bit and route a bunch of new parameters through, which I'd thought was out of scope for a minimally invasive "quick-fix".

I think maybe we had different expectations for the fix; I was hoping to just get a patch in to stop Share from getting infected to buy time to work on other fixes (whether that be a rewrite to the hashing algorithm to fix the problem, or a refactoring to get better error messages). Are you wanting to do something more substantial the first time round? I'd really like to get the Share part deployed ASAP so let's see where we can compromise to help us both be happy :)

@aryairani

Let's just do something reasonable for tests, get CI passing, and merge it, and then create a nice ticket to track adding the tie-breaking algorithm.

Was there some additional testing on top of the incomplete-element-ordering-error.md transcript you wanted?

@ChrisPenner

Was there some additional testing on top of the incomplete-element-ordering-error.md transcript you wanted?

Not necessarily; when I wrote that, I didn't know what LSP test crash was exactly (4c8d17f), so I just meant if you had to remove some meaningful test to get it to pass, that we should replace it with some other test to fill the gap.

But I think we're good!