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

Replace Xor with Either, fixes #1192 #1289

Merged
merged 8 commits into from
Aug 20, 2016

Conversation

adelbertc
Copy link
Contributor

No description provided.

This commit keeps Xor and XorT but moves most Cats functions from Xor to Either.
Xor will be removed after 0.8.0.

- Add methods from Xor onto enrichment class of Either
- Copy XorT into an EitherT based around Either
- Styling: Occurences of Either are styled as Either[A, B] instead of A Either B (as it was in Xor)
* `EitherT[F, A, B]` wraps a value of type `F[Either[A, B]]`. An `F[C]` can be lifted in to `EitherT[F, A, C]` via `EitherT.right`,
* and lifted in to a `EitherT[F, C, B]` via `EitherT.left`.
*/
final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this extends AnyVal?

Copy link
Contributor Author

@adelbertc adelbertc Aug 15, 2016

Choose a reason for hiding this comment

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

Hm I'm getting a lot of EitherT.scala:253 bridge generated for method compare: (x: cats.data.EitherT[F, L, A], y: cats.data.EitherT[F, L, A])Int in trait EitherTOrder which overrides method compare: (x: A, y: A)Int in trait Order clashes with definition of member itself; both have erased type (x: Object, y: Object)Int and similar for other methods (11 such occurences) - I suspect this is also why XorT is currently not a value class?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. Bummer.

- Remove variance workarounds since Either syntax is invariant
- Prefer pattern matching over fold for performance
- Fix ScalaDoc
- Replace ad-hoc casts with leftCast and rightCast
- Re-enable ScalaStyle after disables and disable ScalaStyle on Either.type enrichment
@codecov-io
Copy link

codecov-io commented Aug 15, 2016

Current coverage is 90.58% (diff: 93.42%)

Merging #1289 into master will decrease coverage by 0.17%

@@             master      #1289   diff @@
==========================================
  Files           234        235     +1   
  Lines          3377       3602   +225   
  Methods        3323       3544   +221   
  Messages          0          0          
  Branches         52         54     +2   
==========================================
+ Hits           3065       3263   +198   
- Misses          312        339    +27   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 1b65c5a...ba1a786

@milessabin
Copy link
Member

milessabin commented Aug 16, 2016

Can someone summarize for me what the 2.11/2.12 compatibility story is wrt right-biasedness?

Would there be any benefit to Cats in a backport of the 2.12 Either changes to Typelevel Scala for 2.11.8?

@adelbertc
Copy link
Contributor Author

/cc @soc

As I understand it the right bias change is only in 2.12.x+ currently. It would be useful for the right-bias to be backported to Scala 2.11.x, but 2.10.x would still be missing it and I think they EOL'd 2.10.x (I may be completely wrong about this).

Even once we start compiling against 2.12.x I don't think we need to change anything since calls to flatMap and map just won't trigger the enrichment.

