-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
A mostly clean room reimplementation of Or. #36
Conversation
Implements feedback from typelevel#11. Specifically: 1. LOr and ROr move to `Or.LeftOr` and `Or.RightOr`. 2. Constructors are still public, but `left` and `right` preserved in `OrFunctions` to infer `Or`. Use what you like. 3. Only the Scaladoc is ripped from Scalaz. Also tries to adhere to the emerging consensus on typelevel#27. Preliminar benchmarking showed monomorphism is faster than polymorphism, and that implementing other operations using fold/bimap is immeasurable. The second finding is dubious with respect to allocations. If someone can prove these guilty via benchmark, we can revisit. I'd like to add tests after typelevel#29 happens.
This looks great! If you wouldn't mind adding |
case LOr(_) => left | ||
case ROr(_) => r | ||
} | ||
sealed abstract class Or[+A, +B] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest always doing this for ADTs to get a better LUB when unifying constructors.
sealed abstract class Or[+A, +B] extends Product with Serializable {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpolecat do you have an example of the benefit where it is helpful to add extends Product with Serializable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For abstract classes at the root of ADTs, I've added these silly supers to the root. Here's why. It's even worse than that: the inferred types look like Product with Serializable with Box[B], and they erase to Object, not Box[B], in the stored Java types.
The benefit is improved inference when using case classes explicitly; the drawback is that Product methods appear in the type itself. So keep this, remove it, whichever you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julien-truffaut thus:
sealed trait A
case class Aa() extends A
case class Ab() extends A
scala> List(Aa(), Ab())
res1: List[Product with Serializable with A] = List(Aa(), Ab())
sealed trait A extends Product with Serializable
case class Aa() extends A
case class Ab() extends A
scala> List(Aa(), Ab())
res2: List[A] = List(Aa(), Ab()) // LUB is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpolecat cool I didn't know this trick!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was sitting next to @tpolecat 3 days ago when we both learned this trick during Jon Pretty's nescala talk ;)
@tpolecat Those are great suggestions. I keep mentioning this contributor's guide that doesn't exist, but those would be great things to add to it somewhere. |
new Eq[A Or B] { | ||
def eqv(a1: A Or B, a2: A Or B) = | ||
a1 === a2 | ||
implicit class MergeableOr[A](private val self: A Or A) extends AnyVal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. Why is this is done with syntax rather than a method with a type constraint? i.e., something like
def merge[X >: A](implicit ev: B <:< X]): X = ...
Apologies if I missed an earlier discussion about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scalaz/scalaz#580 discussion in scalaz
Conflicts: data/src/main/scala/cats/data/Or.scala
This lets us override map just once.
This looks good. 👍 One thing that we did in I think we should merge this PR before doing that, but I just wanted to mention it here. |
yeah I agree on the serializable stuff, that's coming up for us with spark. |
concretely what does it mean? Do we simply need to extends all TC with |
Yes, and the instances need to do this as well. I actually have a rule for law-checking which confirms an instances is serializable: https://github.com/non/algebra/blob/master/laws/src/main/scala/algebra/laws/Rules.scala#L12 |
Yep if it matters then it needs to be part of the base test for all typeclasses; |
Should simulacrum do this automatically? I was already considering making simulacrum change all type class traits to universal traits. |
@mpilquist If you are open to it then I think it would be great. I don't see a good reason anyone should specifically want their type classes to be non-serializable. (But of course we still have to remember to extend serializable in the instances too.) |
pardon the stupid question but why do you need the instance to extends |
@julien-truffaut I'm not sure 100% sure why but I think the (Also if someone can demonstrate it working the "normal" way then that would be ideal. I'm just reporting my own experience -- I may have messed something up.) |
Huh. In Java you can inherit it, but maybe it gets squashed out in the trait encoding? |
implicit def OrShow[A, B](implicit showA: Show[A], showB: Show[B]): Show[A Or B] = | ||
Show.show[A Or B](_.fold(a => s"LOr(${showA.show(a)})", b => s"ROr(${showB.show(b)})")) | ||
object Or extends OrInstances with OrFunctions { | ||
final case class LeftOr[+A](a: A) extends (A Or Nothing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the Or
suffix on these names given they are namespaced by the Or
object already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is convenient when the name of the class is unique when you use auto import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. It's really easy to run into trouble when you're shadowing names that are already in scope by default. Common point of bafflement in #scala
for people in the Coursera course, which has exercises to implement List
and Set
.
case ROr(_) => r | ||
} | ||
def fold[C](fa: A => C, fb: B => C): C = this match { | ||
case RightOr(b) => fb(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is really a details but I think it reads slightly better if you pattern match first with LeftOr
since the first function is on A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an almost certainly premature optimization, figuring there'd be more rights than lefts. I'll flip them for readability.
can you add a show method? and implement |
I think we should prefer The reason is that while both allocate |
@non good for me, we should also add this to the contributor guide. |
I think I'm pretty happy with this. 👍 Anyone know a reason why we shouldn't merge this and continue future work in another PR? |
👍 |
A mostly clean room reimplementation of Or.
Implements feedback from #11. Specifically:
Or.LeftOr
andOr.RightOr
.left
andright
preserved inOrFunctions
to inferOr
. Use what you like.Also tries to adhere to the emerging consensus on #27.
Preliminary benchmarking showed monomorphism is faster than polymorphism,
and that implementing other operations using fold/bimap is immeasurable.
The second finding is dubious with respect to allocations. If someone
can prove these guilty via benchmark, we can revisit.
I'd like to add tests after #29 happens.
Fixes #11.