Implement CanFail #2049

adamgfraser · 2019-10-28T19:35:33Z

Resolves #2047.

Adds a new implicit instance CanFail[E] which provides implicit evidence that an effect with an error type E can fail, that is that E is not equal to Nothing. Requires such evidence to exist for ZIO combinators such as orElse that only make sense if an effect can fail.

Not implemented yet is adding annotations so that the implicit type does not appear in the Scaladoc. It looks like this has to be done through the @useCase annotation, but one watch out there is it requires you to specify the "simplified" type signature as a text string, so there is some risk that the documentation gets out of sync with the actual type signature since it is not tied to the code.

Also not implemented is applying this technique to other effect types such as ZManaged and ZStream.

core-tests/shared/src/test/scala/zio/ZIOSpec.scala

core/shared/src/main/scala/zio/ZIO.scala

core/shared/src/main/scala-2.11/zio/CanFail.scala

vasilmkd

Should we add this to ZSink and ZStream combinators as well?

adamgfraser · 2019-10-29T01:48:44Z

@vasilmkd Yes definitely.

neko-kai · 2019-10-29T12:37:19Z

@adamgfraser
What do you think about adding tests that the combinators actually fail when called with Nothing?

For Scala 2 you can use shapeless.test.illTyped in test scope
Dotty apparently has a builtin scala.compiletime.testing.typeChecks function - example

Importantly, behavior of ambiguous implicits changed in Dotty, so I'm not sure the same approach would work there...

adamgfraser · 2019-10-29T12:48:34Z

@neko-kai Yes, we should definitely test this. I also wonder if we should add this functionality to ZIO Test.

You’re right about Dotty. We need to use a slight variation of this technique to work across all platforms. I’ll push a commit to fix that shortly.

jdegoes · 2019-10-29T14:40:37Z

@neko-kai @adamgfraser Please no dependency on Shapeless. If we do anything for this, it should be possible to support Scala 2.x and 3.x simultaneously.

core-tests/jvm/src/test/scala-2.12/zio/StacktracesSpec.scala

core-tests/shared/src/test/scala/zio/ZIOSpec.scala

core-tests/shared/src/test/scala/zio/ZScheduleSpec.scala

core/shared/src/main/scala-2.11/zio/CanFail.scala

core/shared/src/main/scala/zio/ZIO.scala

docs/datatypes/ref.md

streams-tests/jvm/src/test/scala/zio/stream/StreamSpec.scala

adamgfraser · 2019-10-29T15:22:30Z

Okay, so to summarize I rolled back fold and foldM.

Open questions / next steps:

Making similar changes to other ZIO data types.
Do we want to use the @useCase annotation for these? Personally I am somewhat inclined to leave it as is. First, the name CanFail now makes it very clear what it is, thanks to your good suggestion to use a more specific type constraint, and we can turn it into a feature. Second, it will make sure the type signature stays in sync for any changes to these methods. Third, it avoids the "that isn't actually the type signature" critique that I've seen applied to other uses of @useCase. But happy to do whatever you think is best.
Testing - what do you think about pursuing a separate PR to add some "does not compile" functionality to ZIO Test. We would have to do some platform specific work for Scala 2 versus Dotty but I think we could do all of that on the backend and expose a consistent API across platforms that the entire ZIO ecosystem could use so they didn't have to rely on third part libraries. ScalaTest offers this functionality. To be clear I am envisioning this not requiring any dependencies.

adamgfraser · 2019-10-29T17:19:06Z

