Implementation of StrictData language extension
ClosedPublic

Authored by adamse on Jul 3 2015, 6:16 AM.

Details

Summary

This implements the StrictData language extension, which lets the programmer default to strict data fields in datatype declarations on a per-module basis.

Specification and motivation can be found at https://ghc.haskell.org/trac/ghc/wiki/StrictPragma

This includes a tricky parser change due to conflicts regarding ~ in the type level syntax: all ~'s are parsed as strictness annotations (see strict_mark in Parser.y) and then turned into equality constraints at the appropriate places using RdrHsSyn.splitTilde.

Test Plan

Validate through Harbormaster.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
simonpj added inline comments.Jul 6 2015, 4:43 PM
compiler/basicTypes/DataCon.hs
478

better: add NoSrcStrictness, and remove the Maybe

483

Better: add NoSrcUnpack and remove the Maybe.

630

I assume that these are the clauses that are cyptically referred to later. They do indeed look bogus. We should not compare the source pragmas with GHC's decisions. (Indeed the fact that HsBang contains both in a sum type is a bad thing, but that's a bigger thing to fix.)

So I think you must be right that the wrong things are being compared somewhere.

compiler/parser/Parser.y
1568–1569

This all seems a bit laborious. Isn't it just an optional UNPACK/NOUNPACK prag followed by an optional ! or ~? Why write out 9 cases when one production, mentioning two non-terminals, each with three productions, would do?

adamse marked 21 inline comments as done.Jul 7 2015, 10:17 AM
adamse added inline comments.
compiler/parser/Parser.y
1568–1569

Something like this

strict_mark :: { ... }
        : unpackedness strictness { .... }
