Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ContravariantMonoidal #2034

Merged
merged 14 commits into from
Dec 1, 2017

Conversation

stephen-lazaro
Copy link
Contributor

@stephen-lazaro stephen-lazaro commented Nov 22, 2017

Adds the Divisible typeclass as discussed in #1935.
I went ahead and opened this against core though it was suggested it might belong more in more, just because there seems to be thoughts and discussion around #1940 still. This way, we can get something that's relatively ready to merge when/if more is a thing. I'd be happy to move things over at that time.

Anyway, some notes:

  • I switched the names from divide and conquer to contramap2 and unit respectively, since that seems a little more in line with the naming standards in cats. Welcome to bike shedding on that front.

  • While implementing, I noticed that the only real contribution Divisible gives is the presence of a left and right unit along the diagonal, since in point of fact contramap2 is perfectly well implementable on ContravariantSemigroupal as:

def contramap2[A, B, C](fb: F[B], fc: F[C])(f: A => (B, C)): F[A] =
  contramap(product(fb, fc))(f)

I could see the impedance of adding another typeclass being high enough that maybe it'd be more reasonable just to add some enrichment to ContravariantSemigroupal. I could also see having both, or just Divisible. Not sure what the most desirable outcome would be here.

  • I did not add an instance for Show because unit seems a little against the spirit of the thing, for that particular instance. The only lawful instance AFAICT is the constantly empty string unit induced by the A => B where B : Monoid.

  • The branch name is a bit of a misnomer because I decided not to implement Decideable cause honestly I had trouble parsing the laws on that typeclass in the primary source here:
    https://hackage.haskell.org/package/contravariant-1.4/docs/Data-Functor-Contravariant-Divisible.html#t:Divisible
    I'll probably take another stab at it unless someone beats me to it.

  • Added some tut documentation, but am happy to drop it entirely if its unwanted.

  • I couldn't figure out how to the higher arity stuff works in Apply so I didn't take a shot implementing a dual here. If someone points me in the right direction and it is desired, I'd be happy to try.

  • I added an attribution to ekmett in the Scala doc for Divisible. Hopefully, it satisfies that requirement, but if we have a format I've not met, please do let me know.

First time contributing to this repo so I'm sure I missed something. Thanks preemptively for your solicitous review, as well as your forbearance. :)

Adds Divisible typeclass
Adds laws for Divisible and tests for relevant instances
Adds documentation for Divisible typeclass
Adds instances for:
 - Const
 - Nested
 - Tuple2K
 - Function with Monoid codomain
 - ReaderT
 - Kleisli
 - IdT
 - Eq
 - Equiv
 - Order
 - Ordering
 - OptionT
 - WriterT
 - IndexedStateT
Adds tests
@julienrf
Copy link
Contributor

I did not add an instance for Show because unit seems a little against the spirit of the thing, for that particular instance.

Indeed, for some typeclasses there is no sensible unit implementation. I think this is why we should have both Divisible and ContravariantSemigroupal.

Also, since we don’t use the names divide and conquer I’m not sure the Divisible name makes sense. To me it is more intuitive to think of it as a ContravariantMonoidal (since it extends ContravariantSemigroupal with a unit method).

* Given two values in the Divisible context, and a function producing a value of both types,
* yields an element of the domain of the function lifted into the context.
*/
def contramap2[A, B, C](fb: F[B], fc: F[C])(f: A => (B, C)): F[A]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not provide a default implementation in terms of contramap and product? (like we do for covariant semigroupal: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Apply.scala#L44-L50 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense, consistency is pretty valuable. I mostly went this approach based on the primary document.

@stephen-lazaro stephen-lazaro changed the title Add Divisible Add ContravariantMonoidal Nov 22, 2017
Renames to ContravariantMonoidal
Derives contramap2 from product, contramap
Moves contramap2 to ContravariantSemigroupal
@stephen-lazaro
Copy link
Contributor Author

Renamed to ContravariantMonoidal and used product and contramap to implement contramap2 on ContravariantSemigroupal.

@@ -113,14 +113,17 @@ private[cats] trait ComposedContravariantCovariant[F[_], G[_]] extends Contravar
F.contramap(fga)(gb => G.map(gb)(f))
}

