Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#9023 closed bug (fixed)

Error when using empty record update on binary pattern synonym

Reported by: Iceland_jack Owned by: cactus
Priority: normal Milestone: 7.8.3
Component: Compiler Version: 7.8.2
Keywords: Cc: cactus
Operating System: Linux Architecture: x86
Type of failure: None/Unknown Test Case: patsyn/should_compile/T9023
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Using an empty record update with patterns works here

pattern Singleton a = [a]

isSingleton :: [a] -> Bool
isSingleton Singleton{} = True
isSingleton _           = False

but fails in the follow example (there may exist a more minimal example)

pattern P a b = Just (a, b)
foo P{} = True

which outputs (GHC 7.8.2)

$ ghci -ignore-dot-ghci tmp.g2WZtTUkhJ.hs
GHCi, version 7.8.2: http://www.haskell.org/ghc/  :? for help
Loading package ghc-prim ... linking ... done.
Loading package integer-gmp ... linking ... done.
Loading package base ... linking ... done.
[1 of 1] Compiling Main             ( tmp.g2WZtTUkhJ.hs, interpreted )
Var/Type length mismatch: 
    [t{tv aIW} [sk]]
    []
Ok, modules loaded: Main.
*Main> 

Attachments (1)

fix_9023.patch (2.3 KB) - added by archblob 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by archblob

Status: newpatch

This is actually not an error, just a pprTrace that I think was left there accidentally. Everything actually type-checks and works.

comment:2 Changed 3 years ago by simonpj

Cc: cactus added
Owner: set to cactus

Let's not remove the trace. It signals that something quite unexpected is happening ,which should be investigated.

Gergo: could you look at why this is happening, since it involves pattern synonyms? Yell if you get stuck and need help from me.

Simon

comment:3 Changed 3 years ago by archblob

I was pretty uncomfortable doing that and wondered if something else was going on but I removed it because the caller also tests for the length mismatch and prints pretty much the same message if debug flags is set.

comment:4 Changed 3 years ago by archblob

I had no idea assertions were enabled only -DDEBUG, lesson learned. Here is the actual panic:

ghc-stage2: panic! (the 'impossible' happened)
  (GHC version 7.9.20140530 for x86_64-unknown-linux):
	ASSERT failed!
    file compiler/basicTypes/PatSyn.lhs line 268
    patSynInstArgTys P
    [t, t]
    [(t, t)]

comment:5 Changed 3 years ago by archblob

I'm sorry for hijacking this thread again but I'm trying to understand all of this stuff and tinkering with it does it best for me.

As I understand it, tcTyConAppArgs will look through synonyms but not through newtypes. The thing is it will also not get us the args we want in the case of nested application, so in this case for tuples, but the bug will manifest itself for any nested application.

I've attached a patch with a new fix and I'm wondering if you can take a look at it. The fix works for this test case but there may be things that i overlooked.

Changed 3 years ago by archblob

Attachment: fix_9023.patch added

comment:6 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

In 0a55a3cada2fea37586b1a270c1511ed9957dbd4/ghc:

Fix egregious instantiation bug in matchOneConLike (fixing Trac #9023)

We simply weren't giving anything like the right instantiating types
to patSynInstArgTys in matchOneConLike.

To get these instantiating types would have involved matching the
result type of the pattern synonym with the pattern type, which is
tiresome.  So instead I changed ConPatOut so that instead of recording
the type of the *whole* pattern (in old field pat_ty), it not records
the *instantiating* types (in new field pat_arg_tys).  Then we canuse
TcHsSyn.conLikeResTy to get the pattern type when needed.

There are lots of knock-on incidental effects, but they mostly made
the code simpler, so I'm happy.

comment:7 Changed 3 years ago by simonpj

Status: patchmerge
Test Case: patsyn/should_compile/T9023

archblob: you successfully provoked me into looking into this, but your patch was I'm afraid utterly wrong! The [Type] argument to patSynInstArgTys is positional: the order of the types in that list matters a lot! Moreover we need the instantiating types, not the free variables of the instantiating types. So if it worked for you it was a fluke.

But don't be discouraged! There is plenty more to do.

Austin: worth merging this to 7.8.3.

Simon

comment:8 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

comment:9 Changed 3 years ago by simonpj

Milestone: 7.8.3

comment:10 Changed 3 years ago by thoughtpolice

I had a merge conflict while merging this - looking into it...

comment:11 Changed 3 years ago by cactus

After fixing the conflicts, I've pushed a version on top of ghc-7.8 as wip/T9023.

comment:12 Changed 3 years ago by thoughtpolice

Resolution: fixed
Status: mergeclosed

Thanks Gergo, I've merged this.

comment:13 Changed 3 years ago by thoughtpolice

Milestone: 7.8.37.10.1

OK, so this couldn't go through cleanly because the Haddock change has a conflict that's proving to be extremely confusing:

https://github.com/haskell/haddock/commit/e56a8037c04a32922cb0951c66f64ecc6ebfecb3 https://github.com/haskell/haddock/commit/e811b00837d19340047ad83273b4aa2dc2534dd8

I have no idea how to reconcile these two commits, since the v2.14 haddock branch has diverged from the 7.8 branch, and I have no clue what Fuuzetsu etc have planned here. So I'm inclined to just leave this one out I'm afraid.

comment:14 Changed 3 years ago by simonpj

Yes, just leave it out. There are other things wrong with pattern synonyms and we aren't going to fix all of them in the 7.8 branch.

Simon

comment:15 Changed 3 years ago by cactus

That Haddock conflict is because of 7ac600d, which fixes #9175, which is also marked for merging anyway.

I've updated the wip/T9023 branch to contain both, then you can try merging from that again. I've also added a wip/T9023 branch to Haddock for the same purpose.

comment:16 Changed 3 years ago by thoughtpolice

This was merged into GHC 7.8 for 7.8.3 (also part of fixing #9175).

comment:17 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.8.3
Note: See TracTickets for help on using tickets.