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

Contravariant & Invariant #975

Merged
merged 21 commits into from
Sep 12, 2018
Merged

Contravariant & Invariant #975

merged 21 commits into from
Sep 12, 2018

Conversation

Cotel
Copy link
Member

@Cotel Cotel commented Aug 4, 2018

Adding Contravariant & Invariant typeclasses and instances.

@Cotel Cotel self-assigned this Aug 4, 2018
@Cotel Cotel requested a review from pakoito August 4, 2018 08:09

init {
testLaws(
InvariantLaws.laws(Monoid.invariant<Int>(), { Int.monoid() }, EQ)
Copy link
Member Author

Choose a reason for hiding this comment

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

@pakoito It seems like our experiment success at compiling but tests are failing 🤔

fun G(): Invariant<G>

override fun <A, B> Kind<Nested<F, G>, A>.imap(f: (A) -> B, g: (B) -> A): Kind<Nested<F, G>, B> =
F().run { unnest().imap({ ga -> G().run { ga.imap(f, g) } }, { gb -> G().run { gb.imap(g, f) } }) }.nest()
Copy link
Member

@pakoito pakoito Aug 4, 2018

Choose a reason for hiding this comment

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

There's a chance this doesn't work on Android on the D8 compiler due to a problem with scopes and run. Can you please move each of the lambdas in imap to a separate line?

F().run {
val fl: (Kind<G, A>) -> Kind<G, B> = { ga -> G().run { ga.contramap(g) } }
val fr: (Kind<G, B>) -> Kind<G, A> = { gb -> G().run { gb.contramap(f) } }
unnest().imap(fl, fr)
Copy link
Member

@pakoito pakoito Aug 4, 2018

Choose a reason for hiding this comment

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

Sorry, my bad. Take these out of the F() run block I mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌

@Cotel Cotel force-pushed the cotel-contravariance branch from 3d5b50e to e467b97 Compare August 4, 2018 11:42
@Cotel
Copy link
Member Author

Cotel commented Aug 4, 2018

I've run all the tests locally and it seems like Monoid is passing Invariant Laws now!

So now I'm going to add an Invariant instance for Semigroup and that should be it for Invariant.

EDIT: When I add the typealias SemigroupOf<A> = arrow.Kind<ForSemigroup, A> boilerplate, Monoid can no longer extend MonoidOf<A> because as it already extends Semigroup<A> there is an inconsistent type at F of Kind (ForSemigroup, ForMonoid).

Can we fix this? Or should we just offer one instance of Invariant for Monoid or Semigroup?

@@ -11,7 +11,7 @@ import org.junit.runner.RunWith
class MonoidTest : UnitSpec() {

val EQ: Eq<MonoidOf<Int>> = Eq.invoke { a, b ->
a.fix() == b.fix()
a.fix().run { 3.combine(1) } == b.fix().run { 3.combine(1) }
Copy link
Member

Choose a reason for hiding this comment

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

You figured out why it didn't work :D Well done!

@codecov
Copy link

codecov bot commented Aug 4, 2018

Codecov Report

Merging #975 into master will increase coverage by 0.15%.
The diff coverage is 51.3%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #975      +/-   ##
============================================
+ Coverage      47.8%   47.96%   +0.15%     
- Complexity      695      702       +7     
============================================
  Files           320      325       +5     
  Lines          8099     8200     +101     
  Branches        753      753              
============================================
+ Hits           3872     3933      +61     
- Misses         3943     3984      +41     
+ Partials        284      283       -1
Impacted Files Coverage Δ Complexity Δ
...ses/src/main/kotlin/arrow/typeclasses/Bifunctor.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...-data/src/main/kotlin/arrow/instances/coproduct.kt 46.66% <0%> (-3.34%) 0 <0> (ø)
...nces-core/src/main/kotlin/arrow/instances/const.kt 52.38% <0%> (-5.52%) 0 <0> (ø)
...arrow-data/src/main/kotlin/arrow/data/Coproduct.kt 57.14% <0%> (-2.86%) 7 <0> (ø)
...-data/src/main/kotlin/arrow/instances/cokleisli.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...asses/src/main/kotlin/arrow/typeclasses/Functor.kt 33.33% <100%> (+8.33%) 0 <0> (ø) ⬇️
...arrow-core/src/main/kotlin/arrow/core/Function1.kt 88.23% <100%> (+0.73%) 6 <1> (+1) ⬆️
...src/main/kotlin/arrow/typeclasses/Contravariant.kt 25% <25%> (ø) 0 <0> (?)
...es/src/main/kotlin/arrow/typeclasses/Cocomposed.kt 28.57% <28.57%> (ø) 0 <0> (?)
...sses/src/main/kotlin/arrow/typeclasses/Composed.kt 61.47% <45.28%> (-7.28%) 0 <0> (ø)
... and 17 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 25bd0cf...7a8474a. Read the comment docs.

@pakoito
Copy link
Member

pakoito commented Aug 4, 2018

Can we fix this?

Try making ForMonoid inherit ForSemigroup, as the F in Kind is covariant. Subtyping sucks :) Test thoroughly.

@Cotel
Copy link
Member Author

Cotel commented Aug 4, 2018

I'm also having some troubles to create a Function1ContravariantInstance. I've defined a contramap function in Function1:

class Function1<I, out O>(val f: (I) -> O) {
  fun <B> contramap(f: (B) -> I): Function1<B, O> = (this.f compose f).k()
}

But now I can't use it when I am defining the ContravariantInstance:

interface Function1ContravariantInstance<I> : Contravariant<Function1PartialOf<I>> {
  override fun <A, B> Kind<Function1PartialOf<I>, A>.contramap(f: (B) -> A): Function1<I, B> =
    fix().contramap(f)
}

The editor tells me that contramap will be called recursively. But even if I change it to contramapp to distinguish between scopes, the signature is incorrect. I would need it to be:

override fun <A, B> Kind<Function1PartialOf<I>, A>.contramap(f: (B) -> I): Function1<B, A>

@pakoito
Copy link
Member

pakoito commented Aug 4, 2018

@Cotel Push it and I'll take a look

@Cotel Cotel force-pushed the cotel-contravariance branch from 96612ab to 5c422d8 Compare August 4, 2018 16:33
@pakoito
Copy link
Member

pakoito commented Aug 4, 2018

@Cotel can't be done, same problem as the left functor from the other day


testLaws(
InvariantLaws.laws(ComposedInvariant(Option.functor(), NonEmptyList.functor()), cf, EQ_OPTION_NEL),
InvariantLaws.laws(ComposedInvariantCovariant(Option.functor(), NonEmptyList.functor()), cf, EQ_OPTION_NEL),
InvariantLaws.laws(ComposedInvariantContravariant(Option.functor(), Function1.contravariant<Int>()), cf2, EQ_OPTION_FN1),
Copy link
Member

Choose a reason for hiding this comment

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

TestLaws deduplicates tests based on name. That means only one of these will be run. Split 2 of them into a new call to testLaws.

@Cotel
Copy link
Member Author

Cotel commented Aug 6, 2018

I've been looking for Invariant and Contravariant instances in cats but, besides the typeclasses stuff, these are all the instances we can make.

The only thing that remains is to test the laws for ComposedContravariant, ComposedContravariantCovariant and ComposedCovariantContravariant but these are really unintuitive for me, I am not able to create the required lambda for the law or the Eq instance 😢

@Cotel
Copy link
Member Author

Cotel commented Aug 24, 2018

I would like to finish this asap and start something related with comonadic UIs but I was stuck and I'm quite busy right now. I feel bad for keeping this PR open for so long with no activity 😅

@pakoito
Copy link
Member

pakoito commented Aug 24, 2018

I'm traveling, let's talk in September :D

@Cotel Cotel force-pushed the cotel-contravariance branch from 941c721 to 3fa098c Compare September 3, 2018 18:55
@Cotel
Copy link
Member Author

Cotel commented Sep 8, 2018

Hey! I might have some spare time tomorrow and I would like to close this and start working on another issue or something 😁

@Cotel
Copy link
Member Author

Cotel commented Sep 9, 2018

I'm sorry but I really don't know what is failing here. I suppose that the laws which are failing are ComposedInvariantContravariant and the cause are probably the lambda and EQ instance for the test.

val cf2: (Int) -> Kind<Nested<ForOption, Conested<ForFunction1, Int>>, Int> = { x: Int ->
  Some({ y: Int -> x + y }.k().conest()).nest()                                          
}                                                                                        
val EQ_OPTION_FN1: Eq<NestedType<ForOption, Conested<ForFunction1, Int>, Int>> = Eq { a, b ->
  a.unnest().fix() == b.unnest().fix()                                                       
}                                                                                            

Any ideas? 😅

@pakoito
Copy link
Member

pakoito commented Sep 9, 2018

I may take a look directly in the afternoon. Meanwhile, use the extfun Eq.logged and run the tests with -i to at least see the values.

@Cotel
Copy link
Member Author

Cotel commented Sep 11, 2018

God bless that Eq.logged function 😍.

@pakoito
Copy link
Member

pakoito commented Sep 11, 2018

@Cotel It's saved my ass more times that I'm willing to admit hahaha

Gotta give a last check and we'll go green ;)

@@ -36,7 +36,15 @@ class ComposedInstancesTest : UnitSpec() {
}

val EQ_OPTION_FN1: Eq<NestedType<ForOption, Conested<ForFunction1, Int>, Int>> = Eq { a, b ->
a.unnest().fix() == b.unnest().fix()
a.unnest().fix().fold(
Copy link
Member

@pakoito pakoito Sep 11, 2018

Choose a reason for hiding this comment

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

Cheezus fucking chrixt

@@ -16,6 +13,7 @@ class ConstTest : UnitSpec() {
init {
ForConst(Int.monoid()) extensions {
testLaws(
InvariantLaws.laws(this, { Const(it) }, Eq.any()),
Copy link
Member

Choose a reason for hiding this comment

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

InvariantLaws should be part of FunctorLaws, so it's tested by ApplicativeLaws!


testLaws(
InvariantLaws.laws(ComposedInvariant(Option.functor(), NonEmptyList.functor()), cf, EQ_OPTION_NEL),
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, InvariantLaws should be part of FunctorLaws.

Copy link
Member Author

Choose a reason for hiding this comment

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

FuntorLaws are being used at line 74. Should I remove that line too?

object ContravariantLaws {

inline fun <F> laws(CF: Contravariant<F>, noinline cf: (Int) -> Kind<F, Int>, EQ: Eq<Kind<F, Int>>): List<Law> =
listOf(
Copy link
Member

@pakoito pakoito Sep 11, 2018

Choose a reason for hiding this comment

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

In here ContravariantLaws imply InvariantLaws also pass, so replace this line with:

InvariantLaws.laws(...) + listOf(

You'll need to do the same in FunctorLaws.

@@ -211,20 +348,20 @@ interface ComposedBifoldable<F, G> : Bifoldable<Nested<F, G>> {

fun G(): Bifoldable<G>

override fun <A, B, C> BiunnestedType<F, G, A, B>.bifoldLeft(c: C, f: (C, A) -> C, g: (C, B) -> C): C = F().run {
override fun <A, B, C> BinestedType<F, G, A, B>.bifoldLeft(c: C, f: (C, A) -> C, g: (C, B) -> C): C = F().run {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure of this change, or was it part of the merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked the history and this is part from our changes, related to fa50cf1

Copy link
Member

Choose a reason for hiding this comment

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

Right, right, they're correct, there's a couple of fixes to other that were wrong.

Cool, only the test left then.

Copy link
Member

@pakoito pakoito left a comment

Choose a reason for hiding this comment

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

One last fix: the FunctorLaws and ContravariantLaws require also passing the InvariantLaws. With that change we're ready to go!!

@pakoito
Copy link
Member

pakoito commented Sep 12, 2018

🎉

@pakoito pakoito merged commit 0a0bec2 into master Sep 12, 2018
@pakoito pakoito deleted the cotel-contravariance branch September 12, 2018 20:08
@raulraja raulraja mentioned this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants