Opened 14 months ago

Last modified 8 weeks ago

#11959 new bug

Importing doubly exported pattern synonym and associated pattern synonym panics

Reported by: darchon Owned by:
Priority: high Milestone: 8.2.2
Component: Compiler Version: 8.0.1-rc3
Keywords: PatternSynonyms Cc: mpickering
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D2132, Phab:D2133
Wiki Page:

Description

Given

{-# LANGUAGE PatternSynonyms, ViewPatterns #-}
module Pat (Vec2(Nil,(:>)), pattern (:>)) where

newtype Vec2 a = Vec2 {unvec2 :: [a]}

pattern Nil :: Vec2 a
pattern Nil = Vec2 []

pattern (:>) x xs <- ((\ys -> (head $ unvec2 ys,Vec2 . tail $ unvec2 ys)) -> (x,xs))
  where
    (:>) x xs = Vec2 (x:unvec2 xs)

and

{-# LANGUAGE PatternSynonyms #-}
module Main where

import Pat (Vec2(..),pattern (:>))

I get:

$ ghci Main.hs 
GHCi, version 8.0.0.20160411: http://www.haskell.org/ghc/  :? for help
[1 of 2] Compiling Pat              ( Pat.hs, interpreted )

Pat.hs:2:29: warning: [-Wduplicate-exports]
    ‘:>’ is exported by ‘(:>)’ and ‘Vec2(Nil, type (:>))’
[2 of 2] Compiling Main             ( Main.hs, interpreted )
ghc: panic! (the 'impossible' happened)
  (GHC version 8.0.0.20160411 for x86_64-unknown-linux):
	filterImports/combine
  (:>, :>, Nothing)
  (:>, Vec2{Vec2, :>, Nil}, Nothing)

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

Now, the warning about the duplicate export is of course correct. But the panic shouldn't happen when I try to import both the associated pattern synonym and the normal pattern synonym.

Change History (16)

comment:1 Changed 14 months ago by bgamari

Priority: normalhigh

Hmm, this is problematic.

For future reference, we typically call these patterns "bundled" and not "associated" as the latter would be confusing if we ever gain the ability to associate a pattern with a typeclass.

comment:2 Changed 14 months ago by bgamari

Differential Rev(s): Phab:D2132, Phab:D2133
Status: newpatch

comment:3 Changed 14 months ago by Ben Gamari <ben@…>

In 4f2afe1/ghc:

testsuite: Add test for #11959

Test Plan: Validate, expected to fail

Reviewers: goldfire, austin

Reviewed By: austin

Subscribers: thomie

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

GHC Trac Issues: #11959

comment:4 Changed 14 months ago by bgamari

Milestone: 8.0.18.0.2

We're going to bump this off to 8.0.2.

comment:5 Changed 12 months ago by Simon Peyton Jones <simonpj@…>

In 393928db/ghc:

Fix renamer panic

This patch fixes Trac #12216 and #12127.  The 'combine' function
in 'imp_occ_env' in RnNames.filterImports checked for an empty
field-selector list, which was (a) unnecessary and (b) wrong.

I've elaborated the comments.

This does NOT fix #11959 which is related but not the same
(it concerns bundling of pattern synonyms).

comment:6 Changed 10 months ago by bgamari

Status: patchmerge

comment:7 Changed 10 months ago by bgamari

Resolution: fixed
Status: mergeclosed

comment:8 Changed 10 months ago by bgamari

Resolution: fixed
Status: closednew

But comment:5 doesn't actually fix this issue. Reopening.

comment:9 Changed 10 months ago by bgamari

Cc: mpickering added

mpickering, do you suppose you could have a look at this?

comment:10 Changed 10 months ago by bgamari

mpickering has said that he doesn't have enough time to look at this in the near future so I may try to dust off my previous fix, Phab:D2133.

comment:11 Changed 9 months ago by bgamari

On Phab:D2133 Simon said (paraphrasing),

Consider these two export lists

  1. module M( Vec2( Nil, Cons ) ) where ...
  1. module M( Vec2( Nil, Cons ), pattern Cons ) where ...
    These should generate precisely the same .hi file, but they don't. They look like
    a. {{{
       exports:
         Vec2{Cons Nil}
    
  2. {{{ exports:

Cons Vec2{Cons Nil}

}}}

It's not entirely clear to me that this is true. Consider these two export lists,

  1. module M( Vec2( Nil, Cons ) ) where ...
  1. module M( Vec2, pattern Nil, pattern Cons ) where ...

These two mean quite different things for the end-user when they go to import this module. Namely, they will need to use, respectively,

  1. import M(Vec2(Cons))
  2. import M(Cons)

Note that the user cannot import the definitions from export list (1) with import list (2) and vice versa.

This means that if the user gives us export list (b) from above and we map it to the same exports as produced by export list (a), we have made a non-obvious decision for the user, who now cannot use import list (2) to import Cons from M. It is possible we have even acted against M's author's will, which was to allow both import lists.

While we're discussing this, I'd also point out that there is a bit of inconsistency in how we treat bundled things. Consider,

module AModule ( ARecord(..) ) where
data ARecord = ARec { hello :: Int }

In this case one could use either

import AModule (hello)  -- or...
import AModule (ARecord(hello))

to import hello (although the ARec constructor must be imported in "bundled" style). While this makes some amount of sense (since selectors can be easily thought of as either free-floating functions or parts of data), it does make me wonder whether we should be equally liberal in the case of patterns.

comment:12 Changed 9 months ago by mpickering

I don't think your comment is correct Ben. You say that

Note that the user cannot import the definitions from export list (1) with import list (2) and vice versa.

But if the pattern synonym is bundled then you can import it with the pattern keyword just like record selectors.

comment:13 Changed 9 months ago by bgamari

Ahh, right; I likely omitted the pattern keyword in my experiment. Thanks for catching this, Matthew.

comment:14 Changed 7 months ago by bgamari

Milestone: 8.0.28.0.3

Bumping off to 8.0.3.

comment:15 Changed 4 months ago by bgamari

Milestone: 8.0.38.2.1

At this point it is rather unlikely that there will be an 8.0.3. Re-milestoning.

comment:16 Changed 8 weeks ago by bgamari

Milestone: 8.2.18.2.2

At this point this isn't really a regression. Bumping off to 8.2.2 since we have bigger issues to handle for the already-late 8.2.1.

Note: See TracTickets for help on using tickets.