Improve Applicative definitions
ClosedPublic

Authored by dfeuer on Nov 3 2014, 4:46 PM.

Details

Summary

Generally clean up things relating to Applicative and Monad in Base.lhs
and Applicative.hs to make Applicative feel like a bit more of a
first-class citizen rather than just playing second fiddle to Monad. Use
coerce and GND to improve performance and clarity.

Change the default definition of (*>) to use (<$), in case the
Functor instance optimizes that.

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.
dfeuer updated this revision to Diff 1225.Nov 3 2014, 4:46 PM
dfeuer retitled this revision from to Improve Applicative definitions.
dfeuer updated this object.
dfeuer edited the test plan for this revision. (Show Details)
dfeuer added reviewers: austin, hvr, ekmett.
dfeuer updated this revision to Diff 1226.Nov 3 2014, 5:49 PM
dfeuer edited edge metadata.

Adjust expected test output.

dfeuer updated this revision to Diff 1229.Nov 3 2014, 8:13 PM

Adjust T9020 to expect less allocation.

hvr requested changes to this revision.Nov 5 2014, 2:47 AM
hvr edited edge metadata.
hvr added inline comments.
libraries/base/Control/Applicative.hs
75–76

left-over comment?

libraries/base/GHC/Base.lhs
433–442

Why only Maybe? (and not also Either, (,) a, [a], ...?)

558–566

I wonder if we should add liftA4 and liftA5 for consistency

@ekmett, any comment?

620

unrelated whitespace change

1049–1050

I'm not convinced this documentation change is sensible ("strict let" assumes the knowledge of the BangPatterns extension which is not part of Haskell2010, and is supposed to be isomorphic to the use of seq)

Moreover, it's off-topic for this patch

1124

off-topic change

This revision now requires changes to proceed.Nov 5 2014, 2:47 AM
dfeuer added inline comments.Nov 5 2014, 7:25 AM
libraries/base/Control/Applicative.hs
75–76

Yes indeed. I stripped it out of a previous version, but forgot to do so this time.

libraries/base/GHC/Base.lhs
433–442

Dunno. If we're making a laundry list, we should add ST as well.

558–566

Yeah, probably.

620

Sorry.

1049–1050

Supposed to be sort of. See the below-mentioned Trac #2273, which seems to demonstrate a long history of failures in that regard, their cause summed up right near the beginning by Simon Peyton Jones's comment "This example has made me realise that what is really wrong here is that seq should really have a type more like fseq :: a -> (a -> b) -> b".

Yes, seq is the standard's way of expressing this concept, but it's not what's in the code. I think it's rather confusing when the documentation makes a claim that's patently false.

1124

Sorry; would you like me to strip that out and submit it separately?

dfeuer updated this revision to Diff 1276.Nov 5 2014, 6:12 PM
dfeuer edited edge metadata.

Back off irrelevant changes; rebase; improve documentation

dfeuer updated this revision to Diff 1277.Nov 5 2014, 6:22 PM
dfeuer edited edge metadata.

More cleanup.

dfeuer updated this revision to Diff 1278.Nov 5 2014, 8:28 PM

Add specialization pragmas for liftA* and ap for Either, ST.

dfeuer updated this revision to Diff 1280.Nov 5 2014, 9:05 PM

Fix Either error.

dfeuer updated this revision to Diff 1282.Nov 5 2014, 9:20 PM

Revert ST specialization--it look like it might trigger some kind of
wonky problem (bug in 7.6.3?)

dfeuer updated this revision to Diff 1285.Nov 5 2014, 10:11 PM

It works for me...

dfeuer updated this revision to Diff 1288.Nov 5 2014, 10:52 PM

Revert compiler util State change to try to make things
work.

dfeuer updated this revision to Diff 1291.Nov 6 2014, 1:54 AM

No one seems to want liftA4 or liftA5, so forget 'em.

hvr added inline comments.Nov 6 2014, 2:00 AM
libraries/base/GHC/Base.lhs
1049–1050

btw, this still is off-topic in this patch :-)

dfeuer updated this revision to Diff 1293.Nov 6 2014, 2:13 AM

Line break issue; remove contested change.

dfeuer updated this revision to Diff 1297.Nov 6 2014, 2:30 AM

Missing line in type sig

hvr added a subscriber: snoyberg.EditedNov 6 2014, 5:33 AM
In D432#10913, @dfeuer wrote:

No one seems to want liftA4 or liftA5, so forget 'em.

Btw, the discussion on libraries@ (specifically @snoyberg's comment) reminded me that @ekmett considered simply generalising liftM... from Monad m => to Applicative m => as part of AMP/BBP.

At the time I was a bit on the fence (well, it has M in its name, and I thought given there's already liftA... family, it would look odd to have liftM... be just an alias). But now I think my reservations were unfounded. (However, I'd suggest pulling that out into a separate code-rev, if you want to go that route)

dfeuer updated this revision to Diff 1300.Nov 6 2014, 3:40 PM

Back off liftM_ = liftA_; let's hope that wasn't the source of
the performance gain.

dfeuer updated this revision to Diff 1302.Nov 6 2014, 4:14 PM

Start doing some of these elsewhere.

dfeuer updated this revision to Diff 1307.Nov 6 2014, 6:57 PM

Derive Monad for WrappedMonad as I did before.

ekmett edited edge metadata.Nov 7 2014, 12:36 AM

Went through this in some detail. It looks good to me.

ekmett accepted this revision.Nov 7 2014, 12:36 AM
ekmett edited edge metadata.

Went through this in some detail. It looks good to me.

this look great. nice work

libraries/base/GHC/Base.lhs
454

should this be = or == for these relations between pure and return?
seems like it should be == to denote semantic rather than definitional equality. Or maybe some thing along those lines?

hvr accepted this revision.Nov 7 2014, 1:18 AM
hvr edited edge metadata.

Macro sealofapproval:

dfeuer updated this revision to Diff 1309.Nov 7 2014, 1:19 AM
dfeuer edited edge metadata.

Define list monad operations using comprehensions

hvr added a comment.Nov 7 2014, 1:19 AM
In D432#11070, @carter wrote:

this look great. nice work

btw, please use the "accept revision" more often if you consider a patch well done :-)

hvr added inline comments.Nov 7 2014, 1:22 AM
libraries/base/GHC/Base.lhs
454

I tend to use the equiv-sign for laws, see also https://phabricator.haskell.org/D449#10941

This revision was automatically updated to reflect the committed changes.