Lower precedence for {-# UNPACK #-}
ClosedPublic

Authored by int-index on Oct 11 2018, 5:20 AM.

Details

Summary

Lower the precedence of the {-# UNPACK #-} pragma according to an accepted GHC proposal: https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0037-unpack-pragma-precedence.rst

Test Plan

Validate

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.
int-index created this revision.Oct 11 2018, 5:20 AM

This change is not yet accepted by the GHC Steering Committee, but I prepared a patch for the case it gets accepted.

Here's the proposal: https://github.com/ghc-proposals/ghc-proposals/pull/174

int-index updated this revision to Diff 18290.Oct 11 2018, 8:29 AM

Testsuite correction

bgamari requested changes to this revision.Oct 15 2018, 11:21 AM

Bumping out of the review queue until acceptance.

This revision now requires changes to proceed.Oct 15 2018, 11:21 AM
int-index added a comment.EditedOct 20 2018, 12:29 PM

@bgamari The proposal was accepted, would you mind taking another look?

int-index requested review of this revision.Oct 20 2018, 12:31 PM
mpickering added inline comments.
testsuite/tests/parser/should_fail/strictnessDataCon_A.hs
1

Why did this change?

int-index added inline comments.Oct 24 2018, 3:44 PM
testsuite/tests/parser/should_fail/strictnessDataCon_A.hs
1

! + Int now triggers Operator applied to too few arguments: + – this is due to an improvement in clause [opr]:

      if null acc || null (filter isTyElOpd xs)
	        then failOpFewArgs (L l op)

Note the added filter. This codepath is already tested by other tests (e.g. unpack_before_opr), so I decided to modify strictnessDataCon_A in a way that it still triggers the "Strictness annotation cannot appear in this position" error.

int-index updated this revision to Diff 18437.Oct 24 2018, 4:20 PM

Update comments, rebase

osa1 added a subscriber: osa1.Oct 25 2018, 2:29 AM

I realized something that we should've perhaps discussed in the proposal (btw, could you link to the proposal in the description? The comments will get lost but the description will be the commit message).

Namely, we want to allow this:

data T = MkT1 { a :: {-# UNPACK #-} Either Int Bool }

but actually this generates a warning because we're missing a strictness annotation. The problem is strictness annotations still have high precedence so we need parens as well:

data T = MkT1 { a :: {-# UNPACK #-} !(Either Int Bool) }

So it seems to me that a lot of times this change will have no effect, which is a bit unfortunate.

As some of the tests demonstrate this works when you have StrictData or Strict enabled, but I think if you don't you'll always need parens.

Still an improvement though.

docs/users_guide/8.8.1-notes.rst
56

Why not use something simpler, like Either Int Bool? && requires TypeOperators, and it's not a built-in type operator so I think this looks somewhat confusing.

(If this was a comment in the compiler rather than user manual then maybe you would expect the reader to already know this stuff, but in the user manual simpler is better)

int-index updated this revision to Diff 18445.Oct 25 2018, 8:37 AM

Rebase, add proposal link to the commit message

int-index edited the summary of this revision. (Show Details)Oct 25 2018, 8:41 AM
int-index added a comment.EditedOct 25 2018, 9:08 AM

could you link to the proposal in the description?

Good idea, done.

I realized something that we should've perhaps discussed in the proposal <...> strictness annotations still have high precedence so we need parens

The proposal explicitly mentions this fact, and I think it's better to keep it this way, as !Maybe Int looks like ! is applied only to Maybe, and ! Maybe Int doesn't look like a strictness annotation due to additional whitespace. But I agree that it'd be better to have this discussion in the proposal, not in its implementation. Fortunately, we're not doing anything incompatible here, so changing the precedence of ! could be its own proposal.

So it seems to me that a lot of times this change will have no effect, which is a bit unfortunate. <...> As some of the tests demonstrate this works when you have StrictData or Strict enabled

IME most of the times it's a good idea to enable StrictData – if the code relies on laziness of some field in a data structure, it's better to have it marked with ~ explicitly. As people use StrictData more over time (at least I hope this happens), the benefit of lower precedence for {-# UNPACK #-} will increase.

docs/users_guide/8.8.1-notes.rst
56

I wanted to show that {-# UNPACK #-} has lower precedence than type operators, that's why I included them in the example.

If we go with something simpler (e.g. Either Int Bool), the user might think that {-# UNPACK #-} Either Int Bool && String still parses as {-# UNPACK #-} (Either Int Bool) && String, so I'm inclined to keep the example as is – it conveys more information.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 1 2018, 7:34 PM
This revision was automatically updated to reflect the committed changes.