Report internal errors more clearly #5661

sellout · 2025-04-23T05:30:17Z

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.

- `CompileExn` and `internalBug` have been moved to a new module and had duplicate implementations removed, - `internalBug` has a much richer message that should be more helpful to users, - `internalBug` now accepts a list of issue numbers relevant to that message, and - exception-related imports are now more explicit. Also, there are a couple open questions tagged with __TODO__.

Previously there were three ways of handling optional `Width` 1. distinct `x` and `xUnbroken` functions, 2 `Maybe Width`, and 3 `Width 0` implies “unbroken”. Since it’s difficult to use strictly positive numeric types[^1], this now consistently treats `width <= 0`[^2] as “unbroken”. [^1]: If it weren’t, I would have preferred something like `Maybe Positive`. [^2]: It would be nice to define `Width` using something like `Word` instead of `Int`, which would allow `width == 0`, but that has cascading impacts across functions like `length` and `replicate`, which makes it more intrusive. This also results in a couple inconsequential semantic changes to “unbroken” rendering: - “unbroken” is truly unbroken, not limited to `maxBound :: Int` - right-aligning `Pretty` output (which isn’t currently used anyway) would do nothing in the “unbroken” case, rather than padding the output to `maxBound :: Int` columns.

aryairani · 2025-04-24T19:10:30Z

Hey this looks great. Two thoughts:

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.

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 · 2025-04-24T22:18:23Z

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.

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

- rephase - include GitHub issue titles when possible

…rrors

`Unison.Runtime.Exception.die` is overridden in `Machine.Types`, and most call sites end up inheriting and using that instead. This just folds the minor alteration of that version into the one in `Exception`.

aryairani · 2025-05-05T20:38:08Z

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 · 2025-07-07T23:07:15Z

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 · 2025-07-08T10:27:49Z

Sorry @sellout, any ideas on the CI failures?

aryairani · 2025-07-08T10:37:45Z

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 · 2025-07-08T18:40:59Z

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

…rrors

aryairani · 2025-07-11T08:05:29Z

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 · 2025-07-11T14:11:55Z

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 · 2025-07-15T09:03:33Z

erk, accumulating conflicts

sellout · 2025-07-15T13:49:21Z

I’ll get this one wrapped today.

…rrors

sellout · 2025-08-28T18:35:58Z

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

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

…rrors

sellout · 2025-09-03T17:41:54Z

Ok, I merged in trunk again¹, 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.

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

dolio

Mostly looks good (pretty mechanical). Added some explanations about todo comments, and maybe some oddities (although those might have been fixed since I started reviewing).

.github/workflows/ci.yaml

unison-runtime/src/Unison/Runtime/Exception.hs

unison-runtime/src/Unison/Runtime/Stack.hs

unison-src/builtin-tests/interpreter-tests.sh

In unisonweb#5661, exceptions were made much prettier, but also accidentally double-prettified in cases where they were written to then read back off the stack. This reverts that double-prettification. Fixes unisonweb#5864.

sellout added 4 commits April 22, 2025 23:18

Add a transcript to reproduce unisonweb#4463

fe67b59

Improve messages for non-bug runtime exceptions

9b252cc

sellout force-pushed the improve-internal-errors branch from 84a2d1b to 9b252cc Compare April 23, 2025 19:59

sellout requested a review from a team as a code owner April 23, 2025 19:59

sellout force-pushed the improve-internal-errors branch from 8384155 to 26428cd Compare April 26, 2025 04:09

sellout added 3 commits April 26, 2025 12:33

Update runtime error messages

74f0440

- rephase - include GitHub issue titles when possible

Merge remote-tracking branch 'upstream/trunk' into improve-internal-e…

c1b6635

…rrors

Don’t redefine die

37d85c8

`Unison.Runtime.Exception.die` is overridden in `Machine.Types`, and most call sites end up inheriting and using that instead. This just folds the minor alteration of that version into the one in `Exception`.

sellout force-pushed the improve-internal-errors branch from 8754c04 to 37d85c8 Compare April 26, 2025 18:52

Merge remote-tracking branch 'upstream/trunk' into improve-internal-e…

cf3b9ac

…rrors

sellout marked this pull request as draft July 11, 2025 14:12

Merge remote-tracking branch 'upstream/trunk' into improve-internal-e…

5971074

…rrors

sellout force-pushed the improve-internal-errors branch from 9b0a193 to 5971074 Compare August 27, 2025 21:47

Extract prettyPanic function

167e60f

sellout marked this pull request as ready for review August 28, 2025 18:36

aryairani requested a review from dolio September 1, 2025 00:41

Merge remote-tracking branch 'upstream/trunk' into improve-internal-e…

f564bb9

…rrors

dolio requested changes Sep 3, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

unison-runtime/src/Unison/Runtime/Exception.hs Outdated Show resolved Hide resolved

unison-runtime/src/Unison/Runtime/Stack.hs Outdated Show resolved Hide resolved

dolio reviewed Sep 3, 2025

View reviewed changes

unison-src/builtin-tests/interpreter-tests.sh Outdated Show resolved Hide resolved

sellout added 2 commits September 3, 2025 15:33

Point the runtime tests at the new release

dff9658

Replace “TODO” comments with real comments

4b987bb

dolio approved these changes Sep 5, 2025

View reviewed changes

aryairani merged commit bbbb4a2 into unisonweb:trunk Sep 7, 2025
18 checks passed

ceedubs mentioned this pull request Sep 8, 2025

bug Failure message includes extra content #5864

Closed

sellout deleted the improve-internal-errors branch September 8, 2025 18:33

sellout mentioned this pull request Sep 10, 2025

Don’t prettify exceptions written to the stack #5868

Merged

Report internal errors more clearly #5661

Report internal errors more clearly #5661

Uh oh!

Conversation

sellout commented Apr 23, 2025

Overview

Test coverage

Uh oh!

aryairani commented Apr 24, 2025

Uh oh!

sellout commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aryairani commented May 5, 2025

Uh oh!

sellout commented Jul 7, 2025

Uh oh!

aryairani commented Jul 8, 2025

Uh oh!

aryairani commented Jul 8, 2025

Uh oh!

sellout commented Jul 8, 2025

Uh oh!

aryairani commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sellout commented Jul 11, 2025

Uh oh!

aryairani commented Jul 15, 2025

Uh oh!

sellout commented Jul 15, 2025

Uh oh!

sellout commented Aug 28, 2025

Uh oh!

sellout commented Sep 3, 2025

Footnotes

Uh oh!

dolio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sellout commented Apr 24, 2025 •

edited

Loading

aryairani commented Jul 11, 2025 •

edited

Loading