It may not be worth it but we could potentially do the same thing with the R type (e.g. it doesn't make sense to call provide on an effect that does not require any environment).

vasilmkd · 2019-10-29T17:23:29Z

Of course it's worth it. ZIO is a next-generation effect system after all. 💪

jdegoes · 2019-10-30T09:39:37Z

Okay, so to summarize I rolled back fold and foldM.

@neko-kai argued to leave them on grounds that fold can always be replaced by map when the error is Nothing, and foldM can always be replaced by flatMap when the error is Nothing.

I think it is a valid argument, it just concerned me when I saw, e.g. foldM being replaced by foldCauseM merely to work around the (new) compiler error.

What do you think?

Making similar changes to other ZIO data types.

Makes sense.

First, the name CanFail now makes it very clear what it is, thanks to your good suggestion to use a more specific type constraint, and we can turn it into a feature.

I think you're right about this, CanFail is good enough.

Testing - what do you think about pursuing a separate PR to add some "does not compile" functionality to ZIO Test.

Agreed, if we had something that was cross Scala (2.11 - Dotty), it could make sense to add to ZIO Test, and that's precisely the case when I'd feel comfortable adding it here.

vasilmkd · 2019-10-30T09:53:06Z

Probably a good idea to watch this talk, I found it pretty fascinating when I first saw it. There might be some good hints when it comes to naming as well:

Plain Functional Programming by Martin Odersky.

adamgfraser · 2019-10-30T12:38:13Z

@jdegoes I don’t think uio.fold or uio.foldM should compile since they can always be rewritten in terms of map and flatMap. It was my mistake in not rewriting one of the tests correctly. What we should do is put together documentation showing how to rewrite code for this change. I will put together a table that we can reference in the release notes or elsewhere.

I will add back the changes to fold and foldM and make similar changes to other data types.

I have also been working on something to test that code does not compile so I will post that shortly, though that will be in a separate PR.

Did you have any interest in doing something similar for the environment type?

adamgfraser · 2019-10-30T13:31:27Z

ZIO

Code	Rewrite
`uio <> zio`	`uio`
`uio.asError(e)`	`uio`
`uio.bimap(f, g)`	`uio.map(g)`
`uio.catchAll(f)`	`uio`
`uio.catchSome(pf)`	`uio`
`uio.either`	`uio`*
`uio.eventually`	`uio`
`uio.flatMapError(f)`	`uio`
`uio.fold(f, g)`	`uio.map(g)`
`uio.foldM(f, g)`	`uio.flatMap(g)`
`uio.mapError(f)`	`uio`
`uio.option`	`uio`*
`uio.orDie`	`uio`
`uio.orDieWith(f)`	`uio`
`uio.orElse(zio)`	`uio`
`uio.orElseEither(zio)`	`uio`*
`uio.refineOrDie(pf)`	`uio`
`uio.refineOrDieWith(pf)(f)`	`uio`
`uio.refineToOrDie`	`uio`
`uio.retry(s)`	`uio`
`uio.retryOrElse(s, f)`	`uio`
`uio.retryOrElseEither(s, f)`	`uio`*
`uio.tapBoth(f, g)`	`uio.tap(g)`
`uio.tapError(f)`	`uio`

ZManaged

Code	Rewrite
`umanaged <> zmanaged`	`umanaged`
`umanaged.bimap(f, g)`	`umanaged.map(g)`
`umanaged.catchAll(f)`	`umanaged`
`umanaged.catchSome(pf)`	`umanaged`
`umanaged.either`	`umanaged`*
`umanaged.flatMapError(f)`	`umanaged`
`umanaged.fold(f, g)`	`umanaged.map(f)`
`umanaged.foldM(f, g)`	`umanaged.flatMap(g)`
`umanaged.mapError(f)`	`umanaged`
`umanaged.option`	`umanaged`*
`umanaged.orDie`	`umanaged`
`umanaged.orDieWith(f)`	`umanaged`
`umanaged.orElse(zmanaged)`	`umanaged`
`umanaged.orElseEither(zmanaged)`	`umanaged`
`umanaged.refineOrDie(pf)`	`umanaged`
`umanaged.refineToOrDie`	`umanaged`
`umanaged.refineToOrDieWith(pf)(f)`	`umanaged`
`umanaged.retry(s)`	`umanaged`

ZStream

Code	Rewrite
`ustream.bimap(f, g)`	`ustream.map(g)`
`ustream.catchAll(f)`	`ustream`
`ustream.either`	`ustream`*
`ustream.mapError(f)`	`ustream`
`ustream.orElse(zstream)`	`ustream`

ZStreamChunk

Code	Rewrite
`ustream.either`	`ustream`
`ustream.orElse(zstream)`	`ustream`

either, option, orElseEither, and retryOrElseEither wrap their results in Some or Right so after rewriting, code calling these methods can be simplified to accept an A rather than an Option[A] or Either[E, A].

adamgfraser · 2019-10-30T14:37:26Z

This is ready for another review.

jdegoes · 2019-10-31T09:25:04Z

I don’t think uio.fold or uio.foldM should compile since they can always be rewritten in terms of map and flatMap. It was my mistake in not rewriting one of the tests correctly. What we should do is put together documentation showing how to rewrite code for this change. I will put together a table that we can reference in the release notes or elsewhere.

OK, sounds great!

Did you have any interest in doing something similar for the environment type?

Yes, I think it's worthwhile to do that. Maybe RequiresEnv for that one?

adamgfraser · 2019-10-31T09:43:25Z

Okay, great! I will open a separate PR to do the same thing for the environment type. Maybe NeedsEnv? Happy to do either.

jdegoes · 2019-10-31T09:54:11Z

Like NeedsEnv 👍

regiskuckaertz · 2019-10-31T09:54:18Z

core/shared/src/main/scala-2.11/zio/CanFail.scala

+  implicit final def canFail[E]: CanFail[E] = CanFail
+
+  implicit final val canFailAmbiguous1: CanFail[Nothing] = CanFail
+  implicit final val canFailAmbiguous2: CanFail[Nothing] = CanFail


For my own understanding, why is there two such values? Also it may be useful to add an explanatory comment for the future.

We want implicit search to fail when E =:= Nothing so we create two identical instances of CanFail[Nothing] to ensure that the result will always be ambiguous. I can add some documentation when I do the same thing the environment type.

adamgfraser · 2019-10-31T10:10:19Z

I'm on it!

* implement CanFail * fix typo * cache values * update documentation * fix type inference issue * turn off warning for unused implicit parameters * fix dotty compatibility issue * address review comments * add back fold and foldM * add <> * add ZManaged * add ZStream * add ZStreamChunk * cleanup * remove extraneous type parameter

implement CanFail

9f8b8c6

mschuwalow previously approved these changes Oct 28, 2019

View reviewed changes

core-tests/shared/src/test/scala/zio/ZIOSpec.scala Show resolved Hide resolved

core/shared/src/main/scala/zio/ZIO.scala Outdated Show resolved Hide resolved

core/shared/src/main/scala/zio/ZIO.scala Outdated Show resolved Hide resolved

neko-kai reviewed Oct 28, 2019

View reviewed changes

core/shared/src/main/scala-2.11/zio/CanFail.scala Outdated Show resolved Hide resolved

adamgfraser added 2 commits October 28, 2019 16:02

fix typo

0fef395

merge upstream changes

617e7e7

adamgfraser dismissed mschuwalow’s stale review via 617e7e7 October 28, 2019 21:00

cache values

4dd8bf2

vasilmkd reviewed Oct 28, 2019

View reviewed changes

adamgfraser added 2 commits October 28, 2019 21:25

update documentation

842b63f

fix type inference issue

38eb524

turn off warning for unused implicit parameters

e5169bc

adamgfraser requested a review from jdegoes October 29, 2019 03:11

fix dotty compatibility issue

fa47d50