Prevent hangs on internal errors

[rhendric]

Description of the change

Sometimes, when one of our unit tests results in an internal error, the entire test process hangs. This is mildly annoying, and I think this PR is the appropriate fix? I have to confess that I don't have as solid an understanding of how MonadBaseControl works as I'd like. Also, our whole internal error story is kind of confusing to me—we have internalError, but also internalCompilerError, and a few instances of error scattered about to boot. I don't know if further enabling using the exception-based error functions instead of encouraging more use of the monadic internalCompilerError is the right thing to do.

Buuuuuut... this fix is quick and not too disruptive to the status quo, so I'm proposing it!

---

Checklist:

• Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
• Added myself to CONTRIBUTORS.md (if this is my first contribution)
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[hdgarrood]

I think internalError is the one we should be using everywhere. error should of course be avoided because it doesn't include any context or the suggestion to report the crash. I don't think a monadic function for internal errors is the best option; if we've encountered an internal error there's no hope of handling it, so I'm not sure I see the benefit of plumbing it through a transformer stack; all we can do with it is tell the user about it and exit. Also, we often want to use internalError in places where there's no monadic context to throw errors in. But maybe I'm missing something; I see internalCompilerError was introduced as part of polykinds so maybe @natefaubion can point out if I've missed something that makes using the monadic context more appealing.

[hdgarrood]

The code looks reasonable but I'm a little bit hesitant to approve, just because of where this change is. Does this change only appear to affect the unit tests, or does it affect running the compiler normally as well?

[natefaubion]

I see internalCompilerError was introduced as part of polykinds so maybe @natefaubion can point out if I've missed something that makes using the monadic context more appealing.

The error will get tagged with the source position of the thing it was checking when the internal error occured. This is incredibly useful.

[hdgarrood]

Ok, that is very convincing. So maybe the approach should be “use internalCompilerError if you’re working in a context where it works, and internalError otherwise? Maybe we could rename them and/or add comments to make this a bit clearer.

[rhendric]

Does this change only appear to affect the unit tests, or does it affect running the compiler normally as well?

It does affect the compiler running normally, but in a less dramatic way. Based on my tests, prior to this patch, an internal error usually causes the compiler to print only the error itself and purs: thread blocked indefinitely in an MVar operation before exiting. With this patch, the internal error is printed, any warnings (or errors?) produced in other files are also printed, and there is no MVar message.

[hdgarrood]

Ok then, that also sounds like an improvement.