Add Invariant instances for kernel type classes #1914

LukaJCB · 2017-09-15T09:02:42Z

Adds InvariantMonoidal to most of the kernel type classes as discussed in #1904.

kailuowang · 2017-09-15T13:54:52Z

close and reopen to trigger build again (somehow failed on codecov)

codecov-io · 2017-09-15T13:54:56Z

Codecov Report

Merging #1914 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1914      +/-   ##
==========================================
+ Coverage   96.08%   96.21%   +0.13%     
==========================================
  Files         273      272       -1     
  Lines        4541     4627      +86     
  Branches      119      124       +5     
==========================================
+ Hits         4363     4452      +89     
+ Misses        178      175       -3

Impacted Files	Coverage Δ
core/src/main/scala/cats/instances/invariant.scala	`100% <100%> (ø)`
laws/src/main/scala/cats/laws/discipline/Eq.scala	`94.23% <100%> (+3.32%)`	⬆️
core/src/main/scala/cats/Invariant.scala	`95.23% <100%> (+23.8%)`	⬆️
core/src/main/scala/cats/Semigroupal.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <0%> (ø)`	⬆️

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 167d5b4...64cd30d. Read the comment docs.

LukaJCB · 2017-09-15T14:08:24Z

Should mention that this is an API breaking change, as imports from cats.instances.monoid and cats.instances.semigroup no longer work.

kailuowang · 2017-09-15T14:21:31Z

I probably missed something but I had the impression that we want to include all these instances into the companion objects of Invariant ~~and InvariantMonoidal~~, so that no imports is needed at all. Is that not possible or having significant cons?

LukaJCB · 2017-09-15T14:23:07Z

Oh, I may have misunderstood then. The only con, I can think about is that these won't get imported when importing all instances which might be confusing to some?

kailuowang · 2017-09-15T17:18:51Z

If you put them in the companion, they never need to be imported right?

LukaJCB · 2017-09-15T17:37:44Z

They don't? 🤔 Maybe I'm missing something here, but what would happen if I want to do something like:

val semiB: Semigroup[B] = Semigroup[A].imap(...)

I'd need to import the implicit instance from somewhere, right?
So it'd have be something along the lines of import cats.functor.Invariant._

kailuowang · 2017-09-15T19:27:37Z

@LukaJCB for your example, it should work without import if the Invariant instance for Semigroup is defined in Invariant's companion object. Actually what I said previously isn't accurate, we should put these Invariant and InvariantMonoidal instances into the companion object of Invariant

demo, the following code compiles

trait TCParent[F[_]]

trait TCChild[F[_]] extends TCParent[F]

trait A[T]

object TCParent {
  implicit val tcA: TCChild[A] = null
}

object Test {

  implicit class TCParentOps[F[_], T](f: F[T])(implicit tcF: TCParent[F]) {
    def e1: String = "bah"
  }

  implicit class TCOps[F[_], T](f: F[T])(implicit tcF: TCChild[F]) {
    def e: String = "bah"
  }

  val a: A[String] = ???
  a.e
  a.e1
}

LukaJCB · 2017-09-15T19:44:36Z

Ah okay, I think I've found the issue, we currently have tests, that check whether Invariant, Cartesian and InvariantMonoidal instances can be summoned and if we put it in object Invariant it can't find those Cartesian instances. However, if we put them in object InvariantMonoidal it can't find Invariant or Cartesian instances.
I guess the Cartesian instances aren't that important, but I'm not sure, WDYT?

kailuowang · 2017-09-15T20:09:31Z

ah didn't realize about the Cartesian, Crap, we can't put them in both, we have to put them an invariant object for people to import then. @johnynek WDYT?

johnynek · 2017-09-19T16:33:17Z

we could have a fallback possibly that looks in the other places.

So if A extends B we could have a low priority implicit that looks in A to provide an implicit of B. That said, that makes everything have a circular dependency.

Not really sure that's a great idea.

johnynek · 2017-09-19T16:35:50Z

core/src/main/scala/cats/instances/invariant.scala

+  implicit val catsInvariantMonoidalMonoid: InvariantMonoidal[Monoid] = new InvariantMonoidal[Monoid] {
+    def product[A, B](fa: Monoid[A], fb: Monoid[B]): Monoid[(A, B)] = new Monoid[(A, B)] {
+      val empty = fa.empty -> fb.empty
+      def combine(x: (A, B), y: (A, B)): (A, B) = fa.combine(x._1, y._1) -> fb.combine(x._2, y._2)


shame this destroys the combineAllOption optimization. Maybe we can add a ticket to address this after we merge.

In algebird, we buffer 1000 items internally, then pass over the block with each of the underlying monoids.

johnynek · 2017-09-19T16:37:58Z

core/src/main/scala/cats/instances/invariant.scala

+    def pure[A](a: A): CommutativeMonoid[A] = new CommutativeMonoid[A] {
+      val empty = a
+      def combine(x: A, y: A): A = a
+      override def combineAll(as: TraversableOnce[A]): A = a


can we only override combineAllOption? If we override only combineAll, then calls to combineAllOption still get the slow path. combineAll calls back to combineAllOption (combineAll should probably be final, actually).

I did override in all of the instances, but can you comment on how this is useful? I really can't come up with a way 🤔 and therefore can't find a good way to test these

johnynek · 2017-09-19T16:41:35Z

core/src/main/scala/cats/instances/invariant.scala

+import cats.kernel._
+import cats.InvariantMonoidal
+
+trait InvariantInstances {


looks like these are all InvariantMonoidal not just Invariant. Should we put them in a different trait name?

LukaJCB · 2017-09-20T07:40:59Z

I've added an override for combineAllOption to all the instances :)

Edit: seems like the overriden combineAllOption are untested though

kailuowang · 2017-09-25T18:22:34Z

tests/src/test/scala/cats/tests/AlgebraInvariantTests.scala

+  implicit val arbCommutativeGroupInt: Arbitrary[CommutativeGroup[Int]] =
+    Arbitrary(genCommutativeGroupInt)
+
+  test("Semigroup combineAllOption after imap with id is consistent"){


I propose we add this to the Semigroup law as a "Ref" law like these
https://github.com/typelevel/cats/blob/master/laws/src/main/scala/cats/laws/FoldableLaws.scala#L122-L143

Ah, I didn't know we had such a convention, good to know :)

We dont' really, but I'd like to establish one, see discussion here. @peterneyens has some interesting ideas too.

kailuowang · 2017-09-27T14:50:10Z

tests/src/test/scala/cats/tests/AlgebraInvariantTests.scala

+  implicit val arbCommutativeGroupInt: Arbitrary[CommutativeGroup[Int]] =
+    Arbitrary(genCommutativeGroupInt)
+
+  def combineAllOption_Ref[F[_] : InvariantMonoidal, A: Eq](name: String, a: A)(implicit ev: F[A] <:< Semigroup[A]): Unit = {


Hmm, I was suggesting adding this to the SemigroupLaw but I just realized that the consolidation PR #1922 isn't merged yet. Let's wait until that one gets merged, shall we?

I agree we should wait, however, this one isn't really about combineAllOption for any given semigroup, but for the ones created by InvariantMonoidal[F].pure. SemigroupLaws already has a check for combineAllOption here :)

ah I see. so, even without this methods and the tests that use it, the combineAllOption should be covered once you used GroupLaws to test the new instances? So for these pure given instance, why not test them with the GroupLaws again?

I think we could do that, but I didn't think the pure ones were actually lawful instances.

I'd be curious to find out.

Okay so the Semigroup instance is not lawful, because of the combineAllOption, as defined here, is not consistent with reduceOption(_ |+| _), because when you pass a collection with only one value reduceOption will only turn that into an Option. One could argue that this doesn't necessarily break the Semigroup laws though.

In addition the Monoid instance fails to fulfil the left and right identity laws.
Also these instances aren't able to abide by the idempotency law.

Does this mean Monoid is not a valid InvariantMonoidal? I'm not sure I know enough about that.

if the pure instance is not lawful, I am not sure if it's still a InvariantMonoidal, maybe just a Invariant? maybe worthwhile to find out how it's defined in Haskell.

I was surprised at the pure. I thought these two were only Invariant.

By the way, I think Invariant and Cartesian are the two ways you actually want to compose these things.

So, I think we should get rid of all Invariant Monoidal except for Semigroup and Commutative Semigroup and Turn the others into Invariant instances

👍 BTW, for Invariant instances, they can be moved the companion object of Invariant right?

LukaJCB · 2017-09-28T14:27:45Z

To summarize the breaking changes introduced in this PR for later use:

The InvariantMonoidal instance from cats.instances.semigroup was moved to cats.instances.invariant
The InvariantMonoidal instance from cats.instances.monoid was removed as it was not lawful. In its place there's an Invariant instance, located in object Invariant.

kailuowang · 2017-10-02T21:03:50Z

do you mind also update the doc of InvariantMonoidal https://github.com/typelevel/cats/blob/master/docs/src/main/tut/typeclasses/invariantmonoidal.md#L20
Line 20 mentioned Monoid as an InvariantMonoidal

…Ref test

…ly invariant instanes

kailuowang · 2017-10-09T14:25:38Z

@johnynek got any more feedback for this PR?

…cats into add-invariant-instances

johnynek · 2017-10-17T00:46:06Z

tests/src/test/scala/cats/tests/MonoidTests.scala

@@ -6,8 +6,6 @@ package tests
 class MonoidTests extends CatsSuite {
  {
    Invariant[Monoid]
-    Semigroupal[Monoid]


why did we lose this? I don't see it?

Good catch, I falsely assumed, that because Monoid wasn't a valid InvariantMonoidal, that it also wasn't a lawful Semigroupal. Turns out, it is, and I added it back! Thanks! :)

I missed that too. yeah, what broke the law was the pure method added in InvariantMonoidal

johnynek · 2017-10-17T22:05:29Z

👍

johnynek · 2017-10-18T02:36:19Z

@LukaJCB Thank you so much for carrying this over the line!

LukaJCB force-pushed the add-invariant-instances branch from 5ab0e76 to ac4c94f Compare September 15, 2017 09:57

kailuowang closed this Sep 15, 2017

kailuowang reopened this Sep 15, 2017

kailuowang added the in progress label Sep 15, 2017

LukaJCB force-pushed the add-invariant-instances branch from ac4c94f to f3752f1 Compare September 15, 2017 14:06

LukaJCB force-pushed the add-invariant-instances branch from 2ab053d to f3752f1 Compare September 19, 2017 15:01

kailuowang modified the milestone: 1.0.0-RC1 Sep 19, 2017

johnynek reviewed Sep 19, 2017

View reviewed changes

LukaJCB force-pushed the add-invariant-instances branch from 66f8943 to a85ce5c Compare September 22, 2017 15:50

kailuowang reviewed Sep 25, 2017

View reviewed changes

kailuowang reviewed Sep 27, 2017

View reviewed changes

LukaJCB force-pushed the add-invariant-instances branch from 5f9b12a to 29e5322 Compare September 28, 2017 07:25

LukaJCB added the Source Breaking label Sep 28, 2017

kailuowang previously approved these changes Sep 28, 2017

View reviewed changes

LukaJCB removed the in progress label Sep 30, 2017

Luka Jacobowitz added 3 commits October 3, 2017 12:08

Test combineAllOption for consistency

70b8454

Check combineAllOption for imap using GroupLaws and check pure using …

0541bff

…Ref test

Remove invariantMonoidal instances for most kernel classes and add on…

6ac8aaf

…ly invariant instanes

LukaJCB dismissed kailuowang’s stale review via 6ac8aaf October 3, 2017 10:12

LukaJCB force-pushed the add-invariant-instances branch from 29e5322 to 6ac8aaf Compare October 3, 2017 10:12

Remove mention that Monoid is a valid InvariantMonoidal

e0ebd6a

kailuowang previously approved these changes Oct 3, 2017

View reviewed changes

Merge branch 'master' into add-invariant-instances

f1b34cf

LukaJCB dismissed kailuowang’s stale review via f1b34cf October 4, 2017 08:14

kailuowang previously approved these changes Oct 4, 2017

View reviewed changes

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

Luka Jacobowitz added 2 commits October 13, 2017 20:58

Merge branch 'master' into add-invariant-instances

2e6f74a

Merge branch 'add-invariant-instances' of https://github.com/LukaJCB/…

53c9fb0

…cats into add-invariant-instances

LukaJCB dismissed kailuowang’s stale review via 53c9fb0 October 14, 2017 01:00

Luka Jacobowitz added 2 commits October 14, 2017 00:35

Fix with new kernel law testing

599f22a

Merge branch 'master' into add-invariant-instances

60aa276

LukaJCB force-pushed the add-invariant-instances branch from 070f620 to 60aa276 Compare October 16, 2017 15:10

kailuowang previously approved these changes Oct 16, 2017

View reviewed changes

kailuowang mentioned this pull request Oct 16, 2017

Getting ready for RC1 #1974

Merged

12 tasks

johnynek reviewed Oct 17, 2017

View reviewed changes

Add Semigroupal instance for Monoid

64cd30d

LukaJCB dismissed kailuowang’s stale review via 64cd30d October 17, 2017 21:20

kailuowang approved these changes Oct 17, 2017

View reviewed changes

LukaJCB merged commit 8d7b394 into typelevel:master Oct 17, 2017

LukaJCB deleted the add-invariant-instances branch October 17, 2017 23:57

LukaJCB mentioned this pull request Oct 18, 2017

many kernel algebraic typeclasses should be Invariant #1904

Closed

kailuowang added the enhancement label Dec 14, 2017

Add Invariant instances for kernel type classes #1914

Add Invariant instances for kernel type classes #1914

Conversation

LukaJCB commented Sep 15, 2017

kailuowang commented Sep 15, 2017

codecov-io commented Sep 15, 2017 • edited Loading

Codecov Report

LukaJCB commented Sep 15, 2017

kailuowang commented Sep 15, 2017 • edited Loading

LukaJCB commented Sep 15, 2017

kailuowang commented Sep 15, 2017

LukaJCB commented Sep 15, 2017 • edited Loading

kailuowang commented Sep 15, 2017 • edited Loading

LukaJCB commented Sep 15, 2017

kailuowang commented Sep 15, 2017 • edited Loading

johnynek commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Sep 20, 2017 • edited Loading

kailuowang Sep 25, 2017 • edited Loading

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

LukaJCB Sep 27, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

LukaJCB commented Sep 28, 2017

kailuowang commented Oct 2, 2017

kailuowang commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Oct 17, 2017

johnynek commented Oct 18, 2017

codecov-io commented Sep 15, 2017 •

edited

Loading

kailuowang commented Sep 15, 2017 •

edited

Loading

LukaJCB commented Sep 15, 2017 •

edited

Loading

kailuowang commented Sep 15, 2017 •

edited

Loading

kailuowang commented Sep 15, 2017 •

edited

Loading

LukaJCB Sep 22, 2017 •

edited

Loading

LukaJCB commented Sep 20, 2017 •

edited

Loading

kailuowang Sep 25, 2017 •

edited

Loading

LukaJCB Sep 27, 2017 •

edited

Loading

kailuowang Sep 27, 2017 •

edited

Loading