Implement BlockArguments (#10843)
ClosedPublic

Authored by akio on Dec 11 2017, 3:12 AM.

Details

Summary

This patch implements the BlockArguments extension, as proposed at
https://github.com/ghc-proposals/ghc-proposals/pull/90. It also fixes Trac #10855
as a side effect.

This patch adds a large number of shift-reduce conflicts to the parser. All of them
concern the ambiguity as to where constructs like if and let end. Fortunately
they are resolved correctly by preferring shift.

The patch is based on @gibiansky's ArgumentDo implementation (D1219).

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.
akio created this revision.Dec 11 2017, 3:12 AM
akio updated this revision to Diff 14906.Dec 11 2017, 3:17 AM
  • Better error message for LambdaCase
mpickering requested changes to this revision.Dec 11 2017, 3:31 AM

Please add an explanation for the number of shift/reduce conflicts to the commit message.

I see you did a very good job of classifying them all in the commit itself which is very useful as well!

compiler/parser/Parser.y
91

36 --> 206 shift/reduce conflicts !?

Some explanation in the commit message please but this seems terrible.

This revision now requires changes to proceed.Dec 11 2017, 3:31 AM
akio edited the summary of this revision. (Show Details)Dec 11 2017, 5:01 AM
akio updated this revision to Diff 14907.Dec 11 2017, 5:02 AM
akio marked an inline comment as done.
  • Parser.y: typo
Harbormaster failed remote builds in B18843: Diff 14907!
akio added a comment.Dec 11 2017, 5:03 AM

@mpickering Updated the commit message to mention shift/reduce conflicts.

akio updated this revision to Diff 14908.Dec 11 2017, 6:44 AM
  • Record a perf regression in parsing001
akio edited the summary of this revision. (Show Details)Dec 11 2017, 6:48 AM
akio added a subscriber: gibiansky.
bgamari accepted this revision.Dec 11 2017, 2:17 PM

Indeed the ambiguities are very well described. Thank you, @akio, for taking the time to do this. I am surprised by the size of the allocation regression in parsing001, but parsing is generally a small enough part of compilation time that I'm not terribly worried about it.

dfeuer added a subscriber: dfeuer.Dec 13 2017, 6:55 PM
dfeuer added inline comments.
compiler/parser/Parser.y
157

Please add more parentheses to disambiguate. Do you mean

(if x then y else z) :: (F a)

or

if x then y else (z :: (F a))
280

What do you mean by "a parenthesized infix type expression of length 1"? Can you write that out more fully?

344

This changed between GHC 7.10 and 8.0. The change is not mentioned in the release notes for 8.0.1 (as far as I can see). The error message is also horrible, and the workaround (a second pair of parentheses around the first) is surprising. Since you're mucking about in this area anyway, do you think you might be able to do something about the error message, documentation, etc?

compiler/parser/RdrHsSyn.hs
845

This whole thing seems a bit awkward. In particular, if someone needs to add a new kind of expression that requires the extension to be allowed in function arguments, then they'll have to know that they need to dig into this particular bit here. Are there other places where these constructors need to be distinguished? Would it make sense to write a function for doing so that enumerates all the constructors of the type?

testsuite/tests/parser/should_compile/BlockArguments.hs
17

Why not import Control.Monad for the definitions of when and forM? If you have a reason, please add a comment explaining it.

testsuite/tests/parser/should_compile/BlockArgumentsLambdaCase.hs
12

Again, why not import forM from Control.Monad, or for from Control.Applicative?

testsuite/tests/parser/should_fail/NoBlockArgumentsFail.hs
6

And again...

testsuite/tests/parser/should_fail/NoBlockArgumentsFail2.hs
4

It's a bit confusing to call this forM. Why not just use something else?

foo :: [Int]
foo = const [1 .. 10] \x -> putStr x
testsuite/tests/parser/should_fail/NoBlockArgumentsFail3.hs
3

Again.

akio added inline comments.Dec 14 2017, 2:45 AM
compiler/parser/Parser.y
157

Yes I could disambiguate it further, but I decided to only add the pair of parentheses that resolves this particular ambiguity, so that it's more clear that which one is being discussed. In this case it's

(if x then y else z :: F) a

vs.

if x then y else z :: (F a)
280

By infix type expression of length 1, I meant a HsAppsTy with a singleton list, whose sole content is a HsAppInfix. If you (or someone else) could suggest a better expression for this, that would be appreciated.

344

I feel like I don't know enough about the field to do anything about it. I can remove this hunk if it's being more confusing than is useful. It's not directly related to the main change after all.

compiler/parser/RdrHsSyn.hs
845

Are there other places where these constructors need to be distinguished?

I can't think of any.

Would it make sense to write a function for doing so that enumerates all the constructors of the type?

Perhaps so, but HsExpr has a lot of constructors. OrPatterns would help.

testsuite/tests/parser/should_compile/BlockArguments.hs
17

No real reason, I just thought details of the test cases didn't matter. I'll clean them up.

dfeuer added inline comments.Dec 14 2017, 2:51 AM
compiler/parser/Parser.y
344

You definitely should not remove it. It's already proved its value!

akio updated this revision to Diff 14944.Dec 14 2017, 5:34 PM
  • Clean up test cases
akio updated this revision to Diff 14979.Dec 24 2017, 7:48 PM
  • Rebase
  • This is not making it in 8.4
akio added a comment.Dec 24 2017, 7:52 PM

Is there something that needs to be done on my side? Or is this revision waiting for review?

In D4260#119359, @akio wrote:

Is there something that needs to be done on my side? Or is this revision waiting for review?

I'm having a look now, thanks for your patience!

bgamari accepted this revision.Jan 7 2018, 12:27 PM

Looks good to me. I can fix the remaining wibbles when I merge.

docs/users_guide/glasgow_exts.rst
2169

It would be better to use .. code-block:: none here and above to avoid this being highlighted as Haskell syntax.

testsuite/tests/perf/compiler/all.T
482

Wow, that is quite a jump.

akio added a comment.Jan 21 2018, 6:40 PM

@mpickering Any comments on the latest version? (I believe this differential revision is waiting for an action from you. Apologies if I'm mistaken)

mpickering accepted this revision.Jan 22 2018, 6:02 AM

No looks good to me. Thanks @akio

This revision is now accepted and ready to land.Jan 22 2018, 6:02 AM
Wizek added a subscriber: Wizek.Jan 22 2018, 10:56 AM
This revision was automatically updated to reflect the committed changes.