unpackedness :: { ... }
        : {- empty -} { NoSrcUnpack }
        | '{- UNPACK' '-}' { ...
....

creates an explosion of shift/reduce conflicts in the parser (>3000).

adamse updated this revision to Diff 3456.Jul 7 2015, 10:42 AM
  • StrictData: Remove Maybe's from HsSrcBang
  • StrictData: Fix use of eqHsBang in TcRnDriver.checkBootTyCon
  • StrictData: doc fixes
  • StrictData: clean up parser terminals
simonpj accepted this revision.Jul 7 2015, 11:27 AM

Looks good

compiler/parser/Parser.y
1568–1583

OK, well it's certainly better than it was

compiler/typecheck/TcRnDriver.hs
905

Why passing (unused) DynFlags? Ditto checkBootDecl

tibbe added a comment.Jul 7 2015, 11:59 AM

P.S. Please also check the linter errors.

compiler/basicTypes/MkId.hs
537

Unintentional change?

bgamari requested changes to this revision.Jul 7 2015, 12:16 PM

Almost done! Looking great!

compiler/basicTypes/DataCon.hs
478

Fair point.

620

Perhaps let's just kill this instance and the one below and let GHC derive it.

compiler/basicTypes/MkId.hs
537

Fix alignment.

607

I'm not sure I understand the question mark here.

This revision now requires changes to proceed.Jul 7 2015, 12:16 PM
adamse marked an inline comment as done.Jul 7 2015, 12:35 PM

Lint error fix is coming.

I've tried for a split of HsBang, will post this as a separate diff I think.

compiler/basicTypes/DataCon.hs
620

Sounds good.

compiler/basicTypes/MkId.hs
537

Changed as per Simons suggestion: "in wrapper_reqd in MkId, use wrap_bangs rather than orig_bangs"

607

Seems that I did not understand the Bang! Will Fix

adamse updated this revision to Diff 3464.EditedJul 7 2015, 1:11 PM
  • Fix lint warnings

and other things.

adamse added inline comments.Jul 7 2015, 1:15 PM
.gitmodules
103 ↗(On Diff #3464)

Posted a PR to the haddock repo: https://github.com/haskell/haddock/pull/411

adamse updated this revision to Diff 3475.Jul 8 2015, 1:48 AM

Point haddock to correct commit

thomie added a comment.Jul 8 2015, 5:00 AM

Nice work.

  • The change of isBanged now checking dataConImplBangs instead of dataConSrcBangs. Is that a bug fix (i.e. can we write a test for it?). If so, maybe put it in a separate Diff. Or just ignore this comment if it's just a minor issue.
  • Next time: could you please separate whitespace and layout changes from functional changes. It makes reading this Diff harder to read than it needs to be.

Some minor comments below.

compiler/basicTypes/DataCon.hs
459–462

Comment out-of-date: they are not Maybes.

463

Second mention of "What the user wrote in the source code".

522–525

Comment out of date.

539

HsUnpack takes an argument.

compiler/typecheck/TcSplice.hs
1495

Should Opt_StrictData be checked here as well?

adamse updated this revision to Diff 3477.Jul 8 2015, 5:52 AM
  • StrictData: Update comments
adamse added a comment.Jul 8 2015, 6:01 AM

@thomie: I have another patch that will split HsBang into HsSrcBang and HsImplBang which will make this using isBanged and eqHsBang on the wrong Bangs a type error.

compiler/typecheck/TcSplice.hs
1495

I believe it should not, as Simon points out in this ghc-devs thread: https://mail.haskell.org/pipermail/ghc-devs/2015-January/007973.html

adamse updated this object.Jul 8 2015, 6:01 AM
thomie updated the Trac tickets for this revision.Jul 8 2015, 9:19 AM
thomie added a comment.Jul 8 2015, 9:21 AM

Thanks. I have one more design question in ticket Trac #8347.

Note: this patch doesn't validate at the moment. Maybe you need a rebase.

compiler/typecheck/TcSplice.hs
1495

Ah, at least I wasn't the only one with that question.

adamse updated this revision to Diff 3480.Jul 8 2015, 4:22 PM
  • StrictData: update haddock submodule
  • StrictData: fix haddock syntax
adamse updated this revision to Diff 3484.Jul 8 2015, 5:12 PM

Rebase onto master

  • StrictData: remove unused argument
  • StrictData: Add users guide section
  • StrictData: Update haddock to support new strictness annotations
  • StrictData: Update user manual
  • StrictData: update some comments
  • StrictData: Add StrictData to GHC-only extensions
  • StrictData: Appease testsuite
  • StrictData: Bools to more descriptive types and documentation fixes
  • Fix syntax error in users guide
  • StrictData: Remove Maybe's from HsSrcBang
  • StrictData: Fix use of eqHsBang in TcRnDriver.checkBootTyCon
  • StrictData: doc fixes
  • StrictData: clean up parser terminals
  • Fix lint warnings
  • StrictData: remove unused DynFlags arguments
  • StrictData: Derive some instances, fix some whitespace
  • StrictData: Update comments
  • StrictData: update haddock submodule
  • StrictData: fix haddock syntax
adamse added a comment.Jul 9 2015, 8:57 AM

The patch validates locally, there are some additional commits needed for the haddock submodule.

lelf added a subscriber: lelf.Jul 10 2015, 7:28 AM
adamse updated this object.Jul 14 2015, 10:03 AM
adamse updated this revision to Diff 3614.Jul 21 2015, 4:24 AM

Temporarily change Haddock url

  • StrictData: remove unused argument
  • StrictData: Add users guide section
  • StrictData: Update haddock to support new strictness annotations
  • StrictData: Update user manual
  • StrictData: update some comments
  • StrictData: Add StrictData to GHC-only extensions
  • StrictData: Appease testsuite
  • StrictData: Bools to more descriptive types and documentation fixes
  • Fix syntax error in users guide
  • StrictData: Remove Maybe's from HsSrcBang
  • StrictData: Fix use of eqHsBang in TcRnDriver.checkBootTyCon
  • StrictData: doc fixes
  • StrictData: clean up parser terminals
  • Fix lint warnings
  • StrictData: remove unused DynFlags arguments
  • StrictData: Derive some instances, fix some whitespace
  • StrictData: Update comments
  • StrictData: update haddock submodule
  • StrictData: fix haddock syntax
  • change submodule url
adamse updated this revision to Diff 3615.Jul 21 2015, 4:30 AM

rebaste onto master

tibbe accepted this revision.Jul 21 2015, 4:31 AM

Can we get this submitted as is? I'm worried that we (in general) are relying too much on huge blobs of code being reviewed at once. That has several downsides:

  • Scope creep - one review tends to end up growing larger and larger as we add features, refactoring, etc.
  • Hard to review - one can really only read so much code before your eyes glaze over.
  • Merge headaches - the committer will have to keep their code rebased on top of master for longer, adding work.

I realize that small frequent commits aren't always possible, but still we should strive to do it as much as possible. We can always submit more pull requests to improve things over time.

simonpj accepted this revision.Jul 21 2015, 4:52 AM

I'm good with committing this. See my notes above about documenting splitTilde

compiler/parser/RdrHsSyn.hs
1063

As you remark, "This includes a tricky parser change due to conflicts regarding ~ in the type level syntax: all ~'s are parsed as strictness annotations (see strict_mark in Parser.y) and then turned into equality constraints at the appropriate places using RdrHsSyn.splitTilde"

But the code gives no clue about the trickiness. Could you add a Note to explain, with example(s) of what is tricky, and an explanation of how it is resolved. And then refer to the Note appropriately.

bgamari requested changes to this revision.Jul 21 2015, 7:44 AM

As @simonpj mentioned, some more description of the parsing trickiness would be nice.

Also, what is the status of the haddock changes? I believe Harbormaster is now handling submodules correctly so I'm a bit confused as to why the validation is still failing.

This revision now requires changes to proceed.Jul 21 2015, 7:44 AM

@bgamari: Harbormaster still fails with exactly the same error, and my haddock fork still contains the right changes. I wonder if the Harbormaster build script update has gone live?

@adamse, it seems you were right and I believe this should now be fixed. Let's try this again.

adamse updated this revision to Diff 3625.Jul 21 2015, 3:05 PM
  • StrictData: disallow strictness annotation on newtypes
  • StrictData: add note on parsing ~
bgamari resigned from this revision.Jul 21 2015, 3:53 PM
bgamari removed a reviewer: bgamari.

Interesting, looks like T4945 is now failing.

This revision is now accepted and ready to land.Jul 21 2015, 3:53 PM
bgamari requested changes to this revision.Jul 21 2015, 4:30 PM
bgamari added a reviewer: bgamari.

Oops, wrong action.

This revision now requires changes to proceed.Jul 21 2015, 4:30 PM
adamse added a comment.EditedJul 22 2015, 12:55 AM

@bgamari: T4945 is marked expect broken, this does not make sense at all.

Edit: Or rather in the base commit of this diff it is marked broken, but in master it is not.

Edit: I have no idea what could be causing this error. Simon mentions in all.T that this testcase is wobbly.

In D1033#29747, @adamse wrote:

@bgamari: T4945 is marked expect broken, this does not make sense at all.

Ignore that error. You can see here that it's following on master as well: https://phabricator.haskell.org/diffusion/GHC/history/

Note to self: we REALLY need to do something about this. Someone working on a patch should not be bothered by test failures in HEAD.

bgamari accepted this revision.Jul 27 2015, 5:14 AM

Looks good to me. Great work!

This revision is now accepted and ready to land.Jul 27 2015, 5:14 AM
This revision was automatically updated to reflect the committed changes.