Generalize ApplyBuilder to Monoidal structures #555

julienrf · 2015-10-02T16:22:15Z

I’d like to get your feedback on this work in progress. Here is my plan:

Introduce a Monoidal type class ;
Generalize ApplySyntax to monoidal structures ;
Add imap and contramap methods to MonoidalBuilder (that’s why this PR is actually useful) ;
Monoidal laws ;
Tests ;
Documentation.

The motivation is to be able to express function application (for any arity) within a context, like we can currently do with Apply, but with contravariant (e.g. printers) and invariant (e.g. codecs) structures.

codecov-io · 2015-10-03T12:29:07Z

Current coverage is `86.25%`

Merging #555 into master will decrease coverage by -0.11% as of db46fdd

@@            master    #555   diff @@
======================================
  Files          162     166     +4
  Stmts         2251    2292    +41
  Branches        74      74       
  Methods          0       0       
======================================
+ Hit           1944    1977    +33
  Partial          0       0       
- Missed         307     315     +8

Review entire Coverage Diff as of db46fdd

Powered by Codecov. Updated on successful CI builds.

ceedubs · 2015-10-03T13:56:07Z

@julienrf thanks - exciting stuff! It will take me a little while to digest this, but it looks promising. I'm curious what @mpilquist and @non think, as I know they've talked about this recently.

non · 2015-10-04T12:12:10Z

First of all, thanks @julienrf for getting the ball rolling on this!

I do have some design concerns. I think it would be a mistake to introduce a Monoidal type class that is independent of Applicative, since they have equivalent definitions (i.e. given one of them you can build the other).

I had been imagining (and actually started sketching) a version where product was added to Applicative, and many definitions prefer to use product + map instead of ap. The nice thing here is that folks who have code with Applicative don't have to change much, but that we can rewrite more of our default implementations in terms of product and map (which I think will be more efficient).

For example:

def ap[A, B](fa: F[A], ff: F[A => B]): F[B] =
  product(ff, fa).map { case (f, a) => f(a) }

Does this make sense? Additionally, I could imagine adding a Monoidal type class which extends Functor but only provides product (i.e. there is no pure or equivalent method). This would sit alongside Apply as a precursor to Applicative (so all Applicative instances would be guaranteed to be Monoidal, and all Monoidal instances would be guaranteed to be Functor):

trait Monoidal[F[_]] extends Functor[F] {
  def product(fa: F[A], fb: F[B]): F[(A, B)]
}

(Disclaimer: I've spent the weekend getting sick, so I may not be thinking clearly. Please feel free to push back against this design or complicate it with issues I haven't though of.)

julienrf · 2015-10-04T13:58:51Z

I do have some design concerns. I think it would be a mistake to introduce a Monoidal type class that is independent of Applicative, since they have equivalent definitions (i.e. given one of them you can build the other).

My point is that Monoidal + Functor is indeed equivalent to Apply (ie ap can be defined in terms of product and vice versa), but Monoidal is also useful with contravariant and invariant functors, that’s why I think Monoidal should not be coupled with Functor.

Again, the purpose of this PR is to make the current “ApplyBuilder syntax” available also for contravariant and invariant structures. For instance, suppose that you have the following contravariant typeclass:

trait Showable[A] {
  def show(a: A): String
}

You can implement Monoidal[Showable] and Contravariant[Showable]:

implicit val monoidalShowable: Monoidal[Showable] =
  new Monoidal[Showable] {
    def product[A, B](fa: Showable[A], fb: Showable[B]): Showable[(A, B)] =
      new Showable[(A, B)] {
        def show(ab: (A, B)) = {
          val (a, b) = ab
          fa.show(a) ++ " " ++ fb.show(b)
        }
      }
  }

implicit val contravariantShowable: Contravariant[Showable] =
  new Contravariant[Showable] {
    def contramap[A, B](fa: Showable[A], f: B => A): Showable[B] =
      new Showable[B] {
        def show(b: B) = fa.show(f(b))
      }
  }

Then you can easily build a Showable[Foo], given the following Foo definition:

case class Foo(s: String, i: Int, b: Boolean)

