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

Define laws for Traversable #373

Open
sideeffffect opened this issue Nov 14, 2020 · 15 comments · May be fixed by #449 or #679
Open

Define laws for Traversable #373

sideeffffect opened this issue Nov 14, 2020 · 15 comments · May be fixed by #449 or #679

Comments

@sideeffffect
Copy link
Member

sideeffffect commented Nov 14, 2020

Difficulty: Advanced, requires familiarity with the concept of algebraic laws and testing them

Traversable currently doesn't have any laws (only inherits those from Covariant). Figure out which laws should ZIO Prelude's Traversable abide. Then write those tests down in Traversable.laws. Add tests for the missing instances in TraversableSpec (tests for some basic instances, like Option or List are already present).

As an inspiration, have a look Cats or ScalaZ or Haskell Travers/able and their laws:

https://github.com/typelevel/cats/blob/master/laws/src/main/scala/cats/laws/TraverseLaws.scala#L9
https://github.com/scalaz/scalaz/blob/master/core/src/main/scala/scalaz/Traverse.scala#L162
https://hackage.haskell.org/package/base-4.14.0.0/docs/Data-Traversable.html#t:Traversable

@zalbia
Copy link
Contributor

zalbia commented Nov 21, 2020

I'd like to take this.

@zalbia
Copy link
Contributor

zalbia commented Nov 22, 2020

I'm hitting a roadblock writing the naturality law for Traversable. Looking at the implementation of the laws in Cats & ScalaZ, it looks like we need to have an implementation of natural transformation. Do we need to implement it in zio-prelude to write this law or am I missing something?

@sideeffffect
Copy link
Member Author

I'm no category theorist, so please excuse me if I say something nonsensical.

Natural transformations are morphisms between functors, right? In that case we don't have them in ZIO Prelude (yet 😉).

But let me ask "from the other side". Are we able to write some interesting laws/properties for Traversable with the current state of things? If yes, could you please open a PR with that? We'll figure something out about naturality in the meantime 👍

Badmoonz added a commit to Badmoonz/zio-prelude that referenced this issue Nov 29, 2020
@Badmoonz Badmoonz linked a pull request Nov 29, 2020 that will close this issue
@Badmoonz
Copy link

Badmoonz commented Nov 29, 2020

@zach-albia I think we don't need so Generic approach for generating Applicatives used by foreach as you do
testing for several hardcoded instances is sufficient, even using only Id Applicative is enough imho

Badmoonz added a commit to Badmoonz/zio-prelude that referenced this issue Nov 29, 2020
@zalbia
Copy link
Contributor

zalbia commented Nov 30, 2020

@zach-albia I think we don't need so Generic approach for generating Applicatives used by foreach as you do
testing for several hardcoded instances is sufficient, even using only Id Applicative is enough imho

@Badmoonz your commit was super helpful in making the PR run in Dotty. Thanks!

@Badmoonz
Copy link

Badmoonz commented Nov 30, 2020

@zach-albia

I think it will be nice to add some tests to check that laws are really working:

        {
          implicit val badTraversable = new Traversable[Option] {
            // breaks Traversable Identity law
            override def foreach[G[+_]: IdentityBoth: Covariant, A, B](fa: Option[A])(f: A => G[B]): G[Option[B]] =
              (None: Option[B]).succeed
            override def map[A, B](f: A => B): Option[A] => Option[B] = _.map(f)
          }

          testM("Traversable.Identity law violation captured")(checkAllLaws(Traversable)(GenF.option, Gen.anyInt).map(x => assert(x.isFailure)(Assertion.isTrue)))
        },
        {
          implicit val badTraversable = new Traversable[Option] {
            override def foreach[G[+_]: IdentityBoth: Covariant, A, B](fa: Option[A])(f: A => G[B]): G[Option[B]] = fa match {
              case Some(x) => f(x).map(Some(_): Option[B])
              case None    => (None: Option[B]).succeed
            }

            // breaks Covariant Identity law
            override def map[A, B](f: A => B): Option[A] => Option[B] = _ => None

          }
          testM("Covariant.Identity law violation captured")(checkAllLaws(Traversable)(GenF.option, Gen.anyInt).map(x => assert(x.isFailure)(Assertion.isTrue)))
        }

still cannot imagine such traversable instance that has valid Identity laws and broken Composition laws 🤔

@sideeffffect
Copy link
Member Author

So we now have 2 PRs implementing this
#442 has

  • identity
  • sequentialFusion
  • purity
  • naturality
  • parallelFusion

#449 has

  • identity
  • composition (for some predefined functors)

So how do we do this guys? Do you @zach-albia and @Badmoonz coordinate with each other to create one PR? Or should we merge first one and then the other? In what order?
/cc @adamgfraser

@zalbia
Copy link
Contributor

zalbia commented Dec 5, 2020

@sideeffffect I integrated a lot of the code from #449 into #442. I think you can merge his first once #449 runs in 2.11, and then you can take a look at mine? I think mine needs some more review TBH.

@Badmoonz
Copy link

Badmoonz commented Dec 5, 2020

@sideeffffect

I don't want to reject all the job @zach-albia has done introducing new laws

But if we want to keep things simple and valuable, really enough set of traversable laws would be

  1. check that map and foreach commute (traversable instance may override map for perfomance issues)

forall ta, f : ta.map(f) = Id.unwrap(ta.foreach[Id, A, B]((a: A) => Id(f(a)))

  1. Check already defined Covariant.laws

That's all we need!!! It's ever lesser law set than #449

sequentialFusion,purity,naturality,parallelFusion laws within Identity Applicative can be proved by Covariant laws ,
so they probably don't validate any bad case

I think we should introduce new laws in terms of existing such instances that work incorrectly but pass current laws

@sideeffffect
Copy link
Member Author

If what @Badmoonz says is correct, than let's just have the one commutativity law.
If all the other laws follow from that (+ the Covariant laws), then we don't need them and we should strive for minimalism in this regard IMHO.
What do you guys think? /cc @adamgfraser @jdegoes

@adamgfraser
Copy link
Contributor

Yes I think we don't need five laws. The formulation I have seen normally has two, an identity law that calling foreach with the identity function returns the value unchanged, which is basically consistency with map, and then a second that two traversals can be fused, though I am sure there are other minimal formulations of laws.

@sideeffffect
Copy link
Member Author

For the record, here are papers discussing laws for Traversable:

Would somebody like to go forward and implement the identity law

forall ta, f : ta.map(f) = Id.unwrap(ta.foreach[Id, A, B]((a: A) => Id(f(a)))

?

@sideeffffect
Copy link
Member Author

There have been some changes in the codebase, like renaming Traversable to ForEach and others, so the PR(s) might be outdated. But our need for laws for ForEach (aka Traversable) hasn't disappeared 😉
Would some of you guys @Badmoonz or @zalbia or anybody else like to pick up the work on adding laws for this type class?
(And there's also NonEmptyForEach, what laws should that get?)

@zalbia
Copy link
Contributor

zalbia commented Feb 12, 2021

@sideeffffect I would love to have a go at it again as soon as I get the chance (which should be in a week or two). Will maintain @Badmoonz' insight on the minimum number of laws that need to be implemented.

@zalbia
Copy link
Contributor

zalbia commented Feb 12, 2021

That said, if anyone reading this wants to take this instead before I get to chance to work on it, please go ahead.

@zalbia zalbia linked a pull request Aug 1, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants