typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.26k stars 1.21k forks source link

Deprecate nonInheritedOps and ops objects? #3330

Open travisbrown opened 4 years ago

travisbrown commented 4 years ago

Update: now that we've switched to the Scalafix-powered version of Simulacrum (see #3424), we can easily deprecate these definitions, and I personally think we should do this in Cats 2.2.0. If anyone has objections please raise them here.


Cats currently has three ways of importing syntax methods for particular type classes. You can use the subpackage in syntax (this is the standard approach to à la carte imports):

scala> import cats.syntax.functor._
import cats.syntax.functor._

scala> cats.Eval.now(List(0)).widen[Seq[Int]]
res1: cats.Eval[Seq[Int]] = Now(List(0))

You could also use the nonInheritedOps object in the type class companion, which does exactly the same thing:

scala> import cats.Functor.nonInheritedOps._
import cats.Functor.nonInheritedOps._

scala> cats.Eval.now(List(0)).widen[Seq[Int]]
res0: cats.Eval[Seq[Int]] = Now(List(0))

Or you can use the ops object in the companion of any descendant type class:

scala> import cats.Monad.ops._
import cats.Monad.ops._

scala> cats.Eval.now(List(0)).widen[Seq[Int]]
res0: cats.Eval[Seq[Int]] = Now(List(0))

While this last is at least different from the standard cats.syntax.abc._ approach, and maybe potentially useful, I've never actually seen anyone use it, or recommend it, and it has a serious downside—if you import ops._ from multiple type class companions, you lose the syntax methods for the intersection of their ancestors:

scala> import cats.instances.all._, cats.Traverse.ops._, cats.Alternative.ops._
import cats.instances.all._
import cats.Traverse.ops._
import cats.Alternative.ops._

scala> List(1) <+> List(2) <+> List(3)
res0: List[Int] = List(1, 2, 3)

scala> List(Some(1), None, Some(2)).sequence
res1: Option[List[Int]] = None

scala> List(1, 2, 3).fmap(_ + 1)
           ^
       error: type mismatch;
        found   : List[Int]
        required: ?{def fmap: ?}
       Note that implicit conversions are not applicable because they are ambiguous:
        both method toAllTraverseOps in object ops of type [F[_], C](target: F[C])(implicit tc: cats.Traverse[F])cats.Traverse.AllOps[F,C]{type TypeClassType = cats.Traverse[F]}
        and method toAllAlternativeOps in object ops of type [F[_], C](target: F[C])(implicit tc: cats.Alternative[F])cats.Alternative.AllOps[F,C]{type TypeClassType = cats.Alternative[F]}
        are possible conversion functions from List[Int] to ?{def fmap: ?}

Or even worse:

scala> List(1, 2, 3).as(0)
                     ^
       error: value as is not a member of List[Int]
       did you mean last, map, or max?

…which is probably why nobody uses or recommends these.

So nonInheritedOps is redundant, and ops makes it really easy to shoot yourself in the foot. I think we should get rid of both of them in Cats 3, and deprecate them now.

These objects are generated by Simulacrum, and I'm not entirely sure it's possible to output deprecated methods using Scala 2's macro annotations (?), but if we move to the Scalafix approach this would be pretty easy.

rossabaker commented 4 years ago

I generally disrecommend the standard à la carte imports, so I'm a strong :+1: to not having three ways to do them.

mpilquist commented 4 years ago

:+1: Let's deprecate. The ops import is from the original Simulacrum encoding and the nonInheritedOps was to avoid the loss of syntax from common ancestors when importing multiple. They both predate mixing all syntax enrichments in to a single object (or rather, I was experimenting with an encoding that wouldn't require a Scalaz._ import).

travisbrown commented 4 years ago

Thanks, @mpilquist—I didn't really have any idea what the original context was, but that makes sense.