Using where instead of let should be fine, but the internal recursion needs to use go instead of the outer function name—it's the recursion crossing the unsafePartial call that causes the deoptimization (or, rather, would cause it, if the hack preventing it in purescript/purescript#4283 is removed).

Ah... In that case, let me rework how I'm doing that here.

How's that?

That works—and you don't need to eta-expand pop; unsafePartial down Nil should be sufficient.

But now that I'm bothering to test it, this approach does cause the compiler to introduce a thunk where there wasn't before. Not as big a deal as losing TCO, to be sure, but that does explain why the unsafePartials were where they were: the compiler will inline a thunk if it's immediately unthunked, but not if it's stored in a variable.

That works—and you don't need to eta-expand pop; unsafePartial down Nil should be sufficient.

But now that I'm bothering to test it, this approach does cause the compiler to introduce a thunk where there wasn't before. Not as big a deal as losing TCO, to be sure, but that does explain why the unsafePartials were where they were: the compiler will inline a thunk if it's immediately unthunked, but not if it's stored in a variable.

Darn... I'm assuming you're looking at the FFI to conclude that? And if so, is it the FFI of master or of your PR?

Ideas for how to update this without TCO being removed or the thunk being added?

My PR, but I'd expect the same on master.

Two ideas:

We could always add _ -> unsafePartial crash clauses to the relevant case expressions and not use unsafePartial elsewhere. That sort of violates the spirit of the Partial pattern. Honestly, though, I've been updating towards thinking the Partial pattern was a neat idea that doesn't work out all that well in practice.

Or, we could move the unsafePartial all the way up to pop, since pop doesn't call itself recursively, and not add Partial => constraints to the inner bindings. This would require using a where instead of a let for the bindings below pop, since those bindings need access to k which would be lambda-bound.

This all makes me a little sad. Efficient code shouldn't be this fragile.

We could always add _ -> unsafePartial crash clauses to the relevant case expressions and not use unsafePartial elsewhere.

I went with this approach instead.

@natefaubion Any thoughts on this? Merging this will enable Ryan to remove the "hacky" way of dealing with this particular case in purescript/purescript#4283

I'm not sure how often the pattern of unsafePartial case _ of comes up in practice.

I think unsafeCrashWith is definitely the way to go in this case. My opinion is that unsafePartial should only be used to discharge deliberately partial APIs like fromJust. Otherwise you should use unsafeCrashWith because you can add contextual information.

However... the real question here is whether unsafePartial case _ of should trigger TCO or not, which is difficult to answer, because there is no real spec for when TCO should specifically not trigger. From a syntactic/elaboration standpoint, it's clearly not what we consider a tail call. However, the optimizer treats unsafePartial as irrelevant, and removes it, which coincidentally means that it triggers TCO. Same thing happens if you do something like unsafeCoerce $ go .... This issue will come up with any sort of inlining optimizations. Should inlining open up opportunities for TCO, or should TCO only apply to source syntax?