add Either.left and Either.right typed as Either #6329

jozic · 2018-02-15T19:56:47Z

this is another implementation of #6328 inspired by @LukaJCB and others in gitter(https://gitter.im/typelevel/general?at=5a85d126ce68c3bc748f61d4) (thanks!)

motivation: Left and Right constructors are typed as Left and Right respectively, but sometimes we want something like Option.empty which returns None but typed as Option.

The test represents a code that would not compile if plain Left/Right constructors are used

@SethTisue please check it again, thanks!
If this looks better (it does to me) then #6328 should be closed

LukaJCB · 2018-02-15T21:04:41Z

src/library/scala/util/Either.scala

+   */
+  def left[B]: FromLeftPartiallyApplied[B] = new FromLeftPartiallyApplied[B]
+
+  private[util] final class FromLeftPartiallyApplied[B](private val dummy: Boolean = true) extends AnyVal {


I think these dummys can now be Unit since 2.12.3 :)

SethTisue · 2018-04-25T07:54:06Z

src/library/scala/util/Either.scala

+   */
+  def left[B]: FromLeftPartiallyApplied[B] = new FromLeftPartiallyApplied[B]
+
+  private[util] final class FromLeftPartiallyApplied[B](private val dummy: Unit) extends AnyVal {


@why private[util] here (and 14 lines down), why not fully private? (Guess: is it because otherwise you get a warning about the type escaping...?)

are there any pros/cons to using Unit vs Boolean in this pattern?

I'm pretty sure I've tried to make it fully private, but compiler wasn't happy. I will double check it

As for Unit, i guess it's just less to type, no need to give it default value. In any case it's just a trick for AnyVal.
Maybe someone has a better idea here, as I don't see much difference otherwise

I think I saw somewhere that using Unit didn't work until Scala 2.12.3. that would explain it

as expected

[error] /home/jozic/projects/github/scala/src/library/scala/util/Either.scala:468: private class FromLeftPartiallyApplied escapes its defining scope as part of type scala.util.Either.FromLeftPartiallyApplied[B] [error] def left[B]: FromLeftPartiallyApplied[B] = new FromLeftPartiallyApplied[B] [error] ^ [error] one error found [error] (library/compile:compileIncremental) Compilation failed

SethTisue · 2018-04-25T07:56:04Z

src/library/scala/util/Either.scala

    if (test) Right(right) else Left(left)

+  /**
+   * Creates `FromLeftPartiallyApplied[B]` that given an argument of type `A`


Since this name appears in the Scaladoc, perhaps a shorter name could be found?

Do you have any good ideas? I think the name is long, but very descriptive

SethTisue · 2018-04-25T08:00:49Z

src/library/scala/util/Either.scala

+   *   //l: Either[String, Int] = Left("boo")
+   * }}
+   */
+  def left[B]: FromLeftPartiallyApplied[B] = new FromLeftPartiallyApplied[B]


It's my first time seeing this pattern, so it isn't obvious to me (and might not be obvious to other reviewers) what the downsides of this pattern are. I mean, obviously it's a downside that this extra type is involved (and shows up in the Scaladoc). Are there any concealed footguns here?

that was my first time as well, but it looks safe enough, except the fact you mentioned - it appears in public
like you can do Either.left[Int] and get instance of FromLeftPartiallyApplied[Int]

I'm fairly sure there's no footguns here :)

SethTisue · 2018-04-25T08:08:53Z

why doesn't Cats use this pattern, why does it have the #6328 version instead?

core/src/main/scala/cats/syntax/either.scala:

  def left[A, B](a: A): Either[A, B] = Left(a)
  def right[A, B](b: B): Either[A, B] = Right(b)

if this is merged, is it going to be make difficulties for Cats? (offhand, I guess you'd need separate scala-2.12 and scala-2.13 source directories to handle it...? not sure I've thought it through properly)

LukaJCB · 2018-04-25T08:28:27Z

That's a good question, though I think most cats users use the more convenient a.asLeft[A]. We could probably deprecate those in time for 2.13, I think.

jozic · 2018-04-26T14:38:25Z

