Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#10897 closed bug (fixed)

Incorrect ASSERT for buildPatSyn

Reported by: ezyang Owned by:
Priority: normal Milestone: 8.0.1
Component: Compiler (Type checker) Version: 7.11
Keywords: PatternSynonyms Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash Test Case: patsyn/T10897
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Consider the following two files:

-- A.hs
{-# LANGUAGE PatternSynonyms #-}
module A where
pattern Single :: a -> a
pattern Single x = x

-- B.hs
module B where
import A
Single y = True

When I build these using one-shot compilation using a debugged GHC (i.e. with ASSERTs) I get the following error:

[ezyang@hs01 ghc-quick3]$ inplace/bin/ghc-stage2 -c A.hs -fforce-recomp 
[ezyang@hs01 ghc-quick3]$ inplace/bin/ghc-stage2 -c B.hs -fforce-recomp 
ghc-stage2: panic! (the 'impossible' happened)
  (GHC version 7.11.20150918 for x86_64-unknown-linux):
        ASSERT failed! file compiler/iface/BuildTyCl.hs, line 210

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

I expanded the assert and discovered it was claiming the universal type variables of the matcher should be equal to the type variables of the pattern declaration. This assert cannot possibly be right: the matcher and the pattern declaration are typechecked separately and there's no reason that the local binders should actually be the same. The equality test here should be done up to alpha-renaming.

The other thing I found a bit puzzling was whether or not it mattered whether or not we used the local type variables from the matcher or the freshly bound ones. I suppose if we are consistent it shouldn't matter, so I don't think the code is buggy, just a bad ASSERT.

BTW: this assert problem doesn't show up with --make because the assert occurs during typechecking of interface files, and with --make we don't need to typecheck an interface file.

Change History (9)

comment:1 Changed 2 years ago by cactus

Without having looked at the details yet, I'd just like to mention that the reason I've put those ASSERTs in is because the plan, at one point, was to eventually get rid of most arguments to buildPatSyn and just reconstruct them from the matcher's idType. Unfortunately, I can't remember off-hand why that wasn't feasible.

comment:2 Changed 2 years ago by cactus

Keywords: PatternSynonyms added

comment:3 Changed 2 years ago by cactus

Hmm, it's starting to come back -- all those ifPat* type fields of IfacePatSyn are needed so that we can implement pprIfaceDecl and freeNamesIfDecl without the ability to turn the IfExtName of ifPatMatcher into a proper Id. That suggests we should look into it a bit more to see if it's valid when these self-confessed redundant fields aren't actually the same as the types recovered from the Id of the matcher.

comment:4 Changed 2 years ago by Matthew Pickering <matthewtpickering@…>

In 6c9258d/ghc:

Add test for #10897

comment:5 Changed 2 years ago by mpickering

Owner: cactus deleted
Test Case: patsyn/T10897

comment:6 Changed 2 years ago by mpickering

For me this fails with

Declaration for Single:
  Iface type variable out of scope:  t
Cannot continue after interface file error

I've had a poke around but the interface file stuff is a complete mystery for me. Someone else with more experience will have to look.

comment:7 Changed 2 years ago by Simon Peyton Jones <simonpj@…>

In 78248702/ghc:

Fix ASSERT in buildPatSyn, and T10897 test

This closes Trac #10897

comment:8 Changed 2 years ago by simonpj

Resolution: fixed
Status: newclosed

Works fine now.

comment:9 Changed 2 years ago by thomie

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