Report internal errors more clearly#5661
Conversation
- `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.
84a2d1b to
9b252cc
Compare
|
Hey this looks great. Two thoughts:
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.
|
Ah, yeah – I should have included this under “loose ends”. I was thinking we could fetch the title via the GitHub API, so the
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). |
8384155 to
26428cd
Compare
- rephase - include GitHub issue titles when possible
`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`.
8754c04 to
37d85c8
Compare
yeah that could work; we just have to remember to apply short github titles in that case
My bad. Ok so we'll stabilize the PR, then open the contribution for |
|
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. |
|
Sorry @sellout, any ideas on the CI failures? |
|
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? |
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 |
|
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. |
|
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. |
|
erk, accumulating conflicts |
|
I’ll get this one wrapped today. |
9b0a193 to
5971074
Compare
|
Ok, this is finally passing. Before it can be merged,
|
|
Ok, I merged in trunk again1, 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. Footnotes
|
dolio
left a comment
There was a problem hiding this comment.
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).
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.
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.
Overview
Internal errors are rather opaque. In #4463, we see
this adds more context, so now we have
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.