I guess @LukaJCB is right, cats is full of implicits and 3.asLeft[String] is a no-brainer in Catsland, where standard lib doesn't have much of those patterns. So I'm assuming extensions like asLeft have fewer chances to be merged in std lib. Please correct me if i'm wrong here

@LukaJCB you meant deprecate def left[A, B](a: A): Either[A, B] = Left(a) and not asLeft, right? :)

SethTisue · 2018-04-26T14:43:01Z

extensions like asLeft have fewer chances to be merged in std lib

yeah I'm not really comfortable with putting extension methods on Any

dwijnand · 2018-06-06T22:20:11Z

Needs a rebase, given #6681 introduced EitherTest too.

jozic · 2018-06-08T04:31:49Z

rebased on 2.13.x

lrytz · 2018-06-08T08:53:13Z

Another approach would be adding methods to Left / Right that upcast an instance.

final case class Left[+A, +B](value: A) extends Either[A, B] {
  def up[T >: B]: Either[A, T] = this
  ...
}

scala> Left(1).up[String]
res0: scala.util.Either[Int,String] = Left(1)

jozic · 2018-06-08T13:28:18Z

@lrytz I guess that also would work
Do you want me to submit another PR with this approach and let others decide?

lrytz · 2018-06-08T13:31:28Z

I'd say we can just discuss here. But if we go that other way, a new PR is less confusing for future us.

adriaanm · 2018-08-03T15:22:28Z

👋 what's the status here?

jozic · 2018-08-03T15:38:51Z

@adriaanm it went through all checks, and to my understanding lacks only acceptance of someone from the team (that could be you :))
@lrytz suggested another approach but so far no one commented on which one is preferred and why

@SethTisue do you have an idea what should the next step be?

SethTisue · 2018-08-07T01:26:54Z

@jozic Lukas's suggestion is a lot simpler and on that grounds alone seems preferable to me. so yes, new PR with that, please. I'll provisionally close this one.

jozic · 2018-08-07T21:20:05Z

@SethTisue new PR is #7011

scala-jenkins added this to the 2.13.0-M4 milestone Feb 15, 2018

LukaJCB reviewed Feb 15, 2018

View reviewed changes

SethTisue mentioned this pull request Feb 15, 2018

add Either.left and Either.right typed as Either #6328

Closed

jozic force-pushed the either2 branch from ab4624e to 306868a Compare February 18, 2018 03:47

SethTisue reviewed Apr 25, 2018

View reviewed changes

szeiger modified the milestones: 2.13.0-M4, 2.13.0-M5 May 2, 2018

SethTisue mentioned this pull request May 21, 2018

change Either subclasses type parameters? scala/bug#10895

Closed

jozic force-pushed the either2 branch from 306868a to 3eef9ec Compare June 7, 2018 07:17

add Either.left and Either.right typed as Either

ba763d1

jozic force-pushed the either2 branch from 3eef9ec to ba763d1 Compare June 8, 2018 03:16

SethTisue closed this Aug 7, 2018

SethTisue removed this from the 2.13.0-M5 milestone Aug 7, 2018

jozic mentioned this pull request Aug 7, 2018

add methods to Left and Right for upcasting the other type parameter #7011

Merged

jozic deleted the either2 branch August 7, 2018 21:18

lrytz mentioned this pull request Nov 5, 2020

Delete useless type parameters from Left and Right (Either) #9303

Closed

add Either.left and Either.right typed as Either #6329

add Either.left and Either.right typed as Either #6329

Uh oh!

Conversation

jozic commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SethTisue commented Apr 25, 2018

Uh oh!

LukaJCB commented Apr 25, 2018

Uh oh!

jozic commented Apr 26, 2018

Uh oh!

SethTisue commented Apr 26, 2018

Uh oh!

dwijnand commented Jun 6, 2018

Uh oh!

jozic commented Jun 8, 2018

Uh oh!

lrytz commented Jun 8, 2018

Uh oh!

jozic commented Jun 8, 2018

Uh oh!

lrytz commented Jun 8, 2018

Uh oh!

adriaanm commented Aug 3, 2018

Uh oh!

jozic commented Aug 3, 2018

Uh oh!

SethTisue commented Aug 7, 2018

Uh oh!

jozic commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants