Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#13875 closed bug (fixed)

ApplicativeDo desugaring is lazier than standard desugaring

Reported by: dfeuer Owned by: simonmar
Priority: high Milestone: 8.2.1
Component: Compiler Version: 8.3
Keywords: ApplicativeDo Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3681
Wiki Page:

Description

Suppose I write

import Data.Maybe (isJust)

bangy m = do
  (_,_) <- m
  return ()

main = print $ isJust (bangy (Just undefined))

If I compile this with ApplicativeDo, it prints True. Otherwise, it throws an exception. The basic problem is that the (correct) bangy function requires a Monad constraint, but ApplicativeDo replaces it with a lazier function that can get by with Functor. I believe it should desugar correctly, and impose a Monad constraint here. To get the Functor constraint should require an explicit lazy pattern match:

bangy m = do
  ~(_,_) <- m
  return ()

Change History (18)

comment:1 Changed 2 years ago by dfeuer

Note that the documentation indicates

Applicative do-notation desugaring preserves the original semantics, provided that the Applicative instance satisfies <*> = ap and pure = return (these are true of all the common monadic types). Thus, you can normally turn on -XApplicativeDo without fear of breaking your program. There is one pitfall to watch out for; see Things to watch out for.

This is simply not true at the moment. The "Things to watch out for" section is unrelated.

comment:2 Changed 2 years ago by dfeuer

Milestone: 8.4.18.2.1
Priority: normalhigh

I'm setting a milestone of 8.2.1 and high priority to at least document what the extension actually does right now. If the powers that be want to fix it, then we can set a milestone of 8.4 for the fix if necessary.

comment:3 Changed 2 years ago by simonmar

Wow, you're right. I hadn't realised this, and I think you're the first person to point it out.

It's not limited to Functor, the problem occurs with Applicative too:

Prelude Data.Maybe> :set -XApplicativeDo 
Prelude Data.Maybe> let f m = do () <- m; () <- m; return ()
Prelude Data.Maybe> :t f
f :: Applicative f => f () -> f ()
Prelude Data.Maybe> isJust (f (Just undefined))
True

To fix this properly we would have to prevent ApplicativeDo from applying to any statement with a strict pattern match. In practice I doubt anyone is going to write ~p to make ApplicativeDo work, but fortunately it still works for simple variable patterns.

Ugh, I don't know what the best fix is here, but I agree at the very least we need some documentation.

comment:4 Changed 2 years ago by simonpj

Owner: set to simonmar

It's a subtle issue, not described in the paper. So yes we need something in the user manual, but a longer Note somewhere, linking to this ticket, giving example(s), and explaining our design choice, would be helpful.

To fix this properly we would have to prevent ApplicativeDo from applying to any statement with a strict pattern match.

That would not be hard, would it?

comment:5 in reply to:  3 Changed 2 years ago by dfeuer

Replying to simonmar:

To fix this properly we would have to prevent ApplicativeDo from applying to any statement with a strict pattern match. In practice I doubt anyone is going to write ~p to make ApplicativeDo work, but fortunately it still works for simple variable patterns.

It seems terribly unfortunate, in my opinion, if adding a language extension to a module can, all by itself, introduce a memory leak. The fix does seem annoying, but I don't think there's another good way. The current behavior is both surprising and inconsistent. We generally expect, for example, that

  (x,y) <- m
  e

will be exactly the same as

  xy <- m
  case xy of
    (x,y) -> e

but that's not currently the case. Here's an (arguably) more extreme example:

oops m = do
  !xy <- m
  pure (fst xy + snd xy)

With ApplicativeDo, the bang pattern is silently ignored.

So I think the special desugaring should only be used if all the patterns are lazy. It might make sense to check whether an expression could otherwise be desugared specially, and offer an optional warning in that case.

comment:6 Changed 2 years ago by dfeuer

