Opened 20 months ago

Last modified 9 months ago

#14765 new bug

Levity polymorphism panic

Reported by: dfeuer Owned by:
Priority: normal Milestone: 8.10.1
Component: Compiler Version: 8.5
Keywords: LevityPolymorphism Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash or panic Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

I imagine I have made a mistake, but GHC shouldn't panic!

{-# language TypeInType, ScopedTypeVariables, MagicHash #-}

import GHC.Exts (TYPE, Proxy#, proxy#)

fold :: forall rep a (r :: TYPE rep).
          (r -> a -> Proxy# r -> r) -> (Proxy# r -> r) -> [a] -> r
fold f k [] = k proxy#
fold f k (x : xs) = fold f (f (k proxy#) x) xs

This gives me

ghc-stage2: panic! (the 'impossible' happened)
  (GHC version 8.5.20171211 for x86_64-unknown-linux):
        splitFunTy
  ()
  Call stack:
      CallStack (from HasCallStack):
        callStackDoc, called at compiler/utils/Outputable.hs:1150:37 in ghc:Outputable
        pprPanic, called at compiler/types/Type.hs:921:30 in ghc:Type

Change History (8)

comment:1 Changed 20 months ago by goldfire

Keywords: LevityPolymorphism added

comment:2 Changed 19 months ago by simonpj

Richard, this is a continuation of #12709. During desugaring we have

f a b

Turns out that there's a levity-polymophism error in (f a), so (bizarrely) dsWhneNoErrs returns (). Then the desugarer tries to apply that to b, and that fails because () isn't a function type.

Hhe cure (for #12709) was ugly enough; now we find that it doesn't acutally work! Replacing an expression by () doesn't make sense -- the types are wrong. I can see two solutions (there may be others):

  • Instead of returning () return undefined @ty where ty is the needed type.

But the whole dsWhenNoErrs machinery is very heavy; it calls tryM (which in turn allcoates fresh IO refes for errors etc) on every single application node. The cure is worse than the disease. So instead

  • We could define desugarer-only version of mkCoreApp, which doesn't blow up on levity-polymorphic args. (Maybe it builds an ordinary App, say.) It's a bit of code to duplicate but not much, and it's easy to understand.

(Alternatively we could make isUnliftedType return False for a levity-poly type. That would be even simpler.)

What do you think?

comment:3 Changed 19 months ago by goldfire

A few thoughts:

  • I really don't like having isUnliftedType return False for levity polymorphism. Panicking is the right behavior. Anything else might mask bugs.
  • How would the new mkCoreApp work? Would it check its argument for levity polymorphism and then react accordingly? This check is not necessarily cheap, and it would be redundant with other checks for LP.
  • We could make the new mkCoreApp monadic and have it do the only LP check. But then it would need to take the undesugared argument, so that error messages can be at all sensible. That would work in several cases, but sometimes it's not so easy.
  • You're right that returning () is gross. Perhaps returning undefined is better.
  • dsWhenNoErrs uses askNoErrsDs which does indeed use tryM. But there's no great reason askNoErrsDs needs to allocate fresh refs for messages, given that messages are always propagates in askNoErrsDs. Perhaps we could just make that more efficient by not using tryM.

Bottom line: I don't love the current design. But the devil is in the details, and I'm not yet convinced about any new design either. We'd have to try it and see. (I am convinced enough that undefined is better than what we have now.)

comment:4 Changed 19 months ago by simonpj

More thoughts

Perhaps we could just make that more efficient by not using tryM.

No: we need to know if there are any error messages from the enclosed thing.

I really don't like having isUnliftedType return False for levity polymorphism.

Actually I've decided I do! The compiler should panic if there is no other alternative. Here there is -- and Lint will catch the error shortly afterwards in a much more civilised way. It's such a simple fix.

How would the new mkCoreApp work?

Just like the current one, but without panicing in isUnliftedType, and with the entire apparatus introduced by #12709. That is, build a term that may not pass Lint; but it doesn't matter because we are going to report LP errors anyway and stop.

So a "new mkCoreApp" duplicates code, but in exchange means that all other uses of isUnliftedType can panic if you still feel strongly that they should.

comment:5 Changed 19 months ago by goldfire

So are you suggesting that this new mkCoreApp doesn't use isUnliftedType, but instead re-creates it? Who reports the LP error? I suppose that could remain the same as it is right now... but then the check in mkCoreApp will be redundant.

To be clear, I am OK with having a special mkCoreApp that doesn't panic on an LP argument used in the desugarer, as long as isUnliftedType still panics. My worry here is more about performance, but perhaps it would be an improvement over the status quo.

comment:6 Changed 15 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be addressed in GHC 8.6.

comment:7 Changed 9 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

comment:8 Changed 9 months ago by osa1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.