Implement QualifiedDo by pkamenarsky · Pull Request #3373 · purescript/purescript

pkamenarsky · 2018-06-02T19:27:40Z

Implements #3245.

Now, the following is possible:

IxMonad.purs:

module IxMonad where

class IxMonad m where
  pure ∷ forall a x. a -> m x x a
  bind ∷ forall a b x y z. m x y a -> (a -> m y z b) -> m x z b

Main.purs:

import IxMonad as I

test :: forall m a. I.IxMonad m => m a a String
test = I.do
  a <- I.pure "test"
  b <- I.pure "test"
  I.pure (a <> b)

instead of having to rebind bind explicitly.

There's no change in behaviour if do is left unqualified:

test :: forall m. Monad m => m String
test = do
  a <- pure "test"
  b <- pure "test"
  pure b

If there's interest I'll add support for ado as well.

pkamenarsky · 2018-06-03T12:38:03Z

(Tests fail because tests/support/bower.json was referencing stale 0.12 branches. This has been fixed in the meanwhile, but not sure how to restart CI.)

natefaubion · 2018-11-29T03:49:07Z

I personally like this feature, as I think it makes rebindable-do actually useful. I don't know of anyone using the feature in earnest because the ergonomics are so terrible, so at best I consider rebindable-do a neutral side-effect of not having a hard dependency on Prelude. But I know the reception in the ticket was mixed. I think we should make a decision either way if this is something that we want to merge (now or eventually) rather than letting this continue to bit-rot.

hdgarrood · 2018-12-03T17:21:11Z

@natefaubion: Yes, I agree. I'm a little hesitant for the same reasons I gave in #3245, but I'll admit that I don't remember finding myself in a position where I was wanting to use rebindable syntax like this, and in fact I always did view rebindable syntax as a neutral consequence of separating Prelude from the compiler, rather than a feature that I expected to get much use. But looking back at the ticket, I think the ergonomics argument makes sense. You and @garyb being in favour is sufficient for me to say sure, let's do it.

@pkamenarsky this is perhaps a bit late now, but you can trigger a new CI build by pushing an empty commit, or by rebasing your PR on master.

hdgarrood · 2018-12-03T17:21:51Z

Additionally, if we are doing this, I think we should support ado as well.

pkamenarsky · 2018-12-04T12:39:06Z

do works now, will try to finish ado in the next couple of days in order to get this ready for review.

pkamenarsky · 2018-12-05T17:04:04Z

Ok, I think this might be ready for review 🎉

maddie927 · 2018-12-16T16:38:21Z

I’d love to use this in this react-basic branch! I’ve tried a few different approaches to scoping bind and it’s all pretty gross. It’s also harder to understand (both learning and debugging) than qualified do 🙂

In this example I had to move the inner do block out so they each have their own bind scope:

https://github.com/spicydonuts/purescript-react-basic/blob/hooks/examples/counter/src/Counter.purs

And in this one the “convenience” args I was trying we’re kind of useless for the helper function:

https://github.com/spicydonuts/purescript-react-basic/blob/hooks/examples/controlled-input/src/ControlledInput.purs

natefaubion · 2018-12-16T16:50:15Z

I think the approach is straightforward. The only outstanding question I have is how this interacts with something like docs/pursuit. I know ado broke some stuff, and since this doesn't introduce additional constructors I think we should be OK, but I'd like @hdgarrood to weigh in.

hdgarrood · 2018-12-16T19:28:30Z

This should be fine, I think, since a) the Expr type doesn't get serialized during publishing and b) the code to handle this is all happening inside the parser; there haven't been any changes to the type checker. I'm planning on working on a fix for #3414 which would make the whole docs generation process a fair bit more robust than it currently is anyway.

hdgarrood · 2018-12-16T19:45:25Z

Well I mean there are changes in the desugaring too, but ado is already straight up broken with respect to docs, so this can't make it any worse.

maddie927 · 2018-12-17T18:57:02Z

I gave this branch a try and it works! Counter, ControlledInput

One thing I noticed is that it seemed to work even without bind in my exports list. Not sure if that's intentional or good/bad. I'm also kind of unclear whether discard should be included.

maddie927 · 2018-12-27T17:30:32Z

What are the next steps or blockers here?

natefaubion · 2018-12-27T17:33:45Z

One thing I noticed is that it seemed to work even without bind in my exports list.

Can you explain what you meant by these comments?

maddie927 · 2018-12-27T17:36:41Z

The module I defined top level bind in was using explicit exports. That export list did not include bind, but M.do still worked when imported as import M as M. When I changed the import to import M (bind) as M I got a compile error on the import.

natefaubion · 2018-12-27T17:41:44Z

-                 , parseDo
-                 , parseAdo
+                 , P.try parseDo
+                 , P.try parseAdo


I don't think we want to add backtracking here. This will backtrack on any failure inside a do-block, which will result in poor errors. We only want to apply backtracking to the parsing of the do keyword, which means we need to move this into parseDo and parseAdo.

Done. Interestingly, my intuition was that by using <|> the parser would try consuming the qualified keyword first and only if that fails try and parse the unqualified one:

(parseQualified (reserved "do") >>= pure ∘ getQual) <|> (reserved "do" *> pure Nothing)

However we still need to backtrack explicitly while parsing the qualified version, otherwise every qualified token is expected to be do/ado.

