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.

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?

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.

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.

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.