Support ArgumentDo (fixes #10843)
AbandonedPublic

Authored by bgamari on Sep 5 2015, 3:30 AM.

Details

Reviewers
gibiansky
austin
Trac Issues
#10843
Summary

Experimental change to add an ArgumentDo extension

Test Plan

Test that old parser acts as before

gibiansky updated this revision to Diff 4081.Sep 5 2015, 3:30 AM
gibiansky retitled this revision from to Support ArgumentDo (fixes #10843).
gibiansky updated this object.
gibiansky edited the test plan for this revision. (Show Details)
gibiansky updated the Trac tickets for this revision.
bgamari requested changes to this revision.Sep 5 2015, 8:51 AM
bgamari edited edge metadata.

@gibiansky, it looks like there is a validation failure.

This revision now requires changes to proceed.Sep 5 2015, 8:51 AM
gibiansky updated this revision to Diff 4089.Sep 5 2015, 9:25 AM
gibiansky edited edge metadata.
  • ArgumentDo only applies to do blocks, not list comprehensions, etc

I've fixed the validation failure for now (and will fix more if they come), but I think the real question is not whether the code is correct right now, but whether we actually want an extension like this...

Of course, this is clearly a question which will need to be addressed; I just thought I would make sure your are aware of the issue.

I'm not opposed to this but not strongly in favor either. IIRC we've had special type-checker behavior for ($) for some time now specifically to enable the function $ do ... pattern. In light of this it isn't so strange to modify the grammar to eliminate this pattern entirely. Moreover, this feels similar to how record construction syntax can be used with function application, function ARecord { ... }.

On the other hand, I've never felt terribly burdened by the extra $.

IIRC we've had special type-checker behavior for ($) for some time now specifically to enable the function $ do ... pattern.

We do? How does it relate to type-checking? I think there is (or used to be) special behavior so that $ works with any kind...

@gibiansky: Obviously this change is lacking an update to the documentation, but of course you can be lazy and add only when the change itself is going to be accepted.

I’ll discuss the proposal in the ticket; given that Phab (IMHO) is for code review, and not so much for design discussions.

@nomeata: Here's some info on the $ hack from StackOverflow.

This patch is missing a bunch more stuff:

  • Renaming ArgumentDo to something else
  • Fix the parser for lambdas in addition to do
  • Add documentation
  • Decide about LambdaCase
  • Testing to make sure we don't allow more parses when ArgumentDo is off

I just wanted to get some code on Phabricator to make it clear that I'm planning on tackling this and getting it through if we decide it's useful :)

gibiansky updated this revision to Diff 4093.Sep 5 2015, 1:32 PM
gibiansky edited edge metadata.
  • Parse lambdas without dollar sign
  • Fix test that detects ghc-only extensions
gibiansky updated this revision to Diff 4094.Sep 5 2015, 2:16 PM
gibiansky edited edge metadata.
  • Fix tests for lambda case and error messages

I got validation errors in parsing001. This seems to be a performance test. I don't think my parser changes in any way should change the parsing of the parsing001 test... any ideas as to why this is failing? (I restarted the build to see if its intermittent...)

gibiansky updated this revision to Diff 4095.Sep 5 2015, 5:00 PM
gibiansky edited edge metadata.
  • Add strictness in parser to avoid allocation

Hooray, it passes ./validate now.

The following is left undone:

  • Renaming ArgumentDo to something else (Bikeshedding)
  • Adding documentation to Ghc User Guide

Is there anything else I'm missing? (other than deciding if this is a change we actually want :) )

IIRC we've had special type-checker behavior for ($) for some time now specifically to enable the function $ do ... pattern.

We do? How does it relate to type-checking? I think there is (or used to be) special behavior so that $ works with any kind...

Yes. $ has its own typing rule. See Typing rule for ($). It's not about kinds (although there is some kind-y stuff in there), but more about allowing $ to be impredicative, allowing runST $ do .... In any case, this patch doesn't get involved with any of this.

goldfire added inline comments.Sep 6 2015, 9:34 PM
compiler/parser/Parser.y
2126

This will all need some careful study once we agree to proceed.

compiler/parser/RdrHsSyn.hs
754

This is a bit fishy. You're relying here on the pretty-printer doing the wrong thing! The pretty-printer will currently insert parentheses around do, etc., blocks, even when the user didn't write parens here. But this is not the way the pretty-printer should work. (See also commentary around https://phabricator.haskell.org/D1114#30897) The reason this happens at all is because code generated within GHC is sometimes lazy about inserting parentheses, and so the pretty-printer tries to patch this up.

Bottom line: you're much better off inserting an HsPar if you want to be sure to get paren'd output here.

@goldfire: I would actually prefer the pretty printer work properly. I added that because I didn't realize it was working wrong. It used to say something like, "there is an error in the do block below, do you want Argument Do". But then there were parens, so it didn't make sense, because GHC had fixed the error when pretty printing, so the user would be confused, because the shown code was not what they typed, and the shown code was actually NOT an error. So if the pretty printer is fixed, I'll change the error message back to what it was originally (that is what I would prefer).

simonpj added a subscriber: simonpj.Sep 7 2015, 2:48 AM

@goldfire: I would actually prefer the pretty printer work properly.

What do you mean by "properly"? The principle is that the pretty printer for HsSyn should print what the user wrote. If she used parens, print parens; if not don't.

What do you mean by "properly"? The principle is that the pretty printer for HsSyn should print what the user wrote. If she used parens, print parens; if not don't.

Yes, but that's not what happens. Search around in HsExpr.ppr_expr for calls of pprParendExpr. I argue that pprParendExpr should never be called from HsExpr.ppr_expr. From a previous look at all this (https://phabricator.haskell.org/D1114#30897), it seems that recursive use of pprParendExpr is needed to cope with HsExprs that are created programmatically, and therefore might lack necessary HsPars.

In the other thread, I said that the Right Fix was to go through all code that produces HsExprs and insert HsPars where necessary, and then to remove recursive uses of pprParendExpr. But this hasn't happened. If you agree with this, I'll post a ticket.

I said that the Right Fix was to go through all code that produces `HsExpr`s and insert `HsPar`s where necessary, and then to remove recursive uses of `pprParendExpr`. But this hasn't happened. If you agree with this, I'll post a ticket.

OK yes I agree with that. Really very little code is produced automatically except by 'deriving'.

OK. See Trac #10854 for the ticket about pprParendExpr.

austin requested changes to this revision.Oct 12 2015, 9:25 PM
austin edited edge metadata.

(I haven't looked closely at the diff, but punting to Request Changes to move it out of the review queue, since I haven't entirely seen the conclusions of supporting this extension yet re: Trac #10843)

This revision now requires changes to proceed.Oct 12 2015, 9:25 PM

My conclusion was that support for the extension was very mixed, and that I
do not think the extension makes sense without overall approval from the
community. I am not planning on pushing this any further.

bgamari commandeered this revision.Oct 23 2015, 9:56 AM
bgamari abandoned this revision.
bgamari edited reviewers, added: gibiansky; removed: bgamari.

Abandoning.