<|> will only fallback to the next branch if you have wrapped in try or you have not consumed any tokens. parseQualified likely consumes the qualification, so when it does not get a do, it will not fallback to the next alternative unless you explicitly wrap it in try.

natefaubion · 2018-12-27T18:31:33Z

 parseDo :: TokenParser Expr
 parseDo = do
-  m <- parseQualified (reserved "do") >>= \(Qualified m _) -> pure m
+  m <- P.try (parseQualified (reserved "do") >>= pure . getQual) <|> (reserved "do" *> pure Nothing)


>>= pure . k is just fmap.

natefaubion · 2018-12-28T04:03:35Z

@spicydonuts I cannot reproduce that issue with this branch. If I don't export bind, I get the expected error. Can you reproduce it with a minimal example? Something would be very awry if that were the case since this desugaring pass is called before name resolution, so to the compiler it's no different than using the operators directly.

maddie927 · 2018-12-28T18:16:05Z

I tried this here and it seems like it was probably just a build caching issue:

Build
Remove bind from the exports in src/React/Basic.purs
Remove the explicit imports on the qualified import of React.Basic in src/React/Basic/Compat.purs (becomes import React.Basic as React)
Build -- no errors
rm -rf output
Build -- Unknown value React.bind

natefaubion · 2018-12-28T18:44:28Z

By "build", do you mean a full compiler build, or ide build?

natefaubion · 2018-12-28T18:45:51Z

In any case, that doesn't sound like an issue with this PR specifically.

maddie927 · 2018-12-28T18:50:44Z

Full, using pulp build for each of those builds. I'm not sure if incremental builds were happening in the background actually.. I'm using VSCode with the ide plugin but I haven't paid much attention to it for this because it can't parse the qualified do syntax.

natefaubion · 2018-12-29T19:06:20Z

@spicydonuts I've tried, but I still can't reproduce an error calling purs compile directly. If you can reproduce it, please open a ticket. Otherwise I'm going to say it's probably IDE related. It doesn't sound like it's related to this feature, specifically.

Does anyone have any other comments?

hdgarrood

Looks good to me!

natefaubion · 2018-12-30T16:37:32Z

Thank you @pkamenarsky for implementing this! Thank you @spicydonuts for trying this out. 🎉

* Implement QualifiedDo * Improve QualifiedDo tests * Implement QualifiedAdo * Trigger CI * Backtrack only while parsing the do/ado keywords * >>= pure . k is just fmap * Clean up tests

Implement QualifiedDo

8678845

pkamenarsky force-pushed the qualified-do branch from 1a8e990 to 8678845 Compare December 4, 2018 08:27

pkamenarsky added 2 commits December 5, 2018 20:02

Improve QualifiedDo tests

fa1b5f4

Implement QualifiedAdo

f581946

Trigger CI

1ab7ad0

maddie927 mentioned this pull request Dec 19, 2018

Hooks? maddie927/purescript-react-basic#1

Closed

natefaubion reviewed Dec 27, 2018

View reviewed changes

Backtrack only while parsing the do/ado keywords

89c3d0d

natefaubion reviewed Dec 27, 2018

View reviewed changes

pkamenarsky added 2 commits December 27, 2018 20:32

>>= pure . k is just fmap

afa32a6

Clean up tests

2f799d3

natefaubion approved these changes Dec 28, 2018

View reviewed changes

hdgarrood approved these changes Dec 30, 2018

View reviewed changes

natefaubion merged commit 6189770 into purescript:master Dec 30, 2018

garyb mentioned this pull request Jan 12, 2019

0.12.2 Release #3508

Closed

3 tasks

Jashweii mentioned this pull request Mar 28, 2019

Qualified do ghc-proposals/ghc-proposals#216

Merged

9 tasks

nsaunders mentioned this pull request Apr 16, 2019

Add support for qualified-do syntax. purescript-hyper/hyper#70

Merged

jamesdbrock mentioned this pull request Aug 13, 2020

qualified do? rowtype-yoga/purescript-arraybuffer-builder#1

Closed

Uh oh!

Conversation

pkamenarsky commented Jun 2, 2018

Uh oh!

pkamenarsky commented Jun 3, 2018

Uh oh!

natefaubion commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdgarrood commented Dec 3, 2018

Uh oh!

hdgarrood commented Dec 3, 2018

Uh oh!

pkamenarsky commented Dec 4, 2018

Uh oh!

pkamenarsky commented Dec 5, 2018

Uh oh!

maddie927 commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natefaubion commented Dec 16, 2018

Uh oh!

hdgarrood commented Dec 16, 2018

Uh oh!

hdgarrood commented Dec 16, 2018

Uh oh!

maddie927 commented Dec 17, 2018

Uh oh!

maddie927 commented Dec 27, 2018

Uh oh!

natefaubion commented Dec 27, 2018

Uh oh!

maddie927 commented Dec 27, 2018

Uh oh!

natefaubion Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

pkamenarsky Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

natefaubion Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

natefaubion commented Dec 28, 2018

Uh oh!

maddie927 commented Dec 28, 2018

Uh oh!

natefaubion commented Dec 28, 2018

Uh oh!

natefaubion commented Dec 28, 2018

Uh oh!

maddie927 commented Dec 28, 2018

Uh oh!

natefaubion commented Dec 29, 2018

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

natefaubion commented Dec 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

natefaubion commented Nov 29, 2018 •

edited

Loading

maddie927 commented Dec 16, 2018 •

edited

Loading

pkamenarsky Dec 27, 2018 •

edited

Loading