Encode strictness in GHC generics metadata
ClosedPublic

Authored by RyanGlScott on Dec 17 2015, 12:53 AM.

Details

Summary

This augments MetaSel with a Bang field, which gives generic
programmers access to the following information about each field
selector:

  • SourceUnpackedness: whether a field was marked {-# NOUNPACK #-}, {-# UNPACK #-}, or not
  • SourceStrictness: whether a field was given a strictness (!) or laziness (~) annotation
  • DecidedStrictness: what strictness GHC infers for a field during compilation, which may be influenced by optimization levels, -XStrictData, -funbox-strict-fields, etc.

Unlike in Phab:D1603, generics does not grant a programmer the ability
to "splice" in metadata, so there is no issue including
DecidedStrictness with Bang (whereas in Template Haskell, it had to
be split off).

One consequence of this is that MetaNoSel had to be removed, since it
became redundant. The NoSelector empty data type was also removed for
similar reasons.

Fixes Trac #10716.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
arcpatch-D1646
Lint
Lint WarningsExcuse: (╯°□°)╯︵ ┻━┻
SeverityLocationCodeMessage
Warningcompiler/prelude/PrelNames.hs:924TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:925TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:926TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:927TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:928TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:929TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:930TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:931TXT3Line Too Long
Warningcompiler/prelude/PrelNames.hs:932TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 7528
Build 9188: GHC Patch Validation (amd64/Linux)
Build 9187: arc lint + arc unit
RyanGlScott retitled this revision from to Encode strictness in GHC generics metadata.
RyanGlScott updated this object.
RyanGlScott edited the test plan for this revision. (Show Details)
RyanGlScott updated the Trac tickets for this revision.
RyanGlScott edited edge metadata.

Rebase

kosmikus edited edge metadata.Dec 17 2015, 1:18 AM

Looks good to me in general ... but a bit of bikeshedding, sorry:

  1. I don't really like the name Bang as a constructor. It refers to a syntactic peculiarity for a part of what Bang actually covers. Would Representation perhaps be better?
  2. I would prefer if the three pieces of knowledge supplied by Bang would be offered via separate class methods in Selector, not just selBang, but rather selSourceUnpackedness, selSourceStrictness, and selDecidedStrictness. Then the Bang type which is otherwise a strange triple without any accessor functions is mostly internal, so it's name matters somewhat less. The Bang datatype also currently has no documentation, only the datatypes inside do. So that's another hint that maybe it should not be actively exposed.
  3. If you actually go for something like Representation as the name, I'd prefer DecidedStrictness to be called DecidedRepresentation, because then it's clearer that it includes info about both the "unpackedness" and the "strictness".

A more vague remark is that I feel uneasy about duplicating so much of the singletons machinery in the GHC.Generics module. Would it perhaps make sense to move this to a separate GHC module? I've also not completely kept track of the patches Richard has merged recently. How much of this could possibly already be simplified using the new types/kinds layer we now have? (Not saying we necessarily should, even if we could.)

  1. I don't really like the name Bang as a constructor. It refers to a syntactic peculiarity for a part of what Bang actually covers. Would Representation perhaps be better?

I'm not particularly attached to the name Bang. I only chose it originally because that's what GHC uses internally for the same concepts (HsSrcBang and HsImplBang). But the name Representation is a bit vague for my tastes, especially since it's in a module full of things designated as "representation" types.

  1. I would prefer if the three pieces of knowledge supplied by Bang would be offered via separate class methods in Selector, not just selBang, but rather selSourceUnpackedness, selSourceStrictness, and selDecidedStrictness. Then the Bang type which is otherwise a strange triple without any accessor functions is mostly internal, so it's name matters somewhat less. The Bang datatype also currently has no documentation, only the datatypes inside do. So that's another hint that maybe it should not be actively exposed.

I would be agreeable to that. While we're at it, how about we get rid of Bang entirely and just make MetaSel :: Maybe Symbol -> SourceUnpackedness -> SourceStrictness -> DecidedStrictness -> Meta? That would make Selector and MetaSel align more closely, and we can avoid painting the Bang bikeshed entirely.

I would be agreeable to that. While we're at it, how about we get rid of Bang entirely and just make MetaSel :: Maybe Symbol -> SourceUnpackedness -> SourceStrictness -> DecidedStrictness -> Meta? That would make Selector and MetaSel align more closely, and we can avoid painting the Bang bikeshed entirely.

Sounds good to me.

A more vague remark is that I feel uneasy about duplicating so much of the singletons machinery in the GHC.Generics module. Would it perhaps make sense to move this to a separate GHC module?

Perhaps. If @goldfire feels strongly about it, we could perhaps make a GHC.Singletons module for this stuff. Currently, we're using pretty stripped-down singletons typeclasses (e.g., SingKind doesn't have a toSing function), so I don't know if that would be generally useful for singletons programmers.

I've also not completely kept track of the patches Richard has merged recently. How much of this could possibly already be simplified using the new types/kinds layer we now have? (Not saying we necessarily should, even if we could.)

Richard, please chime in if I'm wrong, but as I understand, kind equalities simply make it possible to promote any data type to the type/kind-level. Our encoding of generic metadata is pretty basic, though, so I don't know if we'd need to leverage new features.

RyanGlScott updated this revision to Diff 5792.Dec 17 2015, 3:21 AM
RyanGlScott edited edge metadata.
  • Remove Bang, refactor Selector
RyanGlScott edited edge metadata.
  • Rebase 2
RyanGlScott updated this revision to Diff 5818.Dec 17 2015, 6:28 PM
RyanGlScott edited edge metadata.

Rebase 3

bgamari edited edge metadata.Dec 19 2015, 5:34 PM

@RyanGlScott, do you consider this ready to be merged?

Looks good to me.

libraries/base/GHC/Generics.hs
881

These comments look like Haddock comments but lack the |. Is this intentional?

RyanGlScott edited edge metadata.
  • Haddockify comments
RyanGlScott marked an inline comment as done.Dec 19 2015, 10:52 PM
RyanGlScott added inline comments.
libraries/base/GHC/Generics.hs
881

Oops, those definitely should be Haddock comments.

Now it's ready to be merged. :)

bgamari accepted this revision.Dec 20 2015, 4:33 AM
bgamari edited edge metadata.

Great! Thanks @RyanGlScott. One comment inline.

libraries/base/GHC/Generics.hs
666

@RyanGlScott, I'll merge this as-is but do you suppose you could either clean up this, or, even better, add some content to this section so it can be uncommented?

This revision is now accepted and ready to land.Dec 20 2015, 4:33 AM
RyanGlScott marked an inline comment as done.Dec 20 2015, 9:41 AM
RyanGlScott added inline comments.
libraries/base/GHC/Generics.hs
666

To be honest, I'm not sure why @dreixel added this CPP'ed-out section in the first place. Perhaps he or @kosmikus can explain to me sometime. If I do figure it out, I'll tidy it up in a Diff in the future!

This revision was automatically updated to reflect the committed changes.
kosmikus added inline comments.Dec 23 2015, 12:52 AM
libraries/base/GHC/Generics.hs
666

I think the history is that I wrote the documentation elsewhere, but it was unfinished in places. Then @dreixel moved it over to this place and commented out some of the most broken bits. As far as I'm concerned, this can be removed.