Opened 3 years ago

Closed 3 years ago

#5275 closed bug (fixed)

Data.Typeable not backwards compatible

Reported by: augustss Owned by: simonmar
Priority: high Milestone: 7.2.1
Component: libraries/base Version: 7.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty:
Test Case: Blocked By:
Blocking: Related Tickets:

Description

In ghc 7.1 the type name returned for prelude types has changed. It used to be Int, Integer, etc, but in ghc 7.1 it's GHC.Types.Int, GHC.Integer.Type.Integer, etc.

This has many drawbacks:

  • The names are impossible to guess
  • The names are not backwards compatible
  • The names are not portable to other compilers
  • The names reveal implementation details that should be kept hidden

Change History (9)

comment:1 Changed 3 years ago by simonmar

I agree. Is there some reason we use the qualified original name here? It's not even necessarily unique, because we don't include the package name.

comment:2 follow-up: Changed 3 years ago by simonpj

Simon and I talked about this. We propose to change Typeable.TyCon from this:

data TyCon = TyCon !Key String

to this:

data TyCon = TyCon !Key 
                   String    -- Package name
                   String    -- Module name
                   String    -- TyCon name

and add accessors for these fields. Then you can get the bits you want.

It's not enough to keep just the loacl name (like Integer) because there might be many types with the same name.

A the same time we propose to get rid of the side-effected cache and use MD5 checksums instead.

comment:3 Changed 3 years ago by simonpj

  • Milestone set to 7.2.1
  • Owner set to simonmar
  • Priority changed from normal to high

comment:4 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by batterseapower

Replying to simonpj:

Simon and I talked about this. We propose to change Typeable.TyCon from this:

data TyCon = TyCon !Key String

to this:

data TyCon = TyCon !Key 
                   String    -- Package name
                   String    -- Module name
                   String    -- TyCon name

I don't think Lennart wanted a way to deconstruct a TyCon?. He just wanted equality on TyCon? strings to be a) backward compatible and b) not reveal implementation details about where "built-in" types like Int were actually declared. Do you plan to do something separate to address that point?

comment:5 in reply to: ↑ 4 Changed 3 years ago by simonmar

Replying to batterseapower:

I don't think Lennart wanted a way to deconstruct a TyCon?. He just wanted equality on TyCon? strings to be a) backward compatible and b) not reveal implementation details about where "built-in" types like Int were actually declared. Do you plan to do something separate to address that point?

The specific problem is that tyConString returns a string that is not portable and reveals implementation details. Making tyConString return just the name of the TyCon and not its implementation module would fix that. We also need a way to uniquely identify TyCons, and the hash provides that - the hash would be the Md5 of "package-1.0:Module.TyCon", but computed at compile-time because it's easier. The other fields are just for information (we'll probably provide GHC-specific ways to get at them).

comment:6 Changed 3 years ago by igloo

So if I understand correctly, the value of tyConString for derived Typeable instances will change between 7.0 and 7.2?

comment:7 Changed 3 years ago by malcolm.wallace@…

As I understand it, the value of tyConString has already changed, between the released 7.0 and the development head 7.1. The request is to change it back, before the release of 7.2, to the same behaviour as in 7.0 and before.

comment:8 Changed 3 years ago by simonmar

strictly speaking, in 7.1 tyConString changed only for the built-in types for which instances were declared in Data.Typeable, because we switched from using CPP macros to declare the instances to using standalone deriving. In 7.2 we'll be changing the tyConString for derived instances to match the behaviour of tyConString in the CPP macro instances in 7.0.

So for the built-in types, we'll be reverting to the 7.0 behaviour, and for all other types the tyConStings will change.

comment:9 Changed 3 years ago by simonmar

  • Resolution set to fixed
  • Status changed from new to closed

Now fixed; see 52ddeaaedfed3135a739585b41fdd2ad4f1f8e81 and following commits, and 903c1afa3bdd205dc5473c0fca1065d598145311 in packages/base.

Note: See TracTickets for help on using tickets.