Opened 4 years ago

Closed 4 years ago

#9922 closed bug (fixed)

Partial type signatures + extensions panic

Reported by: monoidal Owned by: thomasw
Priority: normal Milestone: 7.10.1
Component: Compiler (Type checker) Version: 7.9
Keywords: Cc: thomasw
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D595
Wiki Page:

Description

Partial type signatures combined with the following GHC extensions cause a panic:

class C a where default f :: _       -- DefaultSignatures

instance Num Bool where negate :: _  -- InstanceSigs

deriving instance _                  -- StandaloneDeriving

Change History (8)

comment:1 Changed 4 years ago by thomasw

Differential Rev(s): D595
Owner: set to thomasw

comment:2 Changed 4 years ago by thomasw

Differential Rev(s): D595Phab:D595

comment:3 Changed 4 years ago by thomasw

Thanks for finding all the panics I caused :) I fixed it in Phab:D595.

Happy holidays.

comment:4 Changed 4 years ago by hvr

Milestone: 7.10.1
Status: newpatch

comment:5 Changed 4 years ago by simonpj

Austin: let's merge this to 7.10.1.

Thomas: I'm not very happy with the way that wild-card error reporting is done. I think we discussed this before, but left it on one side until it was all working. Which it now is. What I suggest is this:

Things I dislike:

  • I dislike the checkParitalTypeSignature and checkNoPartialType stuff in RdrHsSyn. The only checks that belong in RdrHsSym are ones that prevent you building a syntax tree at all. All other checks are best done later, when good error reporting is easier, and you can recover from errors.
  • I particularly hate the RnTypes.extractWildcards stuff. It's like a whole extra renaming pass over the type, changing HsWildCardTy to HsNamedWildCardTy with an Exact RdrName in it. Yuk!

Here is a possible plan:

  • Remove all the checking from RdrHsSyn, unless we can't build a syntax tree without it.
  • Collapse HsWildcardTy and HsNamedWildcardTy into one, with a boolean (or a Named/Anonymous flag) to distinguish.
  • Provide a specialised version of rnLHsType, perhpas rnLHsTypeWithWildCards, that does the inital pass to find the named wildcards and bring them into scope. This version is called in the places where you can have a type with wildcards, namely
    • TypeSig
    • ExprWithTySig
    • rnHsBndrSig
  • rnLHsTypeWithWildCards can work like this:
    1. Collect all the named wildcards, and bring them into scope. This is a simple, pure function.
    2. Call rnLHsType. When rnLHsType finds an anonymous wildcard, just make up a fresh name, rather than looking it up.
    3. Collect all the wildcards (named or anonymous) to get a [Name]; again a pure function
    4. Return the renamed type and the wildcard names That makes three passes, but each is simple. In fact (1) and (4) could perhaps be the same function, with a boolean flag to say which wildcards to return.
  • All this means that when rnLHsType is called directly (not via rnLHsTypeWithWildCards) on a type like _ -> Int, it will succeed, generateing a fresh name for the _. That's fine. In tc_hs_type we will find it is not in scope, so we can say "Unexpected wildcard in type", and the enclosing location information will nail down the details.
  • There are, I think, three places where HsWithBndrs is used: HsDecls.HsTyPats, HsDecls.RuleBndr, HsPat.SigPatIn. In the latter two I think that wildcards should be legal; in the first not so. (Do you have tests for all three?) So the caller of rnHsBndrSig should check for empty wildcards in the cases where there shouldn't be any. I this this is just in type/data family patterns.

I have not throught throught the extra-constraints wild card, but I think a similar plan should work.

Does this make sense? Might you do it? (To HEAD, of course.)

Thanks

Simon

comment:6 in reply to:  5 Changed 4 years ago by thomasw

Replying to simonpj:

Thomas: I'm not very happy with the way that wild-card error reporting is done.

I agree. I have thought about it too. An "opt-in to wildcards" approach would be much better than my current "opt-out" approach, which was bound to be incomplete.

I think we discussed this before, but left it on one side until it was all working. Which it now is. What I suggest is this: .. Does this make sense? Might you do it? (To HEAD, of course.)

Looks like a good plan. I think your suggestion to provide a separate rnLHsTypeWithWildCards will help me overcome many of the problems I had when I last tried to refactor the renaming of wildcards.

I'll start working on it as soon as the holidays are over, i.e. next week.

comment:7 Changed 4 years ago by Austin Seipp <austin@…>

In c9532f810a82c6395bc08fb77f2a895a50da86b5/ghc:

Fix panics of PartialTypeSignatures combined with extensions

Summary:
Disallow wildcards in stand-alone deriving instances
(StandaloneDeriving), default signatures (DefaultSignatures) and
instances signatures (InstanceSigs).

Test Plan: validate

Reviewers: austin

Reviewed By: austin

Subscribers: carter, thomie, monoidal

Differential Revision: https://phabricator.haskell.org/D595

GHC Trac Issues: #9922

comment:8 Changed 4 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Merged to ghc-7.10.

Note: See TracTickets for help on using tickets.