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

Fix #617: Make Resource covariant in F and A #731

Merged
merged 6 commits into from
Dec 31, 2019

Conversation

neko-kai
Copy link

Following the discussion in #617 this simply makes Resource covariant. Binary compatibility is not affected as non-generic parts of the method signatures are unchanged.

@neko-kai
Copy link
Author

Strange Travis errors, not related to sbt:

bundler's executable "bundle" conflicts with /home/travis/.rvm/rubies/ruby-2.6.0/bin/bundle

Overwrite the executable? [yN]  

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

Maybe the build needs restarting?

@neko-kai neko-kai closed this Dec 17, 2019
@neko-kai neko-kai reopened this Dec 17, 2019
@rossabaker
Copy link
Member

That's yet another gift from our Ruby dependency. It's fixed in #729.

@neko-kai
Copy link
Author

@rossabaker Thanks for pointing that out!

@neko-kai
Copy link
Author

MiMa failures seem to be false positives, lightbend-labs/mima#363

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, but I'd like to hear from @mpilquist or @SystemFw, who as noted in the linked ticket did similar in fs2.

@rossabaker
Copy link
Member

Note the Ruby change will conflict with #733, which I think is a better solution, but it will be an easy merge for whoever goes second.

@LukaJCB
Copy link
Member

LukaJCB commented Dec 20, 2019

Do we know how this will affect type inference? I can see some extra type annotations in the changes that weren't needed before.

@neko-kai
Copy link
Author

@LukaJCB
Seems like F parameter infers worse in Resource.pure when import cats.implicits._ is present, because many high-priority candidates for Applicative cause 'ambiguous implicits' error:

Error:(468, 37) ambiguous implicit values:
 both method catsStdInstancesForEither in trait EitherInstances of type [A]=> cats.MonadError[[β$0$]scala.util.Either[A,β$0$],A] with cats.Traverse[[β$1$]scala.util.Either[A,β$1$]]
 and value catsStdBimonadForFunction0 in trait Function0Instances of type => cats.Bimonad[Function0]
 match expected type cats.Applicative[F]
      case Right(a) => Resource.pure(a)

fs2 avoids that in Stream.apply by returning in essence Stream[Nothing, A], but Resource's pure is not free, so we can't do the same.

Personally, I use neither cats.implicits nor Resource.pure, and I expect that aside of .pure the inference should be on par with fs2.

@djspiewak
Copy link
Member

This is awesome, thank you so much! I want to sort out the false positives on mima so we don't break the master build. I'm not super concerned about the source compatibility issue with pure; it's unfortunate, but not the end of the world, and maybe fixable later before release.

@joroKr21
Copy link
Member

Hmm that specific problem (type inference of pure) seems to be solved by scala/scala#8582, although for reasons other than implicit scope. Tried quickly in the REPL:

scala> class Applicative[F[_]]
class Applicative

scala> implicit val apList: Applicative[List] = new Applicative[List]
val apList: Applicative[List] = Applicative@2ad7bd26

scala> implicit val apVec: Applicative[Vector] = new Applicative[Vector]
val apVec: Applicative[Vector] = Applicative@149d7cc6

scala> class Resource[+F[_], +A]
class Resource

scala> def resource[F[_], A](implicit ap: Applicative[F]): Resource[F, A] = new Resource
def resource[F[_], A](implicit ap: Applicative[F]): Resource[F,A]

scala> val x: Resource[List, Int] = resource
val x: Resource[List,Int] = Resource@33f81280

I wonder what that means about enhancing lower bounds (but probably not safe in Scala 2 due to LUBs).

@neko-kai
Copy link
Author

@joroKr21
That's great, another quality of life boost for variance connoiseurs brought on by your patch, thanks! 🙏

@neko-kai
Copy link
Author

@djspiewak I've added exclusions for MiMa, the build passes now.

@djspiewak djspiewak merged commit 9d22570 into typelevel:master Dec 31, 2019
@djspiewak
Copy link
Member

Sorry for the long delay on this. Thanks for all your hard work, @neko-kai!

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.

5 participants