private[cats] trait ComposedApplicativeDivisible[F[_], G[_]] extends Divisible[λ[α => F[G[α]]]] { outer =>
private[cats] trait ComposedApplicativeContravariantMonoidal[F[_], G[_]] extends ContravariantMonoidal[λ[α => F[G[α]]]] { outer =>
Copy link
Contributor

@julienrf julienrf Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t repeat Monoidal here because Applicative already implies Monoidal since they are equivalent (see also this detailed explanation: https://stackoverflow.com/questions/23316255/lax-monoidal-functors-with-a-different-monoidal-structure).

Edit: sorry I was wrong. This trait composes a covariant applicative with a contravariant one, so the naming is correct, you can ignore my comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SO was a good read, anyway, it clarified some of the points on different Monoidal structures for me, so thank you.

(F.unit).contramap2(fa)(f) <-> fa.contramap(f andThen (_._2))

def contravariantMonoidalContramap2DiagonalAssociates[A](m: F[A], n: F[A], o: F[A]): IsEq[F[A]] =
(m.contramap2(n)(delta[A])).contramap2(o)(delta[A]) <-> m.contramap2(n.contramap2(o)(delta[A]))(delta[A])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this law could go to ContravariantSemigroupal

F.map(fa)(G.contramap(_)(f))

override def product[A, B](fa: F[G[A]], fb: F[G[B]]) =
F.map2(fa, fb)(G.product(_,_))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a space after the comma here :)


override def unit[A]: F[G[A]] = F.pure(G.unit)

override def contramap[A, B](fa: F[G[A]])(f: B => A) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add type annotations here and for product? :)

(a: A) => f(a) match {
case (b, c) => fb.run(b) && fc.run(c)
})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tut fails here, because contramap2 is missing an override modifier and contramap and product are unimplemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you. Nice to have tut catch these things.

@@ -12,4 +12,7 @@ import simulacrum.typeclass
def F = self
def G = Functor[G]
}

def contramap2[A, B, C](fb: F[B], fc: F[C])(f: A => (B, C)): F[A] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure if we need this definition here, as we already have contramap2- contramap22 in the auto-generated SemigroupalArityFunctions. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could mix in the SemigroupalArityFunctions into the companion object of ContravariantSemigroupal maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes! I agree. I didn't realize they were in SemigroupalArityFunctions. I'll do that.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a couple of comments, I think it looks really good already, so thank you very much! Also thanks a lot to @julienrf for reviewing! :)

@LukaJCB LukaJCB added this to the 1.0.0 milestone Nov 25, 2017
source: "core/src/main/scala/cats/ContravariantSemigroupal.scala"
scaladoc: "#cats.ContravariantSemigroupal"
---
# Contravariant Semigroupal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're kind of mixing the Monoidal and the Semigroupal in this doc. Maybe we should fix it all to be ContravariantMonoidal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, I'll pin it all to ContravariantMonoidal.

@LukaJCB LukaJCB mentioned this pull request Nov 26, 2017
3 tasks
@stephen-lazaro
Copy link
Contributor Author

stephen-lazaro commented Nov 27, 2017

