Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#10487 closed bug (fixed)

DeriveGeneric breaks when the same data name is used in different modules

Reported by: andreas.abel Owned by: osa1
Priority: highest Milestone: 8.0.1
Component: Compiler Version: 7.10.1
Keywords: Generics Cc: kosmikus, omeragacan@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: T10487
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1081
Wiki Page:

Description

That's the error you like too see, or rather not. ;(

[180 of 289] Compiling Agda.TypeChecking.Serialise ( src/full/Agda/TypeChecking/Serialise.hs, dist-2.4.2.4/build/Agda/TypeChecking/Serialise.o )

src/full/Agda/TypeChecking/Serialise.hs:1:1:
    Duplicate instance declarations:
      instance GHC.Generics.Datatype Agda.TypeChecking.Serialise.D1Name
        -- Defined at src/full/Agda/TypeChecking/Serialise.hs:1:1
      instance GHC.Generics.Datatype Agda.TypeChecking.Serialise.D1Name
        -- Defined at src/full/Agda/TypeChecking/Serialise.hs:1:1

src/full/Agda/TypeChecking/Serialise.hs:1:1:
    Duplicate instance declarations:
      instance GHC.Generics.Constructor
                 Agda.TypeChecking.Serialise.C1_0Name
        -- Defined at src/full/Agda/TypeChecking/Serialise.hs:1:1
      instance GHC.Generics.Constructor
                 Agda.TypeChecking.Serialise.C1_0Name
        -- Defined at src/full/Agda/TypeChecking/Serialise.hs:1:1

Problems:

  • no position in file
  • system generated names D1Name, C1_0Name
  • obvious bogus (same instance cannot be defined twice at same location)
  • leads me to suspect bug in Generics rather than in my code

Reproduce:

  • git clone agda/agda from github,
  • checkout this commit

https://github.com/agda/agda/commit/46823536559276807639488eae151a0e855fdb95

  • and try to compile with
      make install-bin
    

Change History (21)

comment:1 Changed 4 years ago by andreas.abel

It is also a proper bug.

module M where

data Name = Name
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE StandaloneDeriving #-}

module N where

import GHC.Generics
import qualified M

data Name = Name

deriving instance Generic M.Name
deriving instance Generic Name

Error:

[1 of 2] Compiling M                ( M.hs, interpreted )
[2 of 2] Compiling N                ( /home/abel/play/haskell/bugs/Generics/N.hs, interpreted )

/home/abel/play/haskell/bugs/Generics/N.hs:1:1:
    Duplicate instance declarations:
      instance Datatype N.D1Name
        -- Defined at /home/abel/play/haskell/bugs/Generics/N.hs:1:1
      instance Datatype N.D1Name
        -- Defined at /home/abel/play/haskell/bugs/Generics/N.hs:1:1

/home/abel/play/haskell/bugs/Generics/N.hs:1:1:
    Duplicate instance declarations:
      instance Constructor N.C1_0Name
        -- Defined at /home/abel/play/haskell/bugs/Generics/N.hs:1:1
      instance Constructor N.C1_0Name
        -- Defined at /home/abel/play/haskell/bugs/Generics/N.hs:1:1
Failed, modules loaded: M.

Qualified names are not handled properly by DeriveGeneric.

comment:2 Changed 4 years ago by andreas.abel

Summary: Unhelpful error from instance GenericDeriveGeneric breaks when the same data name is used in different modules

comment:3 Changed 4 years ago by dreixel

I'm afraid this is due to the simplistic way in which the names for the helper datatypes of the representation are generated. Should be easy to fix, though.

comment:4 Changed 4 years ago by simonpj

Milestone: 7.12.1
Owner: set to dreixel
Priority: normalhighest

Thanks Pedro -- was that an offer?

Simon

comment:5 Changed 3 years ago by bgamari

dreixel, do you intend on putting together a patch to fix this?

comment:6 Changed 3 years ago by simonpj

Andres Loh has offered to take over Generic and DeriveAnyClass, but only at the end of the summer.

I'm not sure what his Trac alias is.

Simon

comment:7 Changed 3 years ago by osa1

Cc: omeragacan@… added
Differential Rev(s): D1081

comment:8 Changed 3 years ago by osa1

Owner: changed from dreixel to osa1

comment:9 in reply to:  6 Changed 3 years ago by hvr

Cc: kosmikus added

Replying to simonpj:

Andres Loh has offered to take over Generic and DeriveAnyClass, but only at the end of the summer.

I'm not sure what his Trac alias is.

It's kosmikus

comment:10 Changed 3 years ago by thomie

Differential Rev(s): D1081Phab:D1081

comment:11 Changed 3 years ago by osa1

Simon asked for a concrete example and some explanations on Phabricator thread so here it is:

