Report internal errors more clearly

[sellout]

Overview

Internal errors are rather opaque. In #4463, we see

scratch/main> run main

   renameTo: duplicate rename

this adds more context, so now we have

scratch/main> run main

  ❗️

  You’ve discovered an internal bug in the Unison runtime!

    renameTo: duplicate rename

  See if one of these issues at
  https://github.com/unisonweb/unison/issues reflects what
  you’re seeing. If not, please open a new one:
  * 3625
  * 4463

Test coverage

There is a new transcript to replicate #4463, which does exercise this, but it will stop exercising it once that issue is fixed. But perhaps more reproductions will be added in the meantime …

I recommend reviewing commit-by-commit.

[aryairani]

Hey this looks great. Two thoughts:

1. I think it'd be better if we could pass a super short description of what could be causing it, too:

scratch/main> run main

  ❗️

  Sorry — I've encountered a bug in the Unison runtime.

    renameTo: duplicate rename

  Please check if one of these known issues matches your situation:

  * repeated variable names in pattern match cause "duplicate rename" error
    https://github.com/unisonweb/unison/issues/4463

  If not, please open a new one:
  https://github.com/unisonweb/unison/issues/new/choose

Sorry I ended up making this a bad example by closing 3625 as a duplicate 😅 but yes it should take a list like you have already done.

2. We probably shouldn't set runtime_tests_version to your personal branch; can those changes be PRed into the runtime tests, and then we update here to 0.0.4?

[sellout]

1. I think it'd be better if we could pass a super short description of what could be causing it, too:

Ah, yeah – I should have included this under “loose ends”. I was thinking we could fetch the title via the GitHub API, so the internalBug call sites still just have the issue number, but (as long as we don’t use displayException, which eschews IO) we could get the title dynamically.

2. We probably shouldn't set runtime_tests_version to your personal branch; can those changes be PRed into the runtime tests, and then we update here to 0.0.4?

Yeah, I was following the instructions in interpreter-tests.md. I figured I shouldn’t open a contribution until this PR is stabilized (e.g., you suggestions will require an update to my runtime-tests branch).

[aryairani]

Ah, yeah – I should have included this under “loose ends”. I was thinking we could fetch the title via the GitHub API, so the internalBug call sites still just have the issue number, but (as long as we don’t use displayException, which eschews IO) we could get the title dynamically.

yeah that could work; we just have to remember to apply short github titles in that case

Yeah, I was following the instructions in interpreter-tests.md. I figured I shouldn’t open a contribution until this PR is stabilized (e.g., you suggestions will require an update to my runtime-tests branch).

My bad. Ok so we'll stabilize the PR, then open the contribution for /runtime-tests, make a release, and update the PR, and then merge it?

[sellout]

I think we had a race condition here – I pushed commits addressing your comment (it now pulls issue descriptions from GH), but you were writing another message at like the same time.

Anyway, I just merged in trunk and resolved conflicts, so hopefully this is in a good state to move forward with review.

[aryairani]

Sorry @sellout, any ideas on the CI failures?

[aryairani]

Ok, on the transcript test failures it looks like just a git merge issue; so they just need to be re-run and checked in again; for the interpreter test failures I'm not sure?

[sellout]

Sorry @sellout, any ideas on the CI failures?

Yeah. I think there was behavioral conflict when I merged in master – hashes in stack traces previously got resolved to names, but now they’re not. One of the transcript diffs is due to that too (I ran transcripts before updating this PR, but forgot to check git diff after).

[aryairani]

I think we can use the "update transcripts" workflow to correct it automatically, but I can't seem to do it myself because the branch is in your repo.

[sellout]

I think there’s actually a regression here, but I need to check a couple things. Wasn’t able to figure it out looking at the post merge version of the PR, so I think there’s a chance the regression is on trunk.

I’ll move this into draft until I sort that out.

[aryairani]

erk, accumulating conflicts

[sellout]

I’ll get this one wrapped today.

[sellout]

Ok, this is finally passing. Before it can be merged,

1. https://share.unison-lang.org/@unison/runtime-tests/contributions/4 needs to be merged,
2. A new release of @unison/runtime needs to be cut, and
3. this PR needs to be updated to point at the new @unison/runtime-tests release instead of my branch.

[sellout]

Ok, I merged in trunk again[^1], and this should now run CI against the most recent https://share.unison-lang.org/@unison/runtime-tests/contributions/4, which has had main merged in.

[^1]: I mostly merged just to get a fresh commit to trigger CI – resolving conflicts on this PR is pretty trivial, so I’m not necessarily trying to keep it current.