Stable name type role
ClosedPublic

Authored by dfeuer on Aug 30 2018, 2:18 PM.

Details

Summary

Make the StableName# parameter phantom:

There is actually never any reason to care about the type of
the underlying object of a StableName#. The underlying object
type shouldn't really even *be* a parameter. But at least we
can mark it as phantom.

dfeuer created this revision.Aug 30 2018, 2:18 PM
simonmar accepted this revision.Sep 6 2018, 9:52 AM

It would be nicer to put unrelated changes in separate commits, but OK.

This revision is now accepted and ready to land.Sep 6 2018, 9:52 AM
dfeuer planned changes to this revision.Sep 9 2018, 2:57 PM

The StableName type in base needs to go back to having a representational parameter. The stable-maps package relies on that for safety.

dfeuer updated this revision to Diff 17952.Sep 9 2018, 4:04 PM
  • Don't go quite so far
This revision is now accepted and ready to land.Sep 9 2018, 4:04 PM
dfeuer updated this revision to Diff 17953.Sep 9 2018, 5:26 PM
  • Don't go quite so far
dfeuer updated this revision to Diff 17955.Sep 9 2018, 7:02 PM
  • Silly me
dfeuer updated this revision to Diff 17956.Sep 9 2018, 11:53 PM
  • Hopefully this is the last
dfeuer requested review of this revision.Sep 10 2018, 4:58 AM

I had to make some changes. Could someone please have another look?

simonmar added inline comments.Sep 14 2018, 3:31 AM
libraries/base/GHC/StableName.hs
102–104

It wasn't clear to me where you'd got this from, so I went to look at stable-maps. At least as I understand it, if we were to give StableName a phantom role, that would break System.Mem.Stable.Map because you could coerce a StableName a into a StableName b, look it up in the map and thereby coerce an arbitrary a into b.

I think the comment above could be reworded to be more clear. It's not obvious to me whether you're saying the property holds, or should hold, or should not hold, or what "pulling tricks" means. Could you say precisely what will go wrong if we use a phantom role?

I'd actually be tempted to consider using a phantom role and forcing stable-maps to wrap the StableName type.

118

So if we have this, can't you use it to break stable-maps?

dfeuer added inline comments.Sep 14 2018, 7:37 AM
libraries/base/GHC/StableName.hs
102–104

you could coerce a StableName a into a StableName b, look it up in the map and thereby coerce an arbitrary a into b.

That is precisely the potential problem. I believe the property currently holds. "Pulling tricks" means using something like unsafeCoerce.

I'd actually be tempted to consider using a phantom role and forcing stable-maps to wrap the StableName type.

That sounds like it would require input from the community. If we're going to think about breaking things, I'd much rather break things better:

  1. Change the kind of StableName# so it doesn't have a parameter at all.
  2. Add a StableName_ (or whatever) type as a thin wrapper: data StableName_ = StableName_ StableName#.
  3. Make StableName itself a compatibility wrapper around StableName_ and expose its constructor from an internal module.
118

Definitely. But you have to import an unsafe module and do it on purpose.

dfeuer added inline comments.Sep 14 2018, 11:43 AM
libraries/base/GHC/StableName.hs
102–104

I actually just realized that what stable-maps is doing is pretty badly wrong anyway. The invariant I thought we had is not really there. See the ticket. Perhaps a phantom role for StableName isn't so bad after all, if we can't safely use a representational one.

dfeuer updated this revision to Diff 18005.Sep 14 2018, 5:20 PM
  • Split differential; rebase; revert silliness
dfeuer retitled this revision from Stable name comments and type role to Stable name type role.Sep 14 2018, 5:20 PM
dfeuer edited the summary of this revision. (Show Details)

@simonmar, I reverted all that fancy machinery that turned out to be rather silly. I also split this into two commits: one for the type role change and one for the comment wibbles. Could I get another accept mark?

simonmar accepted this revision.Sep 15 2018, 7:12 AM
simonmar added a subscriber: ekmett.

Ok cool. We better tell @ekmett that stable-maps is not type-safe any more :)

This revision is now accepted and ready to land.Sep 15 2018, 7:12 AM
This revision was automatically updated to reflect the committed changes.