Yet another few thoughts. This is a situation that could be improved offering, in addition, separate ado syntax. Such syntax would offer deeply-lazy-always pattern matching, prohibit inappropriate bangs, and throw an error if desugaring would require >>=. Along with setting laziness appropriately, this would allow the user to declare that they expect applicative optimizations to kick in, even when a monad is actually available.

While I'm mentioning idle thoughts: monad comprehension syntax seems to handle the pure/return/pure $/whatever issue a lot more cleanly than anything do-like. Has anyone considered applicative comprehensions?

comment:7 Changed 2 years ago by simonmar

This bug makes #13242 moot, incidentally.

comment:8 Changed 2 years ago by simonmar

Differential Rev(s): Phab:D3681

comment:9 Changed 2 years ago by Iceland_jack

I have an idea for ado (fdo similarly), restricting the do-notation to be Applicative or weaker. It would make the user intent's clearer, "this should not have a Monad constraint" and paves the way for a wealth of Shakespearean puns

Last edited 2 years ago by Iceland_jack (previous) (diff)

comment:10 Changed 2 years ago by bgamari

Status: newpatch

comment:11 Changed 2 years ago by Ben Gamari <ben@…>

In 1ef4156/ghc:

Prevent ApplicativeDo from applying to strict pattern matches (#13875)

Test Plan:
* New unit tests
* validate

Reviewers: dfeuer, simonpj, niteria, bgamari, austin, erikd

Reviewed By: dfeuer

Subscribers: rwbarton, thomie

GHC Trac Issues: #13875

Differential Revision: https://phabricator.haskell.org/D3681

comment:12 Changed 2 years ago by bgamari

Status: patchmerge

comment:13 Changed 2 years ago by bgamari

Resolution: fixed
Status: mergeclosed

comment:14 Changed 2 years ago by Ben Gamari <ben@…>

In af403b2/ghc:

ApplicativeDo: document behaviour with strict patterns (#13875)

Test Plan: unit tests, built docs

Reviewers: dfeuer, bgamari, simonpj, austin, erikd

Subscribers: rwbarton, thomie

GHC Trac Issues: #13875, #13242

Differential Revision: https://phabricator.haskell.org/D3691

comment:15 Changed 2 years ago by Ben Gamari <ben@…>

In ccb849f8/ghc:

users-guide/rel-notes: Describe #13875 fix

Test Plan: Read it.

Reviewers: simonmar, austin

Reviewed By: simonmar

Subscribers: rwbarton, thomie

GHC Trac Issues: #13875

Differential Revision: https://phabricator.haskell.org/D3710

comment:16 in reply to:  6 Changed 2 years ago by ElvishJerricco

Replying to dfeuer:

While I'm mentioning idle thoughts: monad comprehension syntax seems to handle the pure/return/pure $/whatever issue a lot more cleanly than anything do-like. Has anyone considered applicative comprehensions?

This may no longer be the right place to answer this question, but I do think that using in would be better than special casing return, pure, and $.

do
  a <- foo
  b <- bar
  in a + b

Seems more consistent with let-binding syntax. Though there is definitely some value in desugarring those functions since it means ApplicativeDo can be a drop-in optimization.

comment:17 Changed 2 years ago by andrewthad

I'm not sure if this has already been done, but the "Existentials patterns and GADTs" section of the user manual should probably be removed.

https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/glasgow_exts.html#existential-patterns-and-gadts

comment:18 Changed 2 years ago by Simon Marlow <marlowsd@…>

In 4a677f76/ghc:

Remove section about ApplicativeDo & existentials (#13875)

Summary:
This section is irrelevant now that strict pattern matches don't get
the ApplicativeDo treatment.

Test Plan:
```
make html FAST=YES
```

Reviewers: bgamari, austin, erikd

Subscribers: rwbarton, thomie

GHC Trac Issues: #13875

Differential Revision: https://phabricator.haskell.org/D4087
Note: See TracTickets for help on using tickets.