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

Deprecate nonInheritedOps and ops objects? #3330

Open
travisbrown opened this issue Feb 28, 2020 · 3 comments
Open

Deprecate nonInheritedOps and ops objects? #3330

travisbrown opened this issue Feb 28, 2020 · 3 comments
Labels

Comments

@travisbrown
Copy link
Contributor

travisbrown commented Feb 28, 2020

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
Copy link
Member

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

@mpilquist
Copy link
Member

👍 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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants