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

We need to talk about Stream #2982

Closed
travisbrown opened this issue Aug 9, 2019 · 7 comments
Closed

We need to talk about Stream #2982

travisbrown opened this issue Aug 9, 2019 · 7 comments

Comments

@travisbrown
Copy link
Contributor

Problem

The current Cats 2.0.0 release candidate (RC1) assumes that Scala 2.13 users will be able to alias the new LazyList to Stream (which is deprecated in 2.13.0). For example, the cats.instances.stream object doesn't actually provide instances for Stream on 2.13:

scala> import cats.Traverse, cats.instances.stream._
import cats.Traverse
import cats.instances.stream._

scala> Traverse[Stream]
               ^
       error: could not find implicit value for parameter instance: cats.Traverse[Stream]

But it does provide LazyList instances:

scala> Traverse[LazyList]
res1: cats.Traverse[LazyList] = cats.instances.LazyListInstances$$anon$1@43b7181d

There's also a lazyList object that provides exactly the same instances:

scala> import cats.Traverse, cats.instances.lazyList._
import cats.Traverse
import cats.instances.lazyList._

scala> Traverse[LazyList]
res0: cats.Traverse[LazyList] = cats.instances.LazyListInstances$$anon$1@75decfcd

So the stream object is still around just to facilitate cross-compilation, but only for users who can alias LazyList. For everyone else it's just confusing—when you import the contents of something called instances.stream you're probably going to assume that you'll get some Stream instances.

We're not even really consistent about this, in that cats-kernel takes a slightly different approach: like cats-core, it provides a stream object in cats.kernel.instances that also contains LazyList instances but not Stream ones, but unlike cats-core it doesn't have a lazyList object.

None of this would be that big of a problem if everyone using 2.13 could just alias LazyList (there would still be the inconsistencies, but we have lots of inconsistencies), but that's not realistic, since there are lots of libraries that are published for 2.13 that still use Stream.

Solutions

There are a few things we could do to address these problems in Cats 2.0:

Provide stream instances

We could have cats.instances.stream and cats.kernel.instances.stream provide exactly the same instances across 2.11, 2.12, and 2.13, and introduce new cats.instances.lazyList and cats.kernel.instances.lazyList objects for 2.13. This seems to me like the right thing to do. The downside is that it means we have deprecation warnings, which means we either have to disable fatal warnings or move these instances to entirely separate modules that cats-kernel and cats-core depend on. I'd advocate for turning off fatal warnings, but I know that's not a battle I'm going to win. Ideally we'd be consistent about our names everywhere, not using "Stream" to mean "LazyList", but I know that's even less likely to happen.

Don't provide either instances or instance packages

If we decide not to provide instances for Stream on 2.13, it seems to me pretty aggressively user-unfriendly to provide some objects called instances.stream, because "a missing import error is easier to diagnose than a missing instance error". We could drop cats.instances.stream and replace cats.kernel.instances.stream on 2.13, and users who are able to alias LazyList and want to cross-compile can use the cats.instances.all import.

Nothing

We could just leave things the way they are, possibly introducing a lazyList object in cats-kernel to get rid of that inconsistency.

@travisbrown
Copy link
Contributor Author

(I should note that I'm happy to do all of the work required to make either of these proposals happen.)

@travisbrown
Copy link
Contributor Author

@kailuowang
Copy link
Contributor

kailuowang commented Aug 9, 2019

move these instances to entirely separate modules that cats-kernel and cats-core depend on

Won't that be circular dependency?
Principally I don't think cats should continue provide instance for deprecated things. If other libs choose to continue depends on that, a separate repo can provide such instances for their users (I myself am one of them).

The naming of stream related stuff in cats is for the sake of cross compiling. I am okay if we remove the instance.stream reference in 2.13, to me it's no big deal either way. The name stream will be a lie in many libs on 2.13, e.g. scalacheck.

I am 👍 adding a lazylist object to kernel for consistency.

@travisbrown
Copy link
Contributor Author

Won't that be circular dependency?

Oh, right. Something like that came up briefly in the conversation yesterday but I didn't really think it through.

@Daenyth
Copy link
Contributor

Daenyth commented Aug 9, 2019

it means we have deprecation warnings, which means we either have to disable fatal warnings or move these instances to entirely separate modules

That's not the only option, perhaps. You could use the silencer library to annotate those specific objects and suppress the deprecation warnings.

@kailuowang
Copy link
Contributor

You could use the silencer library to annotate those specific objects and suppress the deprecation warnings.

Well, those instance really should be deprecated.

@travisbrown
Copy link
Contributor Author

Fixed in #2983.

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 a pull request may close this issue.

3 participants