Switched everything over to use the SemigroupalArityFunctions and the TupleSemigroupalSyntax.
Thanks for reviewing, both @julienrf and @LukaJCB.
(And thank you for lending a hand on the MIMA exceptions @LukaJCB, hadn't seen that before).

@LukaJCB
Copy link
Member

LukaJCB commented Nov 27, 2017

Seems there's still some binary compatibly issues:

Cats core: found 2 potential binary incompatibilities while checking against org.typelevel:cats-core_2.10:1.0.0-RC1  (filtered 177)�[0m
�[0m[�[31merror�[0m] �[0m * abstract method catsSyntaxContravariantSemigroupal(java.lang.Object,cats.ContravariantSemigroupal)cats.ContravariantSemigroupal#Ops in trait cats.syntax.ContravariantSemigroupalSyntax is inherited by class AllSyntax in current version.�[0m
�[0m[�[31merror�[0m] �[0m   filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("cats.syntax.ContravariantSemigroupalSyntax.catsSyntaxContravariantSemigroupal")�[0m
�[0m[�[31merror�[0m] �[0m * abstract method catsSyntaxContravariantMonoidal(java.lang.Object,cats.ContravariantMonoidal)cats.syntax.ContravariantMonoidalOps in trait cats.syntax.ContravariantMonoidalSyntax is inherited by class AllSyntax in current version.�[0m
�[0m[�[31merror�[0m] �[0m   filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("cats.syntax.ContravariantMonoidalSyntax.catsSyntaxContravariantMonoidal")�

If you could add those two to the build.sbt, I think we're done. Thanks again!

Edit:
I quickly added it myself :)

@codecov-io
Copy link

codecov-io commented Nov 27, 2017

Codecov Report

Merging #2034 into master will decrease coverage by 0.6%.
The diff coverage is 90.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2034      +/-   ##
==========================================
- Coverage   95.25%   94.64%   -0.61%     
==========================================
  Files         305      318      +13     
  Lines        5179     5363     +184     
  Branches      126      127       +1     
==========================================
+ Hits         4933     5076     +143     
- Misses        246      287      +41
Impacted Files Coverage Δ
...src/main/scala/cats/ContravariantSemigroupal.scala 66.66% <ø> (ø) ⬆️
...n/scala/cats/syntax/contravariantSemigroupal.scala 0% <0%> (ø)
...main/scala/cats/syntax/contravariantMonoidal.scala 0% <0%> (ø)
...re/src/main/scala/cats/ContravariantMonoidal.scala 0% <0%> (ø)
...s/laws/discipline/ContravariantMonoidalTests.scala 100% <100%> (ø)
core/src/main/scala/cats/data/IdT.scala 97.61% <100%> (+0.25%) ⬆️
core/src/main/scala/cats/data/Tuple2K.scala 95% <100%> (+0.55%) ⬆️
...in/scala/cats/laws/ContravariantMonoidalLaws.scala 100% <100%> (ø)
core/src/main/scala/cats/data/Nested.scala 95.23% <100%> (+0.41%) ⬆️
core/src/main/scala/cats/data/WriterT.scala 93.13% <100%> (-1.55%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab5433e...6e7a1b5. Read the comment docs.

@@ -13,3 +13,5 @@ import simulacrum.typeclass
def G = Functor[G]
}
}
object ContravariantSemigroupal extends SemigroupalArityFunctions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but can we get rid of these curly braces? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, yeah, for sure.

@LukaJCB
Copy link
Member

LukaJCB commented Nov 27, 2017

I wonder, can we create instances for EitherK in the way we can for Applicative?

```tut:book:silent
import cats.Contravariant

trait ContravariantMonoidal[F[_]] extends Contravariant[F] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we shouldn't use tut here as it shadows the real ContravariantMonoidal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could still use tut here then add a tut:reset after (explained here) so that it has no effect after.

@stephen-lazaro
Copy link
Contributor Author

stephen-lazaro commented Nov 27, 2017

Yeah, that seems like a no brainer. I'll do that. Thanks for taking care of the binary compatibility issues. I think I'm not running the check correctly locally, because it keeps telling me it passes 🤔

EDIT: Doesn't look like EitherK is in the having, product breaks down since there's no sensible way to combine F[A] and G[B] in the mixed Right/Left case. Will leave to someone cleverer ;)

@LukaJCB
Copy link
Member

LukaJCB commented Nov 27, 2017

Yeah, you probably need to switch to scala 2.11.11 to see the breakage.

@kailuowang
Copy link
Contributor

we squash merge, did you see my comment on using tut:reset?

@stephen-lazaro
Copy link
Contributor Author

Just did, will do 👍 Thanks for the pointer.

LukaJCB
LukaJCB previously approved these changes Nov 27, 2017
The `ContravariantMonoidal` type class is for [`Contravariant`](contravariant.html) functors that can define a
`product` function and a `unit` function.

```tut:reset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't make it clear. I was suggesting using a tut:silent here and tut:reset:silent for the next block at line 41.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, I was just starting to debug this. Ok, cool, sorry about this, I'm not used to testing against multiple versions of the compiler. I'll fix shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, checked it against 2.10.6, 2.11.11, and 2.12.3 and the docs all compile with that recommendation. So we should be GTG.

LukaJCB
LukaJCB previously approved these changes Nov 27, 2017
@LukaJCB
Copy link
Member

LukaJCB commented Nov 27, 2017

I was sort of negative on this being in cats-core before, but I think it makes a lot of sense especially how we formulated it. It really makes the relationship between Semigroups/Monoids and Semigroupals/Monoidals/Applicatives much clearer for me. So kudos to all! 🎉

@stephen-lazaro
Copy link
Contributor Author

Awesome! Thanks for the help all involved. Was fun to try my hand at something like this.

def liftContravariant[A, B](f: A => B): F[B] => F[A] =
ContravariantMonoidal.contramap2(unit[B], _: F[B])(((b: B) => (b, b)) compose f)(self, self)

// Technically, this is not correct, as the Applicative is composed with the ContravariantMonoidal, not the other way around
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind elaborate a bit more (or point to a more detailed discussion/explanation) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can also just change the name and move this possibly. The point here is mostly that I didn't want to add anything to Applicative about ContravariantMonoidal since this might live in cats.more, so I ended up having this composition live here in ContravariantMonoidal even though the composition is Applicative compose ContravariantMonoidal, so the name and the signature are a little out of sync with what I saw in the rest of the repository.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I propose we rename and move it to Applicative

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, will do. Thanks.

@@ -165,6 +165,9 @@ private[data] sealed abstract class KleisliInstances1 extends KleisliInstances2
private[data] sealed abstract class KleisliInstances2 extends KleisliInstances3 {
implicit def catsDataAlternativeForKleisli[F[_], A](implicit F0: Alternative[F]): Alternative[Kleisli[F, A, ?]] =
new KleisliAlternative[F, A] { def F: Alternative[F] = F0 }

implicit def catsDataContravariantMonoidalForKleisli[F[_], A](implicit F0: ContravariantMonoidal[F]): ContravariantMonoidal[Kleisli[F, A, ?]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention in cats is that instances that are more specific should be at higher priority. I.E. instance of ContravariantMonoidal should be lower in the inheritance chain than Contravariant and ContravriantSemigroupal and Semigroupal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, will fix.

LukaJCB
LukaJCB previously approved these changes Nov 29, 2017
@kailuowang
Copy link
Contributor

kailuowang commented Nov 29, 2017

The code looks good for me. But I don't know the area well enough to be comfortable for my sign-off to be the final one for merge. @iravid @julienrf @adelbertc would anyone of you guys give a quick review? (You guys can focus just on the API the laws)

(F.unit[B], fa).contramapN(f) <-> fa.contramap(f andThen (_._2))

def contravariantMonoidalContramap2DiagonalAssociates[A](m: F[A], n: F[A], o: F[A]): IsEq[F[A]] =
((m, n).contramapN(delta[A]), o).contramapN(delta[A]) <-> (m, (n, o).contramapN(delta[A])).contramapN(delta[A])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can’t we move this law to ContravariantSemigroupal? It seems that we don’t need unit here.

Copy link
Contributor Author

@stephen-lazaro stephen-lazaro Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we probably could. It'll just involve adding a ContravariantSemigroupal laws trait and so forth. Looks like I overlooked your prior comment, sorry about that. I can move that over.

Copy link
Contributor

@iravid iravid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great stuff. Thanks for implementing this @stephen-lazaro.

I left some comments inline; they're mostly around efficiency and consistency.

*/
def unit[A]: F[A]

def liftContravariant[A, B](f: A => B): F[B] => F[A] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can actually be moved to Contravariant, no? (as it's just a curried contramap)


def product[A, B](fa: Eq[A], fb: Eq[B]): Eq[(A, B)] =
Eq.instance { (left, right) => fa.eqv(left._1, right._1) && fb.eqv(left._2, right._2) }
Eq.instance { (l, r) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, the previous version allocates at least one tuple less (although it is admittedly uglier :-))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's true, I sort of forgot that aspect of things. 👍

* Note: resulting instances are law-abiding only when the functions used are injective (represent a one-to-one mapping)
*/
def contramap[A, B](fa: Eq[A])(f: B => A): Eq[B] =
Eq.instance { (l: B, r: B) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to delegate to Eq.by, just for consistency?

}

def product[A, B](fa: Equiv[A], fb: Equiv[B]): Equiv[(A, B)] =
new Equiv[(A, B)] {
def equiv(x: (A, B), y: (A, B)): Boolean =
fa.equiv(x._1, y._1) && fb.equiv(x._2, y._2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment re allocations

val z = fa.compare(x._1, y._1)
if (z == 0) fb.compare(x._2, y._2) else z
}
def compare(x: (A, B), y: (A, B)): Int =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment re allocations

/** Derive an `Order` for `B` given an `Order[A]` and a function `B => A`.
*
* Note: resulting instances are law-abiding only when the functions used are injective (represent a one-to-one mapping)
*/
def contramap[A, B](fa: Order[A])(f: B => A): Order[B] = Order.by[B, A](f)(fa)
def contramap[A, B](fa: Order[A])(f: B => A): Order[B] =
new Order[B] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same re delegation to Order.by

}

def contramap[A, B](fa: Ordering[A])(f: B => A): Ordering[B] =
new Ordering[B] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same re delegation to on

val z = fa.compare(x._1, y._1)
if (z == 0) fb.compare(x._2, y._2) else z
def compare(x: (A, B), y: (A, B)): Int = (x, y) match {
case ((aL, bL), (aR, bR)) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same re allocations

@stephen-lazaro
Copy link
Contributor Author

Thanks @iravid! I'll take care of those tonight.

@kailuowang kailuowang merged commit e431b72 into typelevel:master Dec 1, 2017
@kailuowang
Copy link
Contributor

Thanks so much @stephen-lazaro for this awesome contribution!

@stephen-lazaro
Copy link
Contributor Author

🎉 Thanks everyone for the assistance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants