UnorderedTraverse should extend Functor
Nobody has claimed this yet.
- Dominant language
- Scala
- Stars
- 5.5k
- Forks
- 1.2k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 5
Description
I had assumed that UnorderedTraverse extended Functor since it can clearly be implemented:
override def map[A, B](fa: F[A])(f: A => B): F[B] =
unorderedTraverse[Id, A, B](fa)(f)
But I was surprised to learn (or recall) it didn't. I looked up the original PR to see if I could see why. I found: https://github.com/typelevel/cats/pull/1981 (cc @LukaJCB ).
It seems it was briefly considered in that PR, but then discarded because Set[A] could not implement UnorderedTraverse then. I now have a different take: it's code smell that the laws on UnorderedTraverse as so weak that we could implement map in the above, but it is totally unspecified what the result might be. I suppose returning an empty F[B] on every unorderedTraverse and nothing else might actually be lawful.
For instance, we have this code in UnorderedTraverseLaws:
https://github.com/typelevel/cats/blob/e94ddf038c741a93c01e79fef21737afb5cd9699/laws/src/main/scala/cats/laws/UnorderedTraverseLaws.scala#L30
def unorderedTraverseIdentity[A, B](fa: F[A])(f: A => B)(implicit ev: Functor[F]): IsEq[F[B]] =
F.unorderedTraverse[Id, A, B](fa)(f) <-> (ev.map(fa)(f))
But that code is never called in the repo. Moreover, there are only two implementations of UnorderedTraverse in the repo: Set and [V] =>> Map[K, V]. Cats collections implements UnorderedTraverse for the parametric HashMap[K, V] there, which was where I was using this.
I think we should have a stern talk with whoever approved these changes (😅 )
https://github.com/typelevel/cats/pull/1981#pullrequestreview-78965435
We are kind of in a jam here. In my opinion, the correct fix is that UnorderedTraverse should extends Functor. We can implement map as described above and not break anyone. However, that would give Set a Functor which would be unlawful, and Functor is a much more commonly used typeclass than UnorderedTraverse (which with this new change really only has one instance that we know about: hashmaps). Moreover, the UnorderedTraverseLaws don't have the correct typeclasses to actually call the FunctorLaws, and due to binary compatibility requirements, we can't add them.
I see two paths forward here:
-
admit we messed up and just consider UnorderedTraverse somewhat lawless: if
unorderedTraverse(f)(identity)doesn't even have to bef, which our laws don't require, I don't know how you can really program against this API. We can't add this law currently because the UnorderedTraverseLaws don't accept implicits to check equality onF[A](we don't haveEq[F[A]]We could do some hack, like check that law inside of aX.pure(...)because we haveEq[X[F[A]]but that seems ugly. -
we could make UnorderedTraverse extends Functor, which would make our current Set implementation lawless. We could fix this by removing the
implicitfrom the current instance, mark it deprecated and make a new implicit that is lowered toUnorderedFoldablewhich is IMO the correct thing for Set. Note thatSetis already suspect for UnorderedTraverse because to go fromSet[A]toSet[B]you are relying on universal hashing onB. If we had a proper parametricHashSet, we would require not onlyA => F[B]butHash[B]to create theSet[B], which makes it clear that this could not be a validUnorderedTraverse. I think the code smell here is pretty strong that we made the wrong choice originally and this would be my preferred path forward.
If we can get some agreement, I will prepare the PR to do step 2.
cc @satorg @armanbilge @LukaJCB
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with UnorderedTraverseLaws.scala and the Set and [V] =>> Map[K, V] instances mentioned in the issue, then review PR 1981 and its discussion. Determine whether the proposed Functor extension is compatible with the existing laws and binary-compatibility constraints. Done means the maintainers have agreed on a path and its affected instances and laws are covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100