Implement the Strict language extension
ClosedPublic

Authored by adamse on Aug 10 2015, 1:48 PM.

Details

Summary

Add a new language extension -XStrict which turns all bindings strict as if the programmer had written a ! before it. This also upgrades ordinary Haskell to allow recursive and polymorphic strict bindings.

See the wiki[1] and the Note [Desugar Strict binds] in DsBinds for specification and implementation details.

[1] https://ghc.haskell.org/trac/ghc/wiki/StrictPragma

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

Broadly ok.

Please add a substantial Note, probably in DsBinds, to explain the desugaring. Refer to the wiki page, yes, but give a compact summary here. The list of "force_vars" is the key bit that needs explaining. EXAMPLES.

There is a real bug in the mkSelectorBinds thing when there are no binders.

I have outlined one way to deal with the tricky polymorphic case, in Implementation Notes, on the wiki:StrictPragma page.

I'd like to see more regression tests that test the corner cases, such as let False = e in b, and recursive bindings with Strict and so on. The wiki page gives some key examples.

Good work. Nearly there.

Simon

compiler/deSugar/DsBinds.hs
86–91

What are these [Id]s? Give an example.

It's the list of Ids that should be forced in the body of the binding group. Need to give a Note describing the desugaring, and referring to the StrictPragma wiki page.

93–94

Why is one a [Id] and one an OrdList?

137
if xopt Opt_Strict dflags && matchGroupArity == 0

No point in returning a variable to force if it's going to be a lambda! (Put a comment though.)

203

See "implementation notes" on wiki:StrictPragma.

284

I'd pass DynFlags.. then the function becomes more self-describing than given a Bool

compiler/deSugar/DsUtils.hs
614–615

Always return exactly one Var, not a list.

614–615

This is wrong. Consider

False = e

under Strict. This is a pattern binding that binds zero variables (hence null binders) but you must evaluate it under the Strict semantics.

This revision now requires changes to proceed.Aug 26 2015, 11:27 AM
tibbe added a comment.Sep 21 2015, 9:18 AM

Adam, do you think you'll still have time to look into this now when your studies have started?

In D1142#35454, @tibbe wrote:

Adam, do you think you'll still have time to look into this now when your studies have started?

Yes, I have made some time this week to continue the project and I expect that I will be able to make the 8.0 November deadline if I don't hit any major problems along the way.

adamse updated this object.Sep 22 2015, 2:16 AM
adamse updated this revision to Diff 4257.Sep 22 2015, 2:48 AM
  • Working desugaring?
  • Rebase
adamse updated this revision to Diff 4258.Sep 22 2015, 3:50 AM
  • rebase
adamse updated this revision to Diff 4261.Sep 22 2015, 7:46 AM
  • Add Strict to expected GHC language extensions test
  • add Strict to users guide
  • add literal tags to docs
  • strict toEnum error currently
  • Strict: Desugar strict bindings working
  • Strict: fix T9140
  • add back opt_strict
adamse updated this object.Sep 22 2015, 7:58 AM
adamse updated this revision to Diff 4263.Sep 22 2015, 8:40 AM
  • Strict: Accept new error output T6078
adamse updated this revision to Diff 4337.Sep 27 2015, 4:00 PM
  • Strict: OrdList -> []
  • Strict: add regression test for let-like binders
bgamari requested changes to this revision.Oct 6 2015, 9:33 AM

@adamse, how is progress going? It appears this needs to be rebased on the ReST user-guide change.

This revision now requires changes to proceed.Oct 6 2015, 9:33 AM

@adamse, how is progress going? It appears this needs to be rebased on the ReST user-guide change.

Yes, I just hit the users guide rebase and decided to do something else for a while... Is there a simple way to convert my docbook to rst?

I hope to be in a review ready state by the weekend.

@adamse, see https://gist.githubusercontent.com/bgamari/8bc90d2f159fadf7fe0a/raw/7e03d43573d04a960b05968662af87274ad196b5/Strict.rst. That is just a cleaned up version of the output of pasting your XML fragment from the diff into pandoc --from docbook --to rst.

docs/users_guide/glasgow_exts.xml
11395 ↗(On Diff #4337)

You'll need to resolve this one manually.

adamse updated this revision to Diff 4432.Oct 6 2015, 12:11 PM
  • Update users guide entry to rst
  • Rebase
adamse updated this revision to Diff 4433.Oct 6 2015, 1:03 PM
  • Strict: fix T4437
adamse updated this revision to Diff 4512.Oct 16 2015, 5:50 AM
adamse marked 18 inline comments as done.
  • Strict: update comments here and there
  • Rebase

Mark a bunch of comments as done.

adamse retitled this revision from WIP: Implement Strict language extension to Implement Strict language extension.Oct 16 2015, 6:00 AM
adamse updated this object.

I'm feeling pretty happy with the patch, can I get a round of reviews @bgamari, @tibbe, @simonpj?

simonpj requested changes to this revision.Oct 16 2015, 11:24 AM

Some things still look fishy.

compiler/deSugar/DsBinds.hs
171

What are these "don't handle strict binds" tests doing? If not here, then where? Perhaps you mean to defer to the general case which follows? If so say so.

And if so, what's all this matchup_export stuff doing?

I suspect you have two plans mixed up together here.

234–236

This is weird. Why both exported_force_vars and extra_exports? They are more or less the same aren't they?

You desperately need an example to illustrate this case. It is utterly opaque right now, because the main idea is only on the wiki page... you could transfer the text from "Implementation" notes into this file.

260

I think this is basically fine, but please give an example to show what is going on here.

277

This is all a bit impenetrable. I think the key thing you are doing here is to match locals to globals. So just do that! From the abe_exports, make an IdEnv Id which maps locals to globals. (cf inline_env). Now you can easily find the locals that don't have a corresponding global, etc.

466

Missing is all the stuff from "Implementation notes" at the bottom of the wiki page.

488

This isn't right. See my comments on mkSelectorBinds, and fix this Note to match! Thanks

compiler/deSugar/DsUtils.hs
614–615

No, this is still not right. For the source code

let False = rhs in body

you are generating

let x = rhs in x `seq` body

But that is wrong. You want

let x = case rhs of { False -> (), True -> error "match fail" }
in x `seq` body

I think you can get this by simply deleting this entire null binders case, and letting the subsequent cases handle it

compiler/deSugar/Match.hs
821

I'm struck by the duplication of this code (plus the Opt_Strict test above) and getUnbangedLPat in DsBinds. (Only it deals with HsPar and the code here does not, which I suspect is not deliberate.) Can't you share the code?

This revision now requires changes to proceed.Oct 16 2015, 11:24 AM
tibbe accepted this revision.Oct 17 2015, 7:43 PM

I'll let Simon PJ deal with the details I don't understand.

docs/users_guide/glasgow_exts.rst
12544

Just an idea. We could require an explicit ~ in front of these if -XStrict is on, similarly to how we require ! in front of unlifted binding if -XStrict is not on. I don't know if it's worth it though. Probably just annoying.

adamse updated this revision to Diff 4754.Oct 29 2015, 8:45 AM
  • Strict: Use IdEnv to find corresponding global vars
  • strict: fix patterns with no binders
  • strict: add some examples to comment
  • strict: share some code
  • strict: add comments
  • strict: expand note
adamse updated this revision to Diff 4755.Oct 29 2015, 8:53 AM
  • Rebase
adamse updated this revision to Diff 4759.Oct 29 2015, 10:46 AM
  • strict: fix compilation
  • strict: beautify users guide entry
  • rebase
adamse updated this revision to Diff 4761.Oct 29 2015, 11:03 AM
  • fix rebase
adamse updated this revision to Diff 4763.Oct 29 2015, 3:00 PM
  • rebase again
adamse added a comment.EditedOct 29 2015, 4:29 PM

Todo: fix new error

Actual stderr output differs from expected:
--- ./deSugar/should_compile/T5455.stderr.normalised	2015-10-29 22:25:37.000000000 +0100
+++ ./deSugar/should_compile/T5455.comp.stderr.normalised	2015-10-29 22:25:37.000000000 +0100
@@ -1,4 +1,8 @@

-T5455.hs:13:13:
-    warning: Pattern match(es) are non-exhaustive
+T5455.hs:8:11: warning:
+    Pattern match(es) are non-exhaustive
+    In a pattern binding: Patterns not matched: []
+
+T5455.hs:13:13: warning:
+    Pattern match(es) are non-exhaustive
              In a pattern binding: Patterns not matched: []
\ No newline at end of file
*** unexpected failure for T5455(normal)

Unexpected results from:
TEST="T5455"

Problem is probably in mkSelectorBinds, which needs some more changes to properly deal with patterns w/o binders.

Relevant ticket Trac #5455, relevant commit rGHCbe53e4fc84f3282d80b25d811f8c8a991fc7b712

adamse added a comment.EditedNov 6 2015, 8:31 AM

@bgamari, @tibbe, @simonpj: T5455 checks that we don't warn for inexhaustive pattern matches in let binds if we never demand the pattern:

w :: String -> String
w x = let (_:_) = x in "1"

should produce no warning which is reasonable, but in the case of -XStrict or a bang pattern:

w :: String -> String
w x = let !(_:_) = x in "1"

the pattern will be demanded, should we warn or not in this case?

Edit: we shall warn!

15:40  bgamari> adamse, with -XStrict that wouldn't fail at runtime, correct?
15:41  adamse> if x is [] it will fail
15:41  bgamari> adamse, then I think we should certainly warn
In D1142#41706, @adamse wrote:

@bgamari, @tibbe, @simonpj: T5455 checks that we don't warn for inexhaustive pattern matches in let binds if we never demand the pattern:

w :: String -> String
w x = let (_:_) = x in "1"

should produce no warning which is reasonable, but in the case of -XStrict or a bang pattern:

w :: String -> String
w x = let !(_:_) = x in "1"

the pattern will be demanded, should we warn or not in this case?

Given that this would fail at runtime I think throwing a warning is the only responsible thing to do here when -XStrict is enabled.

adamse updated this revision to Diff 5061.Nov 12 2015, 9:05 AM
  • make warnings on non-exhaustive pattern matches in let work
adamse updated this revision to Diff 5062.Nov 12 2015, 9:58 AM
  • Update expected GHC extensions

@adamse, this looks pretty reasonable but it would be helpful if you could take a pass through the inline comments and mark those that you have addressed as "done". At the moment it's not clear what has been done and what remains.

compiler/deSugar/DsBinds.hs
234–237

@adamse, did this ever get addressed?

adamse marked 8 inline comments as done.Nov 12 2015, 11:26 AM
adamse added inline comments.
compiler/deSugar/DsBinds.hs
234–237

I'll be making another pass over the Note and make sure that it is referenced at all important places.

adamse updated this revision to Diff 5077.Nov 13 2015, 9:59 AM
adamse marked an inline comment as done.
  • Strict: refer to note
adamse retitled this revision from Implement Strict language extension to Implement the Strict language extension.Nov 13 2015, 10:07 AM
adamse updated this object.
adamse marked 3 inline comments as done.Nov 13 2015, 10:11 AM
adamse added inline comments.
docs/users_guide/glasgow_exts.rst
12544

Agree that it sounds mostly annoying.

bgamari accepted this revision.Nov 14 2015, 3:05 PM

Looks good to me.

This revision was automatically updated to reflect the committed changes.