implicit val showableFoo: Showable[Foo] =
  (
    Showable[String] |@|
    Showable[Int] |@|
    Showable[Boolean] |@|
  ).contramap(foo => (foo.s, foo.i, foo.b))

A really useful use case for this would be, in circe, to build JSON codecs for a case class from the JSON codecs of each field of the case class (codecs are invariant structures).

julienrf · 2015-10-04T14:00:26Z

On the other hand, note that my PR changed Apply so that now Apply extends Monoidal (and implements product in terms of ap and map). That’s what makes existing code works without any change (modulo some imports renaming).

julienrf · 2015-10-04T14:04:10Z

core/src/main/scala/cats/Apply.scala

@@ -45,6 +37,9 @@ trait Apply[F[_]] extends Functor[F] with ApplyArityFunctions[F] { self =>
      def F: Apply[F] = self
      def G: Apply[G] = GG
    }
+
+  override def product[A, B](fa: F[A], fb: F[B]): F[(A, B)] = ap(fb)(map(fa)(a => b => (a, b)))


Note that I’m not actually overriding this method, just implementing it, but simulacrum does not accept my code if I omit the override qualifier.

Would you be willing to reverse this? I.e. leave product abstract, and implement ap in terms of product and map? I think the implementation will end up being a bit more efficient.

Ok but then I have to update every place in the code where we create instances of Apply to implement product instead of ap. Are you ok with that?

Oh, and we have another problem: Applicative implements map in terms of ap, so if I implement ap in terms of map we hit an infinite recursion.

I'm leaving a comment on the overall issue which will (hopefully) clear up where I see this going.

non · 2015-10-04T14:10:40Z

Aha, I see. OK, that totally makes sense. As long as Applicative ends up being Monoidal I am happy. I'll have to go back and read this more closely.

non · 2015-10-05T00:29:15Z

core/src/main/scala/cats/data/Streaming.scala

