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.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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. | |
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, 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. |
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` bodyI 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? | |
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. | |
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
@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
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, 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? | |
| 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. | |
| docs/users_guide/glasgow_exts.rst | ||
|---|---|---|
| 12544 | Agree that it sounds mostly annoying. | |