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

Right-bias Either #5135

Merged
merged 2 commits into from Jul 12, 2016
Merged

Right-bias Either #5135

merged 2 commits into from Jul 12, 2016

Conversation

@soc
Copy link
Member

soc commented Apr 27, 2016

  • Add operations like map, flatMap which assume right-bias
  • Deprecate {Left,Right}Projection
  • Deprecate left and right in favor of swap
  • Add contains, toOption, toTry, toSeq and filterOrElse
  • toSeq returns collection.immutable.Seq instead of collection.Seq
  • Don't add get

There are no incompatible changes.
The only possibility of breakage that exists is when people have added
extension methods named map, flatMap etc. to Either in the past doing
something different than the methods added to Either now.

One detail that moved the scales in favor of deprecating LeftProjection
and RightProjection was the desire to have toSeq return
scala.collection.immutable.Seq instead of scala.collection.Seq
like LeftProjection and RightProjection do.
Therefore keeping LeftProjection and RightProjection would introduce
inconsistency.

filter is called filterOrElse because filtering in a for-comprehension
doesn't work if the method needs an explicit argument.

contains was added as safer alternative to
if (either.isRight && either.right.get == $something) ...

While adding filter with an implicit zero value is possible, it's
dangerous as it would require that developers add a "naked" implicit
value of type A to their scope and it would close the door to a future
in which the Scala standard library ships with Monoid and filter could
exist with an implicit Monoid parameter.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 27, 2016
@soc
Copy link
Member Author

soc commented Apr 27, 2016

Discuss!

Some notes:

  • filter is called filterOrElse because filtering in a for-comprehension doesn't work if the method needs an explicit argument, and I don't want to close the door to a future in which the Scala standard library ships with Monoid and filter could exist with an implicit Monoid parameter.
  • There is a lot of code doing no-ops like case Left(a) => Left(a) replacing it with this would require a cast. Not sure what's the right decision between efficiency and code beauty is ...
@smarter
Copy link
Contributor

smarter commented Apr 27, 2016

There is a lot of code doing no-ops like case Left(a) => Left(a) replacing it with this would require a cast.

Can't you do something like this match { case left @ Left(_) => left; ... }

@soc
Copy link
Member Author

soc commented Apr 27, 2016

@smarter the problem is the other case: If Right as any kind of transformation, the type of Left doesn't fit anymore (Left and Right both carry both type params).

If you have e. g. map with R => S then the result is Either[L, S], but Left is still Either[L, R].

@Sciss
Copy link

Sciss commented Apr 27, 2016

I think the problem is if you want to introduce right-bias, you should also redefine Left as Either[A, Nothing]. The following package works as @smarter suggested:

sealed trait Either[+A, +B]

final case class Left[+A](a: A) extends Either[A, Nothing]

final case class Right[+B](b: B) extends Either[Nothing, B]

implicit class EitherOps[A, B](in: Either[A, B]) {
  def flatMap[AA >: A, Y](f: B => Either[AA, Y]): Either[AA, Y] = in match {
     case l @ Left (_) => l
     case     Right(b) => f(b)
  }

  def map[Y](f: B => Y): Either[A, Y] = in match {
    case l @ Left (_) => l
    case     Right(b) => Right(f(b))
  }
}
@soc
Copy link
Member Author

soc commented Apr 27, 2016

@Sciss I agree with you, but this would break every single code base where someone wrote Left[L, R] or Right[L, R].

@Sciss
Copy link

Sciss commented Apr 27, 2016

You could also safely use asInstanceOf[Left[AA, Y]] - it's ugly as hell, but scala-collections is already full of it...

@Sciss
Copy link

Sciss commented Apr 27, 2016

Why not deprecate Either in 2.12, add a correct implementation e.g. Or, and then in 2.14 (?) we drop the problematic Either?

@soc
Copy link
Member Author

soc commented Apr 27, 2016

@Sciss Might be possible. I just want to avoid making library author's lives who want to support 2.10-2.1x harder than necessary.

@EECOLOR
Copy link
Contributor

EECOLOR commented Apr 27, 2016

I might have missed it, but it seems it's missing the withFilter. This is required if you want to extract things in a for comprehension:

for {
  (a, b) <- ...
} yield ...

Oh wait, this is probably problematic... what to do when A => Boolean returns false...

I wished Scala made the distinction between extracting tuples or case classes and guards or pattern matching.