@@ -302,10 +301,10 @@ private[cats] trait EvalInstances extends EvalInstances0 {
def flatMap[A, B](fa: Eval[A])(f: A => Eval[B]): Eval[B] = fa.flatMap(f)
def extract[A](la: Eval[A]): A = la.value
def coflatMap[A, B](fa: Eval[A])(f: Eval[A] => B): Eval[B] = Later(f(fa))
def tailRecM[A, B](a: A)(f: A => Eval[A Xor B]): Eval[B] =
def tailRecM[A, B](a: A)(f: A => Eval[Either[A, B]]): Eval[B] =
f(a).flatMap(_ match {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change this to defaultTailRecM(a)(f)?


implicit def catsDataSemigroupKForEither[L]: SemigroupK[Either[L, ?]] =
new SemigroupK[Either[L, ?]] {
def combineK[A](x: Either[L, A], y: Either[L, A]): Either[L, A] = x match {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about this. Is this how Xor worked?

The standard combine is to prefer Left, but this combineK is preferring Right in that case. Maybe this is what you expect with MonadCombine, but I'm a bit concerned there is no documentation of this, and it is a bit unclear why one works in one direction, but the other the opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, all of the new either syntax is copied over from the Xor data type. You can see the equivalent here: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/Xor.scala#L229

That change was made in this PR I believe: #996

final class RightOps[A, B](val right: Right[A, B]) extends AnyVal {
/** Cast the left type parameter of the `Right`. */
def leftCast[C]: Either[C, B] = right.asInstanceOf[Either[C, B]]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to name them like this to follow the leftMap convention - the name indicates which type parameter side to cast.

@milessabin
Copy link
Member

/cc @soc

In that case, would it make sense to split some of this out as an independent compatibility library which can be used by other projects which care about right bias? eg. fs2 ... paging @mpilquist ...

@mpilquist
Copy link
Member

In FS2, there are only 12 uses of right projections that we could drop with right-biasedness. I don't think I'd want to take on a dependency to get rid of those 12, though admittedly the deprecation of .right in 2.12 is going to irritate me quite a bit. My preference would be to pick up the right-biasedness in a 2.11.x but I think that's unlikely to happen.

@milessabin
Copy link
Member

@mpilquist If there's a need we can make it happen in Typelevel Scala for 2.11.8 ... the need for that is what I'm trying to gauge here.

@adelbertc
Copy link
Contributor Author

adelbertc commented Aug 17, 2016

There are also folks still on 2.10 like me and there's definitely no back
porting all the way back there. I doubt my colleagues would want to switch
to TLC and taking on a dependency just for syntax seems too much and I
could also see it giving a messier experience with Cats. My preference
would be to leave it so as not to special case Either which is one of the
motivations for this move.

EDIT Sorry misread. I would be OK with having like a cats-either and having cats-core depend on it, done in a way such that cats.syntax.either._ and/or cats.implicits._ still brought in the syntax, but I can see those who don't want to take on a Cats dependency also not wanting to take a dependency just got Either syntax. Depending on how feel people feel we can split it out, though I'd prefer it be in a separate PR as this PR is already quite large. We can create a ticket to track it in the interim.

@johnynek
Copy link
Contributor

👍

@adelbertc
Copy link
Contributor Author

Will wait for two more 👍 's before merging since this is a big change - thanks @johnynek !

@adelbertc
Copy link
Contributor Author

@johnynek So I can see how it would work with the cats.implicits._ import ( @rossabaker is working on this as I'm writing this comment) but I think it is impossible for a la carte imports without inserting EitherSyntax into the hierarchy of every (relevant) type class syntax class. Meaning the following would not work:

import cats.instances.either._
import cats.syntax.either._
import cats.syntax.functor._

@johnynek
Copy link
Contributor

yeah, I think if you import either and functor, too bad, so sad.

@adelbertc
Copy link
Contributor Author

adelbertc commented Aug 18, 2016

So this is what I've done:

--- a/core/src/main/scala/cats/implicits/package.scala
+++ b/core/src/main/scala/cats/implicits/package.scala
@@ -1,3 +1,15 @@
 package cats

-object implicits extends syntax.AllSyntax with instances.AllInstances
+import cats.syntax.{EitherOps, EitherObjectOps, LeftOps, RightOps}
+
+object implicits extends Implicits0 {
+  override implicit def catsSyntaxEither[A, B](eab: Either[A, B]): EitherOps[A, B] = super.catsSyntaxEither(eab)
+
+  override implicit def catsSyntaxEitherObject(either: Either.type): EitherObjectOps = super.catsSyntaxEitherObject(either) // scalastyle:off ensure.single.space.after.token
+
+  override implicit def catsSyntaxLeft[A, B](left: Left[A, B]): LeftOps[A, B] = super.catsSyntaxLeft(left)
+
+  override implicit def catsSyntaxRight[A, B](right: Right[A, B]): RightOps[A, B] = super.catsSyntaxRight(right)
+}
+
+trait Implicits0 extends syntax.AllSyntax with instances.AllInstances

And this works (it didn't prior to the above change):

import cats.implicits._

type Result[A] = Either[String, A]

val r: Result[Int] = Right(5)
r.map(_ + 1)

The following change doesn't work:

--- a/core/src/main/scala/cats/implicits/package.scala
+++ b/core/src/main/scala/cats/implicits/package.scala
@@ -1,3 +1,7 @@
 package cats

-object implicits extends syntax.AllSyntax with instances.AllInstances
+object implicits extends Implicits0
+
+trait Implicits0 extends syntax.EitherSyntax with Implicits1
+
+trait Implicits1 extends syntax.AllSyntaxWithoutEither with instances.AllInstances
diff --git a/core/src/main/scala/cats/syntax/all.scala b/core/src/main/scala/cats/syntax/all.scala
index 1fc52fc..bdefc5d 100644
--- a/core/src/main/scala/cats/syntax/all.scala
+++ b/core/src/main/scala/cats/syntax/all.scala
@@ -45,3 +45,47 @@ trait AllSyntax
     with WriterSyntax
     with XorSyntax
     with XorTSyntax
+
+trait AllSyntaxWithoutEither
+    extends ApplicativeSyntax
+    with ApplicativeErrorSyntax
+    with ApplySyntax
+    with BifunctorSyntax
+    with BifoldableSyntax
+    with BitraverseSyntax
+    with CartesianSyntax
+    with CoflatMapSyntax
+    with ComonadSyntax
+    with ComposeSyntax
+    with ContravariantSyntax
+    with CoproductSyntax
+    with EqSyntax
+    with FlatMapSyntax
+    with FoldableSyntax
+    with FunctorSyntax
+    with FunctorFilterSyntax
+    with GroupSyntax
+    with InvariantSyntax
+    with ListSyntax
+    with MonadCombineSyntax
+    with MonadErrorSyntax
+    with MonadFilterSyntax
+    with MonoidSyntax
+    with OptionSyntax
+    with OrderSyntax
+    with PartialOrderSyntax
+    with ProfunctorSyntax
+    with ReducibleSyntax
+    with SemigroupSyntax
+    with SemigroupKSyntax
+    with Show.ToShowOps
+    with SplitSyntax
+    with StrongSyntax
+    with TransLiftSyntax
+    with TraverseFilterSyntax
+    with TraverseSyntax
+    with TupleSyntax
+    with ValidatedSyntax
+    with WriterSyntax
+    with XorSyntax
+    with XorTSyntax

I believe we need the members directly on implicits there in order for the prioritization to work. Unless I'm mistaken (which is entirely possible) I don't think there's a way to simply prioritize things given the current layout to make it all work. Assuming this is OK I'll go ahead and submit the PR.

Also, in response to "yeah, I think if you import either and functor, too bad, so sad." it would also mean similarly for any type class that has syntax which already exists on the Either syntax enrichment.

@rossabaker
Copy link
Member

I don't think implicits needs to override anything more than catsSyntaxEither. The others don't conflict with instance syntax. With just that override, I can compile on 2.11 without SI-2712 fix.

But I don't know that this is progress. If you're introducing cats to a teammate, are you better prepared to explain:

  1. "Use this special import, which you can't decompose, because there's this compiler bug that interacts with unary type aliases ..."
  2. "Use this special compiler plugin. It backports a bugfix in Scala 2.12 and makes cats easier."

Since we have to sell magic, I'd rather sell the magic that improves the whole project instead of the magic that fixes one thing and needs to be taught to each new developer.

@adelbertc
Copy link
Contributor Author

adelbertc commented Aug 19, 2016

Is that to say you prefer (1) or (2)?

I think I prefer (2) since that solves the root problem and it'd make Cats in general easier to use (e.g. traverse with Either instead of traverseU).

@rossabaker
Copy link
Member

I prefer (2) as well. Let's teach people the trick that has multiple benefits, is how we will write in Scala 2.12+, etc.

@johnynek
Copy link
Contributor

I would think something like this should work:

object instances extends RightBiasedEitherSyntax

trait RightBiasedEitherSyntax extends Implicits0 {
  // either stuff here
}

this is a bit cleaner since we are not putting the either stuff directly in the object, but still keeps it at highest priority.

@adelbertc
Copy link
Contributor Author

@johnynek Did you mean instances or implicits ?

I'm also wondering if maybe it's not worth doing - it's a manifestation of SI-2712 and we're special casing here. It can be fixed on Scala 2.10.x and 2.11.x via the plugin and for those who don't want to bring it in the workaround is fairly straightforward (momentarily unalias it). My main concern is muddying the codebase for the sake of fixing this special case.. but if people feel strongly enough about this I suppose it can be done. It's just strange to say "if you want this particular syntax use import cats.implicits._ but if you want to a-la carte import then too bad.

@johnynek
Copy link
Contributor

I think it should definitely work cleanly with the plugin. I misunderstood, if that's the case, I think we should not muddy the water and just say that cats really recommends that plugin as a compatibility layer with 2.12.

@adelbertc
Copy link
Contributor Author

👍 Cool beans. Yeah @rossabaker confirmed it works as is with the SI-2712 plugin.

@rossabaker
Copy link
Member

I have not tried anything on 2.12. But yes, with SI-2712-fix, everything works great on 2.11.8 from @adelbertc's branch. I can also confirm that EitherSyntax consistently takes priority over FunctorSyntax, which is what we want for performance.

@non
Copy link
Contributor

non commented Aug 19, 2016

This looks good to me! I agree that we should recommend the SI-2712 plugin, and given the direction stdlib is going I think this PR is the right approach. 👍

Thanks so much for working on this @adelbertc!

@adelbertc
Copy link
Contributor Author

adelbertc commented Aug 19, 2016

Awesome, thanks for all the comments! I think I'll wait for #1266 to be merged to master, merge with my branch here, fix and push. The merge should be straightforward as the change is mostly adding stuff as opposed to changing existing stuff.

EDIT On second thought I think I'll just merge now and offer a PR to #1266 to correct it to use Either - thanks guys!

@adelbertc adelbertc merged commit 34360f8 into typelevel:master Aug 20, 2016
@stew stew removed the in progress label Aug 20, 2016
non pushed a commit to non/cats that referenced this pull request Aug 20, 2016
Due to a communications error we merged some PRs which we had been
planning to keep out of master until after 0.7.0. Since one of them
(typelevel#1289) was large and long-running, it was difficult to undo this
merge in an automated way.

This commit represents the following:

 1. Downloaded patches for PRs 1301 and 1289 from Github
 2. Tried to reverse-apply them (-R)
 3. Fixed rejections/failures manually
 4. Got code compiling and tests passing

The code is now different than it was before the PRs were merged,
but things *seem* to be in a reasonable state. I'm keeping all these
changes in one commit so that it will be easier to "reverse" this
once 0.7.0 is relaesed.
non pushed a commit to non/cats that referenced this pull request Aug 20, 2016
Due to a communications error we merged some PRs which we had been
planning to keep out of master until after 0.7.0. Since one of them
(typelevel#1289) was large and long-running, it was difficult to undo this
merge in an automated way.

This commit represents the following:

 1. Downloaded patches for PRs 1301 and 1289 from Github
 2. Tried to reverse-apply them (-R)
 3. Fixed rejections/failures manually
 4. Got code compiling and tests passing

The code is now different than it was before the PRs were merged,
but things *seem* to be in a reasonable state. I'm keeping all these
changes in one commit so that it will be easier to "reverse" this
once 0.7.0 is relaesed.
non pushed a commit to non/cats that referenced this pull request Aug 20, 2016
Due to a communications error we merged some PRs which we had been
planning to keep out of master until after 0.7.0. Since one of them
(typelevel#1289) was large and long-running, it was difficult to undo this
merge in an automated way.

This commit represents the following:

 1. Downloaded patches for PRs 1301 and 1289 from Github
 2. Tried to reverse-apply them (-R)
 3. Fixed rejections/failures manually
 4. Got code compiling and tests passing

The code is now different than it was before the PRs were merged,
but things *seem* to be in a reasonable state. I'm keeping all these
changes in one commit so that it will be easier to "reverse" this
once 0.7.0 is relaesed.
non pushed a commit to non/cats that referenced this pull request Aug 20, 2016
Due to a communications error we merged some PRs which we had been
planning to keep out of master until after 0.7.0. Since one of them
(typelevel#1289) was large and long-running, it was difficult to undo this
merge in an automated way.

This commit represents the following:

 1. Downloaded patches for PRs 1301 and 1289 from Github
 2. Tried to reverse-apply them (-R)
 3. Fixed rejections/failures manually
 4. Got code compiling and tests passing

The code is now different than it was before the PRs were merged,
but things *seem* to be in a reasonable state. I'm keeping all these
changes in one commit so that it will be easier to "reverse" this
once 0.7.0 is relaesed.
non pushed a commit to non/cats that referenced this pull request Aug 20, 2016
Due to a communications error we merged some PRs which we had been
planning to keep out of master until after 0.7.0. Since one of them
(typelevel#1289) was large and long-running, it was difficult to undo this
merge in an automated way.

This commit represents the following:

 1. Downloaded patches for PRs 1301 and 1289 from Github
 2. Tried to reverse-apply them (-R)
 3. Fixed rejections/failures manually
 4. Got code compiling and tests passing

The code is now different than it was before the PRs were merged,
but things *seem* to be in a reasonable state. I'm keeping all these
changes in one commit so that it will be easier to "reverse" this
once 0.7.0 is relaesed.
adelbertc added a commit to adelbertc/cats that referenced this pull request Aug 20, 2016
All APIs that are present in 0.6.x continue to use `Xor`. New additions, notably FlatMapRec stuff, use `Either`.
adelbertc added a commit that referenced this pull request Aug 20, 2016
adelbertc added a commit to adelbertc/cats that referenced this pull request Aug 21, 2016
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.

9 participants