@@ -887,7 +887,7 @@ trait StreamingInstances extends StreamingInstances1 {
      def combine[A](xs: Streaming[A], ys: Streaming[A]): Streaming[A] =
        xs concat ys

-      override def map2[A, B, Z](fa: Streaming[A], fb: Streaming[B])(f: (A, B) => Z): Streaming[Z] =
+      override def map2[A, B, Z](fa: Streaming[A], fb: Streaming[B])(f: (A, B) => Z)(implicit ev: Monoidal[Streaming]): Streaming[Z] =


Why was this change necessary? If you have a Streaming in hand you can easily do map or flatMap without needing a Monoidal instance.

non · 2015-10-05T00:36:08Z

So yeah, everything seems fine except a lot of the added (implicit ev: Monoidal[...]) code seems unnecessary (and ev seems unused). Would it be possible to remove those? Given that the instances in question should be Monoidal themselves, passing another one seems unnecessary.

xuwei-k · 2015-10-05T01:27:58Z

What is the Monoidal laws? Maybe can't define laws only Monoidal.

I think

Monoidal + Functor = Apply
Monoidal + Contravariant = Divide

and Divide has laws.

non · 2015-10-05T01:46:31Z

@xuwei-k Right. If Monoidal abstracts across Functor, Invariant, and Contravariant then the laws need to be specified for each of those combinations.

I wonder if we should add Divide to Cats? I like the way it mirrors Apply.

julienrf · 2015-10-05T07:57:00Z

So yeah, everything seems fine except a lot of the added (implicit ev: Monoidal[...]) code seems unnecessary (and ev seems unused). Would it be possible to remove those?

This parameter is needed just because in all these places we override map2, which is originally defined in Functor and takes this implicit Monoidal[F] parameter.

My reasoning for pulling up map2 into Functor was that Monoidal is sufficient to support the |@| syntax, and then all we need to support mapN is to know if we have a covariant (Functor), contravariant (Contravariant) or invariant (Invariant) structure. So, Monoidal gives you |@| and then Functor gives you map.

Maybe we could find a better design. I need to think a bit about that.

non · 2015-10-09T13:31:43Z

So first of all thanks for bearing with me on this. It's not always obvious how much context one is carrying around in their head until one has to explain it.

One thing we've talked about in Gitter (and agreed on) was that Cats should move away from putting implementations in the default type class traits. Particularly, we wanted to move await from designs which allow one to implement Applicative (or Monad) in a terse but inefficient way (i.e. map via pure and flatMap, or pure and ap), since in practice you pretty much always have to override those implementations for efficiency. We haven't gone through and made this change everywhere yet, but I was taking it as implicit that this should be part of the design.

Concretely, for your PR, here is how I see it fitting together:

Implement all derived methods (like map2) in terms of product and map.
Leave map entirely abstract on Applicative
Add Divide as per @xuwei-k's suggestion
Fix instances to keep them compiling
Add custom syntax for Functor + Monoidal, but leave map2 off of Functor itself (and don't require an implicit map2 in any type class instances)

Does this make sense? If it seems too big, I would be happy to try to get the redesign (away from derived methods) pushed through first, which might make it easier for you to make your changes.

Thanks again for your work on this.

julienrf · 2015-10-09T16:20:17Z

Hi, I reworked a bit the PR. Notably, I achieved points 1, 2, 4 and 5 of your previous comment.

I kept implementing product as a derived method in FlatMap, though.

map2, map3, etc., contramap2, contramap3, etc., imap2, imap3, etc. and tuple2, tuple3, etc. are implemented in Monoidal companion object. Then, they are referenced by monoidal syntax (ie. (fa |@| fb |@| fc).contramap calls Monoidal.contramap3). I also kept the aliases to map3, map4, etc. and tuple3, tuple4, etc. in Apply (we could remove them, though).

Sorry for all the changes…

Before I fix the tuts, can you confirm me that the direction this work is going looks good to you?

non · 2015-10-09T17:03:47Z

Actually let's hold off on (3) for now. I think that should happen in another issue/PR.

This looks great!

In the long run I'd like to try to deprecate the apply builder syntax (i.e. |@|) but let's hold off doing that for now to limit the scope of this PR.

I'm interested to see what everyone else (particularly @mpilquist) thinks of this.

mpilquist · 2015-11-11T00:21:52Z

laws/src/main/scala/cats/laws/MonoidalLaws.scala

+
+import cats.Monoidal
+
+trait MonoidalLaws[F[_]] {


Can we define the left/right identity and associativity laws here, but instead of returning IsEq[X], return a tuple, because the first and second elements will have different but isomorphic types? For example, the left identity law would return a (F[(Unit, A)], F[A]). This stresses that this type class is not lawless, even if we need some type of functor in order to do the equality check for most Fs.

We'd then bind these laws in InvariantTests, using the Invariant[F] instance.

julienrf · 2015-11-21T22:52:30Z

Hi,

I decided not to include pure neither unit into Monoidal. I first tried to include unit, but it had cumbersome consequences: in several places we had to require a Monoid where we previously just required a SemiGroup. When I realized that I had to provide such a Monoid for NonEmptyList, I gave up.

So, here is a Monoidal typeclass with just an associativity law.

adelbertc · 2015-12-06T08:01:55Z

@julienrf sorry for the lack of chatter - I'm taking a closer look now

I may be missing something but I'm not sure where the Semigroup or Monoid comes in. It seems we could split Monoidal out similar to how we split Applicative - one with pure and one without. We'll of course figure out what we want to name the one without pure. It would look something like

trait Semigroupal[F[_]] { // yeah i know
  def zip[A, B](fa: F[A], fb: F[B]): F[(A, B)]
}

trait Monoidal[F[_]] extends Semigroupal[F] {
  def pure[A](a: A): F[A]
}

The Semigroupal laws would just be associativity, and the Monoidal laws would have left and right identity, much like Semigroup and Monoid laws.

Also, sorry again, seem to be merge conflicts now.

julienrf · 2015-12-06T09:21:29Z

Hi, your idea totally makes sense.

The issue I mentionned above with SemiGroup and Monoid is that if Monoidal has this pure method and if I want to implement e.g. Monoidal[Validated] then I need to implement a Monoid[NonEmptyList]. If only implement SemiGroupal[Validated] nothing has to be changed, though (i.e. I only need a SemiGroup[NonEmptyList], which is ok).

So, if I find some more time, it could be interesting to follow your suggestion and rename the Monoidal I introduced into SemiGroupal and then define Monoidal as you did.

adelbertc · 2015-12-06T16:10:10Z

Right, so some data types would only be able to define Semigroupal, others Monoidal

If you are busy these next couple weeks, perhaps we can resolve the merge conflicts and see if we can push what you have here, and then someone else can take over the other work. What are people's thoughts on that?

julienrf · 2015-12-08T11:28:34Z

OK, I just updated my PR to master and resolved the conflicts.

…nctors

adelbertc · 2015-12-09T01:23:45Z

Pinging @mpilquist and @non - do we want to merge this in the interim and add the version with unit later on, or would we prefer a more complete solution?

…nctors

julienrf · 2015-12-10T21:03:12Z

Hi, keeping my branch free of conflicts is a tedious job, if you are ok with that let’s merge this work as it is and we will improve it later by:

renaming Monoidal to Semigroupal ;
introduce Monoidal, which extends Semigroupal and adds pure[A](a: A): F[A], and has identity laws ;
implement some Monoidal instances (e.g. Const, etc.).

I can create the ticket to track progress on this path.

milessabin · 2015-12-10T21:10:09Z

I think it's a shame we have to duplicate the pure signature. A Pure type class, without laws of it's own, but piggybacking on the laws of the type classes that extend it, would be a win here.

mpilquist · 2015-12-11T00:42:45Z

Sorry, I haven't had time to look in detail but I agree that we should merge this and address any issues subsequently.

On Dec 10, 2015, at 4:10 PM, Miles Sabin notifications@github.com wrote:

I think it's a shame we have to duplicate the pure signature. A Pure type class, without laws of it's own, but piggybacking on the laws of the type classes that extend it, would be a win here.

—
Reply to this email directly or view it on GitHub.

adelbertc · 2015-12-11T00:56:09Z

Alright, I'm going to merge this in the interest of getting something up there, and let's address improvements moving forward.

Thanks so much @julienrf , sorry it took a while!

Also, will leave this here for posterity https://gitter.im/non/cats?at=5669e953de5536717680f8b0

Generalize ApplyBuilder to Monoidal structures

stew · 2015-12-12T15:28:52Z

yay

- Reverts many changes introduced by typelevel#555 - Changes `cats.syntax.all._` to `import cats.syntax.monoidal._` in doc

- Remove all equivalent definition of `product` introduced by typelevel#555 - Changes `cats.syntax.all._` to `import cats.syntax.monoidal._` in doc - Minor type argument spacing

julienrf force-pushed the monoidal-functors branch 2 times, most recently from ff5b4c4 to 72aa68e Compare October 3, 2015 11:59

julienrf reviewed Oct 4, 2015
View reviewed changes

non reviewed Oct 5, 2015
View reviewed changes

julienrf force-pushed the monoidal-functors branch 2 times, most recently from 9839dfa to c102c40 Compare October 9, 2015 11:05

julienrf force-pushed the monoidal-functors branch 3 times, most recently from 6ecc616 to 5c4776f Compare October 9, 2015 16:09

julienrf force-pushed the monoidal-functors branch from 5c4776f to c4606c4 Compare October 13, 2015 15:38

julienrf added 2 commits October 15, 2015 18:36

Generalize ApplyBuilder to Monoidal.

00e5ab4

wip laws

1073bbe

julienrf force-pushed the monoidal-functors branch from c4606c4 to 00e5ab4 Compare October 15, 2015 16:49

non mentioned this pull request Nov 2, 2015

Let's talk the next release (again) #594

Closed

ceedubs added this to the cats-1.0.0 milestone Nov 9, 2015

mpilquist reviewed Nov 11, 2015
View reviewed changes

julienrf added 2 commits November 21, 2015 23:46

remove identity laws

7ddb6a7

Merge remote-tracking branch 'origin/master' into monoidal-functors

4e5de08

fix tests

ecd307e

julienrf force-pushed the monoidal-functors branch from f1a2808 to ecd307e Compare November 22, 2015 08:01

julienrf force-pushed the monoidal-functors branch 2 times, most recently from 18dfb2e to 3058538 Compare December 8, 2015 11:42

Merge branch 'master' of https://github.com/non/cats into monoidal-fu…

58b4319

…nctors

julienrf force-pushed the monoidal-functors branch from 3058538 to 58b4319 Compare December 8, 2015 12:32

julienrf added 2 commits December 9, 2015 09:40

Merge branch 'master' of https://github.com/non/cats into monoidal-fu…

4b7d967

…nctors

Merge branch 'master' of https://github.com/non/cats into monoidal-fu…

9e84c04

…nctors

julienrf force-pushed the monoidal-functors branch from e362fc9 to 9e84c04 Compare December 10, 2015 20:21

adelbertc added a commit that referenced this pull request Dec 11, 2015

Merge pull request #555 from julienrf/monoidal-functors

fdf6baf

Generalize ApplyBuilder to Monoidal structures

adelbertc merged commit fdf6baf into typelevel:master Dec 11, 2015

adelbertc mentioned this pull request Dec 11, 2015

Monoidal improvements #740

Closed

OlivierBlanvillain added a commit to OlivierBlanvillain/cats that referenced this pull request Jan 9, 2016

Define Apply.product in term of ap and map

a7b752c

- Reverts many changes introduced by typelevel#555 - Changes `cats.syntax.all._` to `import cats.syntax.monoidal._` in doc

OlivierBlanvillain added a commit to OlivierBlanvillain/cats that referenced this pull request Jan 9, 2016

Define Apply.product in term of ap and map

35341d5

- Reverts many changes introduced by typelevel#555 - Changes `cats.syntax.all._` to `import cats.syntax.monoidal._` in doc

julienrf mentioned this pull request Jan 10, 2016

Add Semigroupal for Monoidal hierarchy, fixes #740 #795

Merged

kailuowang removed this from the 1.0.0-MF milestone May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize ApplyBuilder to Monoidal structures #555

Generalize ApplyBuilder to Monoidal structures #555

julienrf commented Oct 2, 2015

codecov-io commented Oct 3, 2015

ceedubs commented Oct 3, 2015

non commented Oct 4, 2015

julienrf commented Oct 4, 2015

julienrf commented Oct 4, 2015

julienrf Oct 4, 2015

non Oct 5, 2015

julienrf Oct 9, 2015

julienrf Oct 9, 2015

non Oct 9, 2015

non commented Oct 4, 2015

non Oct 5, 2015

non commented Oct 5, 2015

xuwei-k commented Oct 5, 2015

non commented Oct 5, 2015

julienrf commented Oct 5, 2015

non commented Oct 9, 2015

julienrf commented Oct 9, 2015

non commented Oct 9, 2015

mpilquist Nov 11, 2015

mpilquist Nov 11, 2015

julienrf commented Nov 21, 2015

adelbertc commented Dec 6, 2015

julienrf commented Dec 6, 2015

adelbertc commented Dec 6, 2015

julienrf commented Dec 8, 2015

adelbertc commented Dec 9, 2015

julienrf commented Dec 10, 2015

milessabin commented Dec 10, 2015

mpilquist commented Dec 11, 2015

adelbertc commented Dec 11, 2015

stew commented Dec 12, 2015

Generalize ApplyBuilder to Monoidal structures #555

Generalize ApplyBuilder to Monoidal structures #555

Conversation

julienrf commented Oct 2, 2015

codecov-io commented Oct 3, 2015

Current coverage is 86.25%

ceedubs commented Oct 3, 2015

non commented Oct 4, 2015

julienrf commented Oct 4, 2015

julienrf commented Oct 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

non commented Oct 4, 2015

Choose a reason for hiding this comment

non commented Oct 5, 2015

xuwei-k commented Oct 5, 2015

non commented Oct 5, 2015

julienrf commented Oct 5, 2015

non commented Oct 9, 2015

julienrf commented Oct 9, 2015

non commented Oct 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrf commented Nov 21, 2015

adelbertc commented Dec 6, 2015

julienrf commented Dec 6, 2015

adelbertc commented Dec 6, 2015

julienrf commented Dec 8, 2015

adelbertc commented Dec 9, 2015

julienrf commented Dec 10, 2015

milessabin commented Dec 10, 2015

mpilquist commented Dec 11, 2015

adelbertc commented Dec 11, 2015

stew commented Dec 12, 2015

Current coverage is `86.25%`