@soc
Copy link
Member Author

soc commented Apr 27, 2016

@EECOLOR withFilter has the same problem as filter.

*
* {{{
* Right(12).right.foreach(x => println(x)) // prints "12"
* Left(12).right.foreach(x => println(x)) // doesn't print

This comment has been minimized.

@xuwei-k

xuwei-k Apr 28, 2016 Contributor

Right(12).foreach(x => println(x)) // prints "12"
Left(12).foreach(x => println(x))  // doesn't print
@soc soc force-pushed the soc:topic/biased-either branch from b485474 to 3a260ad May 7, 2016
@soc
Copy link
Member Author

soc commented May 7, 2016

Updated!

Things to discuss:

  • Add get to Either?
    Most opinions: "no"
  • Added toTry. Good/Bad? (Should it special-case fatal exceptions?)
    Opinions somewhere between "don't care" and "sure, why not?"
  • Should RightProjection be deprecated?
    Opinions generally in favor
  • Should LeftProjection be deprecated? (In favor of swap)
    Opinions generally in favor, but less so than for deprecating RightProjection
  • More precise return type for Either#toSeq? (collection.immutable.Seq instead of collection.Seq) Opinions varied, usually along the lines of "would be nice if the inconsistency with LeftProjection and RightProjection doesn't become to apparent" -> Reason for deprecating LeftProjection and RightProjection?
  • Should we switch all cases to check Right first?
    Mostly, I flipped cases for Either and RightProjection, keeping LeftProjection as-is.
@japgolly
Copy link
Contributor

japgolly commented May 7, 2016

Here's a tiny nit-pick, when using right-biased either the most frequent path is generally the success path (i.e. the Right path). Therefore it would be slightly better performance to switch most of the cases from

case Left (_) => _
case Right(_) => _

to

case Right(_) => _
case Left (_) => _

because IIUC Scala will check each case sequentially. One of those things you'd never notice would can add up after a large number of calls.

@soc soc force-pushed the soc:topic/biased-either branch 2 times, most recently from ea1d1c6 to 6af4b0a May 7, 2016
@soc
Copy link
Member Author

soc commented May 11, 2016

@japgolly I flipped the checks in the Either and Right types, left Left as is. I think this should be the optimal configuration to have the first check succeed.

@som-snytt
Copy link
Contributor

som-snytt commented May 11, 2016

@japgolly b/c "In the interest of efficiency the evaluation of a pattern matching expression may try patterns in some other order than textual sequence." I'd be interested in knowing whether the compiler does the right thing when there are just two choices.

@soc
Copy link
Member Author

soc commented May 13, 2016

/rebuild

@soc soc force-pushed the soc:topic/biased-either branch 2 times, most recently from e750f09 to 5cdb028 May 17, 2016
@soc
Copy link
Member Author

soc commented May 17, 2016

Added ScalaCheck tests.

case Left(a) => this.asInstanceOf[Either[A, Y]]
}

/** Returns `None` if this is a `Left` or if the

This comment has been minimized.

@szeiger

szeiger May 18, 2016 Member

"Returns this?"

This comment has been minimized.

@soc

soc May 19, 2016 Author Member

Is this different from #5135 (comment)?

This comment has been minimized.

@szeiger

szeiger May 20, 2016 Member

I was referring to "Returns None" in the doc comment. None is an Option. This method returns an Either.

This comment has been minimized.

@soc

soc May 20, 2016 Author Member

Ah, I see, sorry. Looks like I fixed this in a later change.

* Left(12).filterOrElse(_ > 10, -1) // Right(-1)
* }}}
*/
def filterOrElse[BB >: B](p: B => Boolean, zero: => BB): Either[A, BB] = this match {

This comment has been minimized.

@szeiger

szeiger May 18, 2016 Member

What's the motivation for adding this method? Neither RightProjection, Option nor Try currently have it.

This comment has been minimized.

@soc

soc May 19, 2016 Author Member

People seem to want filter (as observed from third-party libraries), but as we lack a Monoid type in the standard library, there is no way to make it work in for-comprehensions without introducing a dangerous implicit zero: BB parameter).

Picking a different name allows this method to exist without interfering with for-comprehension and doesn't prevent us from adding a for-comprehension-compatible filter in the future if the std lib ever gets Monoid.

If we want this method, we should discuss whether => B shoud be B instead, and whether we should move the parameter to a different param list...

This comment has been minimized.

@szeiger

szeiger May 20, 2016 Member

I see. I assumed the reason why people seem to want filter is only to enable for comprehensions.

@szeiger
Copy link
Member

szeiger commented May 18, 2016

Is a pattern match the best solution for implementing all these methods? It should compile to an instanceOf check but I would expect polymorphic dispatch to be faster still.

@szeiger
Copy link
Member

szeiger commented May 18, 2016

My take on the discussion points:

  • Deprecate RightProjection (not needed anymore)
  • Deprecate LeftProjection (in favor of swap)
  • Make toSeq return an immutable.Seq. toSeq methods in the collections library have varying return types. Generally immutable collections return an immutable.Seq (and declare it as such). Either is immutable so it would be consistent with the other implementations.
@mpilquist
Copy link

mpilquist commented Sep 13, 2016

@dwijnand I think doing so will trigger an unused import warning on 2.12 though. The extension class is marked private[fs2] which should accomplish the same, no? (Ignoring non-Scala clients)

@dwijnand
Copy link
Member

dwijnand commented Sep 13, 2016

Hmm, not sure.

@SethTisue
Copy link
Member

SethTisue commented Sep 13, 2016

re: warnings, have y'all tried https://github.com/ghik/silencer ?

@sjrd
Copy link
Member

sjrd commented Sep 13, 2016

@SethTisue No, I haven't. In theory, it looks interesting. In practice, Scala.js cannot afford to have any Scala dependency besides Scala itself. Otherwise, we wouldn't be able to publish for new Scala versions as quickly as needed. Note that most of the ecosystem transitively depends on Scala.js being published at this point, so we need to stay very close to the root of the dependency graph.

@SethTisue
Copy link
Member

SethTisue commented Sep 26, 2016

I wonder if we should delay the deprecation of .left and .right until 2.13, to facilitate warning-free 2.11/2.12 cross-building. in my work on the Scala 2.12 community build lately I'm seeing huge amounts of these deprecation warnings. and it's not clear (to me anyway) how satisfying the suggested replacement for .left (namely) (.swap) is in practice. and if you are cross-building, there is no suitable substitute for .right

community: how are y'all feeling about this? now that 2.12.0-RC1 has been out for a few weeks

supporting 2.11/2.12 cross-building was one of our core promises for 2.12

in addition to the comments on this in the last 13 days, see also previous discussion back in June, beginning with #5135 (comment)

@Ichoran
Copy link
Contributor

Ichoran commented Sep 26, 2016

I would rather not deprecate it immediately. The benefit of right-biasing need not come with such a heavy penalty of changing existing code and living with deprecation warnings. Yes, we'll be tied to .right and .left for longer, but the cost is relatively low if they are things that people don't want to use, and if people do want to use them, why are we taking them away so fast?

@mpilquist
Copy link

mpilquist commented Sep 26, 2016

+1 for delaying the deprecation until 2.13. This also has the benefit of giving us some time to consider what other changes (if any) we want to make to Either.

@ghost
Copy link

ghost commented Sep 26, 2016

My 2 cents: Other than two, reported scalac issues, Either changes are the only remaining issues and as noted ^^^ are a killer for cross version compiling, unless we all write some compat code such as pasted by @mpilquist .

Is it not possible to have a standard "2.12 fwd compat" module for 2.10 and 2.11? And/or a backwards compat module for 2.12 so existing code works out-of-the-box? Or in other words, keep the change but have tools and strategies in place for easy migration.

@milessabin
Copy link
Contributor

milessabin commented Sep 26, 2016

It's a shame, but I'm also in favour of postponing the deprecation. I'd like to see support for early migration to the new form though.

@dwijnand
Copy link
Member

dwijnand commented Sep 27, 2016

@mpilquist

@dwijnand I think doing so will trigger an unused import warning on 2.12 though. The extension class is marked private[fs2] which should accomplish the same, no? (Ignoring non-Scala clients)

Yep, seems to do the trick, nice one (I can stop creating impl package objects for these then) 👍

scala> import scala.util._
import scala.util._

scala> val e: Either[String, Int] = Right(1)
e: scala.util.Either[String,Int] = Right(1)

scala> e.map(_ + 1)
<console>:16: error: value map is not a member of scala.util.Either[String,Int]
       e.map(_ + 1)
         ^

scala> import fs2._
import fs2._

scala> e.map(_ + 1)
<console>:19: error: value map is not a member of scala.util.Either[String,Int]
       e.map(_ + 1)
         ^
@SethTisue
Copy link
Member

SethTisue commented Sep 28, 2016

we discussed this at a Scala team meeting as well. there seems to be a consensus that the deprecation should be delayed. I'll make a PR for 2.12.0-RC2

@ghost
Copy link

ghost commented Sep 28, 2016

@SethTisue Thanks for the heads up.

Is there a rough ETA for 2.12.0-RC2? And related to that, what would the advice be for a project like cats that has made some unpublished changes around this, but is yet to publish as 2.12.0-RC1? ie should it hold off on a 2.12.0-RC1 release or....?

@adriaanm
Copy link
Member

adriaanm commented Sep 28, 2016

We plan to merge the last few RC2 PRs this week, and release next week,
assuming no new blocker bugs are discovered before the end of the week.
On Wed, Sep 28, 2016 at 13:33 BennyHill notifications@github.com wrote:

@SethTisue https://github.com/SethTisue Thanks for the heads up.

Is there a rough ETA for 2.12.0-RC2? And related to that, what would
the advice be for a project like cats that has made some unpublished
changes around this, but is yet to publish as 2.12.0-RC1? ie should it hold
off on a 2.12.0-RC1 release or....?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5135 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjywkYFRSqWe4G7vq_v39ixvCYdNmQks5qus8zgaJpZM4IRRVH
.

@ghost
Copy link

ghost commented Sep 28, 2016

👍

@SethTisue
Copy link
Member

SethTisue commented Sep 28, 2016

Is there a rough ETA for 2.12.0-RC2?

We're hoping to freeze it in the next couple days, so then there will be a nightly which is a release-candidate-candidate. If all goes well, RC2 could conceivably hit Maven Central next week sometime. Keep an eye on https://github.com/scala/scala/milestones/2.12.0-RC2

And related to that, what would the advice be for a project like cats that has made some unpublished changes around this, but is yet to publish as 2.12.0-RC1? ie should it hold off on a 2.12.0-RC1 release or....?

Unless there's a great deal of extra hassle involved, I would still strongly recommend everyone try to publish for 2.12.0-RC1 — especially a popular dependency like Cats. The best way to avoid having to do an RC3 (heaven forbid) is for everyone to be doing as much testing/publishing as possible now.

SethTisue added a commit to SethTisue/scala that referenced this pull request Sep 29, 2016
for two reasons:
* to facilitate warning-free cross-compilation between Scala 2.11
  and 2.12
* because it's not clear that .swap is a good replacement for .left

Either.right seems almost certain to be deprecated in 2.13.
Either.left's future is uncertain; see discussion (and links to
additional discussions) at scala#5135
@adriaanm adriaanm modified the milestone: 2.12.0-RC1 Oct 29, 2016
@adriaanm adriaanm added the 2.12 label Oct 29, 2016
rtyley added a commit to rtyley/scalatest that referenced this pull request Feb 13, 2020
* scala/scala#5135 - `Either` became right-biased
  in Scala 2.12
* scala/scala#6682 - `either.right` becomes a
  deprecated field with Scala 2.13

With Scala 2.13, it's no longer possible to test an `Either`'s right value
using ScalaTest's `EitherValues` without getting a deprecating warning -
ie writing:

```
either.right.value should be > 9
```

will give you this on compile:

```
method right in class Either is deprecated (since 2.13.0): Either is now
right-biased, use methods directly on Either
```

Given that `Either` is now right-biased, this change gives us the ability
to write:

```
either.value should be > 9
```
rtyley added a commit to rtyley/scalatest that referenced this pull request Feb 13, 2020
* scala/scala#5135 - `Either` became right-biased
  in Scala 2.12
* scala/scala#6682 - `either.right` becomes a
  deprecated field with Scala 2.13

With Scala 2.13, it's no longer possible to test an `Either`'s right value
using ScalaTest's `EitherValues` without getting a deprecating warning -
ie writing:

```
either.right.value should be > 9
```

will give you this on compile:

```
method right in class Either is deprecated (since 2.13.0): Either is now
right-biased, use methods directly on Either
```

Given that `Either` is now right-biased, this change gives us the ability
to write:

```
either.value should be > 9
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.