(I'm just learning this stuff so there may be mistakes)

When we ask GHC to derive a Generic instance, it generates some instances other than Generic, and it also generates new data types for constructors of the type(and I think also for fields of constructors).

Let's say I have this:

module N where

import GHC.Generics

data Name = N1 String | N2 Int

deriving instance Generic Name

GHC generates these instances:

  instance GHC.Generics.Generic N.Name where
    GHC.Generics.from (N.N1 g1_a17z) = ...
    GHC.Generics.from (N.N2 g1_a17A) = ...
    GHC.Generics.to (...) = ...
    GHC.Generics.to (...) = ...

  instance GHC.Generics.Datatype N.D1Name where
    GHC.Generics.datatypeName _ = "Name"
    GHC.Generics.moduleName _ = "N"

  instance GHC.Generics.Constructor N.C1_0Name where
    GHC.Generics.conName _ = "N1"

  instance GHC.Generics.Constructor N.C1_1Name where
    GHC.Generics.conName _ = "N2"

and these new data types:

    N.D1Name
    N.C1_0Name
    N.C1_1Name
    N.S1_0_0Name
    N.S1_1_0Name

Now the problem is, if I have something like this:

module N where

import GHC.Generics
import qualified M as Blah

data Name = Name

deriving instance Generic Blah.Name
deriving instance Generic Name

---

module M where
data Name = Name

It generates same data types and instances(including head parts, because generated data types are same so instance heads have to refer to same names) for both Names. This leads to duplicate data type and instance declarations.

What I did for D1081 so far was to add module names as prefix to generated data types. It worked fine(currently validates), but if we use package imports it should break. So we thought maybe we should use qualified names of modules as a prefix. In our case, that would mean generating Blah_ prefixed types for Name in module M, and non-prefixed types for Name in current module. With package imports the user need to give modules different names so this should work.

But it turns out to be hard to implement, because at the point we're generating instance code, we don't have any knowledge about qualified imports. RdrNames are eliminated during renaming. With some experiments I realized Outputable.PrintUnqualified doesn't give this info etc.

That's where I got stuck. We thought of some solutions:

  • Add RdrName as a field to Name. Name is a pretty central data type and we may not want to change it. Also, this probably means changing a lot of other code.
  • Pass RdrNames through type checker. No changes in any data types, but we still need to change a lot of other code, functions etc. just to pass this argument through.
  • (We had the idea of using Outputable.PrintUnqualified data but that won't work)

I must mention, I don't have an example with package imports. Maybe GHC is already giving modules different names when package imports is used? That would solve everything. I'll try to build a broken(with my patch) example today.

comment:12 Changed 3 years ago by simonpj

Are the newly declared data types supposed to be user-visible, with predictable names (they don't look very user-friendly!) or is it just an internal matter.

If it's just a question of generating fresh names that are distinct, you could use the same approach as for dictionary-functions. Look at TcEnv.newDFunName.

This reminds me (Andres) that there is an outstanding task to clean up this generics stuff using DataKinds.

comment:13 Changed 3 years ago by osa1

I don't think they're user visible, because for example if I load the file in GHCi I can't see those types there(although I must say I don't know how this is possible to implement, I'd expect everything in that module to be visible).

I'll have a look at TcEnv.newDFunName, it sounds like a great idea.

Is there a ticket for the clean up task?

comment:14 in reply to:  13 Changed 3 years ago by simonpj

Is there a ticket for the clean up task?

I don't think so -- there should be

comment:15 Changed 3 years ago by dreixel

The generated datatypes do not have to be user-visible.

As for using DataKinds in generics, I wouldn't really call it a clean up task, as I think there's some bit of design left to do. Anyway, it's mostly described in here: https://ghc.haskell.org/trac/ghc/wiki/Commentary/Compiler/GenericDeriving#Kindpolymorphicoverhaul

comment:16 Changed 3 years ago by osa1

I fixed name generation and the patch is ready for reviews.

Only missing thing is a test for package imports. Does anyone know if it's possible to add a test with packages without modifying test driver? (driver/runtests.py)

comment:17 Changed 3 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:18 Changed 3 years ago by Edward Z. Yang <ezyang@…>

In b08a533/ghc:

Fix DeriveGeneric for types with same OccName (#10487)

Summary:
DeriveGeneric generates some data types (for data type constructors and for
selectors of those constructors) and instances for those types. This patch
changes name generation for these new types to make it working with data types
with same names imported from different modules and with data types with same
names imported from same modules(using module imports).

Bonus content:

- Some refactoring in `TcGenGenerics.metaTyConsToDerivStuff` to remove some
  redundant partial function applications and to remove a duplicated function.
- Remove some unused names from `OccName`. (those were used for an old
  implementation of `DeriveGeneric`)

Reviewers: kosmikus, simonpj, dreixel, ezyang, bgamari, austin

Reviewed By: bgamari, austin

Subscribers: ezyang, thomie

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

GHC Trac Issues: #10487

comment:19 Changed 3 years ago by ezyang

Resolution: fixed
Status: newclosed

Pushed. I assume we aren't backporting to 7.10?

comment:20 Changed 3 years ago by osa1

Test Case: T10487

comment:21 Changed 3 years ago by simonpj

Keywords: Generics added
Note: See TracTickets for help on using tickets.