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

Move standard library type class instances into implicit scope #3043

Merged

Conversation

@travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Sep 9, 2019

Please see this blog post for some additional discussion of this proposal.

This is a proof-of-concept and proposal more than a pull request (even a WIP one), but I don't know a better way to put it up for discussion in a place where changes can be tracked, etc. It's a pretty sweeping change to the way type class instances are provided in Cats, and I haven't talked about it yet in any substantive way with other Cats maintainers.

Motivation

The basic idea is that instead of type class instances for standard library types (e.g. Monad[Option]) living in packages that require users to import them (cats.instances.option._), these instances are included in the type class companion objects, where they are available to users in implicit scope, without imports.

This means that Cats users would only ever have to think about imports for syntax, not instances. For example, the following just works:

scala> cats.Parallel[Either[String, ?]].parProductR(Left("foo"))(Left("bar"))
res0: scala.util.Either[String,Nothing] = Left(foobar)

While in Cats now you'd need something like this:

import cats.instances.parallel._, cats.instances.string._

…or the "kitchen sink" cats.implicits._ import.

This might seem like a small thing, but I've been playing with the idea for a couple months now (starting with this attempt at porting Cats to Dotty), and I really do feel like it significantly improves my experience of working with Cats. It's just one less you have to worry about everywhere—in your source, in the REPL, etc.

What this PR does

For this initial experiment, I've added these type class instances to the appropriate companion objects as methods that point to the instances in the cats.instances packages. I've changed all code in cats-core, cats-free, and the tests so that there is no use of cats.instances imports, the cats.implicits._ import, or any instances brought into scope by the Instances traits.

I'm sure I've missed some instances, and we'd want much better tests before we'd ever consider merging this, but this change verifies that all of the standard library instances used in Cats itself are available in implicit scope, without imports.

The change is binary compatible with Cats 1.x and 2.x, and while I'm sure that it breaks source compatibility, I'd be surprised if it affects anything but fairly weird cases. You can still import the instances instances, and they'll override the implicit scope instances. It's just not necessary.

Longer term changes

This is entirely hypothetical, but if we decided to go this route in Cats 3 or some other future release, I can imagine a progression like the following:

  1. A major release (3.0.0) that does only what this PR does: introduces the implicit scope instances but doesn't remove anything.
  2. A minor release (3.1.0) that deprecates the instances traits and packages.
  3. A major release (4.0.0) that removes the instances traits and packages.

The initial step would be binary compatible with previous releases, but it has such a big effect on usage that I don't personally think introducing it in a minor release is a good idea.

Challenges

I've been using Scalaz for a few months short of a decade, and in that time it's always taken the approach that Cats inherited, requiring imports for standard library instances. I think this may have been different in earlier versions of Scalaz, but that's not clear from the history in the repo on GitHub, and I haven't done any further archeology yet. I've asked around about the reasons for this design decision a few times, and this thread includes the most context I know of.

The biggest immediate difficulty with putting these instances into implicit scope is the fact that the compiler only searches supertype companions for instances, not subtypes. This means that if you provide the MonadError instance for Option in the MonadError companion object, it won't be found in a search for a Functor[Option] instance.

As far as I can tell there are two viable approaches to making instances like this available for subtype searches:

  1. Put "upcast" instances in every companion object, so that if you'll always have a Functor[F] if you have a Monad[F] in implicit scope.
  2. Put the instances in the roots of the type class hierarchy, where they'll be always be found when searching supertype companions.

I've experimented with both, and so far the second feels like the better choice. It requires a little more up-front coordination, but with a diagram of the Cats type class hierarchy it's not too bad, and it's much less invasive.

Another potential difficulty involves type class hierarchies that cross module boundaries, as @non and @johnynek point out here. I think this is addressable, and I've done some experiments in that direction, but we'll definitely need to spend more time looking at how this change would impact real projects like cats-effect, Algebra, and Spire.

Compile times

This is the primary thing I'm worried about (see for example this article by Bill Venners for some context). I don't have strong intuitions about the relative compile-time cost of imported implicits vs. the kind of implicit scope search this change requires.

I've done a few experiments in this respect, including running the following several times on both master and this branch:

sbt clean test:clean
sbt compile
time sbt test:compile

And all of the results are within a couple seconds of each other (around 150 seconds on my machine). That's not very scientific, though, and I'm not sure how accurately the Cats test usage reflects the real world usage we care about.

Jar size

I was also originally a little worried that duplicating the instances would hurt jar sizes in a significant way, especially because in this initial attempt the code-gen for kernel instances isn't as optimized as it could be, and many of the generated instance definitions are duplicated. If we go this route, eventually this wouldn't be a concern at all, since we'd remove the instance packages altogether, but I was worried that in the meantime we'd be stuck with some bloat.

It turns out that this isn't really the case. Here are the cats-core sizes after this change:

5736    cats-core_2.11.jar
4556    cats-core_2.12.jar
4664    cats-core_2.13.jar

And on current master:

5704    cats-core_2.11.jar
4528    cats-core_2.12.jar
4632    cats-core_2.13.jar

So less than a percent in all cases (+0.71% for 2.13, which has the biggest change).

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 9, 2019

Codecov Report

Merging #3043 into master will decrease coverage by 1.36%.
The diff coverage is 40.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3043      +/-   ##
=========================================
- Coverage   93.46%   92.1%   -1.37%     
=========================================
  Files         368     368              
  Lines        6975    7153     +178     
  Branches      187     186       -1     
=========================================
+ Hits         6519    6588      +69     
- Misses        456     565     +109
Impacted Files Coverage Δ
core/src/main/scala/cats/data/NonEmptyList.scala 99.33% <ø> (ø) ⬆️
...src/main/scala/cats/syntax/unorderedFoldable.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/package.scala 88.88% <ø> (ø) ⬆️
core/src/main/scala/cats/Applicative.scala 95% <ø> (ø) ⬆️
core/src/main/scala/cats/Foldable.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/EitherT.scala 98.18% <ø> (ø) ⬆️
core/src/main/scala/cats/data/ZipVector.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/WriterT.scala 91.26% <ø> (ø) ⬆️
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 96.2% <ø> (ø) ⬆️
...estkit/src/main/scala/cats/tests/ListWrapper.scala 100% <ø> (ø) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dce008...fdaf9d3. Read the comment docs.

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 9, 2019

Codecov Report

Merging #3043 into master will decrease coverage by 2.09%.
The diff coverage is 27.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
- Coverage   93.33%   91.23%   -2.10%     
==========================================
  Files         378      379       +1     
  Lines        7723     7974     +251     
  Branches      218      217       -1     
==========================================
+ Hits         7208     7275      +67     
- Misses        515      699     +184     
Flag Coverage Δ
#scala_version_212 91.20% <27.55%> (-2.19%) ⬇️
#scala_version_213 91.01% <27.55%> (-2.10%) ⬇️
Impacted Files Coverage Δ
core/src/main/scala/cats/Applicative.scala 95.00% <ø> (ø)
core/src/main/scala/cats/Bifunctor.scala 83.33% <0.00%> (-16.67%) ⬇️
core/src/main/scala/cats/Defer.scala 0.00% <0.00%> (ø)
core/src/main/scala/cats/Foldable.scala 92.59% <ø> (ø)
core/src/main/scala/cats/Parallel.scala 81.15% <0.00%> (-3.69%) ⬇️
core/src/main/scala/cats/arrow/Compose.scala 50.00% <0.00%> (-16.67%) ⬇️
core/src/main/scala/cats/arrow/Profunctor.scala 75.00% <0.00%> (-25.00%) ⬇️
core/src/main/scala/cats/data/EitherT.scala 98.50% <ø> (ø)
core/src/main/scala/cats/data/NonEmptyList.scala 98.73% <ø> (ø)
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 97.50% <ø> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c494157...b9b4afb. Read the comment docs.

Loading

@milessabin
Copy link
Member

@milessabin milessabin commented Sep 9, 2019

I'm very much in favour of moving instances to the implicit scope whenever possible. Imports make prioritization much harder.

Loading

@mpilquist
Copy link
Member

@mpilquist mpilquist commented Sep 9, 2019

Big 👍 from me.

Loading

@neko-kai
Copy link

@neko-kai neko-kai commented Sep 9, 2019

@travisbrown @milessabin @mpilquist

Another potential difficulty involves type class hierarchies that cross module boundaries, as @non and @johnynek point out here. I think this is addressable, and I've done some experiments in that direction, but we'll definitely need to spend more time looking at how this change would impact real projects like cats-effect, Algebra, and Spire.

With regards to supporting downstream type class hierarchies without imports, previously I dicovered a scala-2 and dotty-compatible way to create non-orphan type class instances for libraries that are NOT dependencies, https://blog.7mind.io/no-more-orphans.html, it can probably also work for libraries that are downstream and create a cycle.

I do not propose to create cross-repository cycles however, instead I propose a linking trick inspired by scalac/dotty permissive linking properties found in above exploration, in essence:

package mycats

import mycats.effect.CatsEffectFunctorInstances
import mycats.mtl.CatsMtlFunctorInstances

trait Extensions[Ext]

trait Functor[F[_]] extends Extensions[(
  CatsEffectFunctorInstances,
  CatsMtlFunctorInstances,
)]

Fictional library in repo that is NOT PUBLISHED:

package mycats.effect

trait CatsEffectFunctorInstances
///
package mycats.mtl

trait CatsMtlFunctorInstances

Implementation in a separate downstream library:

package mycats.effect

import mycats.Functor

trait Sync[F[_]] extends Functor[F]

sealed trait CatsEffectFunctorInstances
object CatsEffectFunctorInstances {
  implicit val syncList: Sync[List] = new Sync[List] {}
}

Summoning Functor[List] for a user of this library now works without any imports whatsoever, the type and runtime behavior is exactly as it should be:

def the[A <: AnyRef](implicit a: A): a.type = a
val isSync: Sync[List] = the[Functor[List]]
println(isSync)
// mycats.effect.CatsEffectFunctorInstances$$anon$1@7abbd2fa

Remember that companions of type parameters and their superclasses are searched recursively, so if 22 tuple size limit is exceeded for extensions you can just nest more tuples. Downstream libraries can extend their own extensions points with more extension points too.

Usage WITHOUT a downstream library is also completely fine, because non-existent symbols are erased and ignored by both scalac & dotty:

println(new Functor[Option] {})
// test.TestClassNonbroken$$anon$1@76e09de1

The implementation formatted as sbt project is here – https://github.com/7mind/no-more-orphans/pull/1/files (branch) – if you want to test how it works.

The management of these extension points must be done in a centralized way in cats repository, but new extension points can be added in minor releases without breaking bincompat – the extension points themselves are completely erased from bytecode.
I don't think there's a way to have a completely decentralized extensible implicit scope – and it's probably not desirable anyway! However, this approach creates an infinite amount of extension points that can be allocated ahead of time and further extended in downstream libraries.

Loading

@milessabin
Copy link
Member

@milessabin milessabin commented Sep 9, 2019

@neko-kai that's a nice trick, but I confess that relying on linker magic to solve a semantic problem makes me a little uneasy.

Loading

@milessabin
Copy link
Member

@milessabin milessabin commented Sep 9, 2019

FWIW, I think that mechanism is doing more or less the same work as the @imports annotation in (now deprecated) export-hook.

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Sep 9, 2019

Thanks, @neko-kai! I hadn't seen that blog post, and I like the technique—I'm wondering now if there's some way we could have used the idea in circe-generic and avoided the necessarily-macro-provided exported stuff.

The question of where to put instances for standard library types in a library that depends on Cats and extends Cats type classes doesn't seem likely to need anything quite that clever, though. We'll want to work through it and make sure we can make everything consistent with the approach in Cats itself, but I'm hoping that will be a fairly mechanical exercise.

Loading

@jackcviers
Copy link

@jackcviers jackcviers commented Sep 9, 2019

This gets a big 👍 from me.

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Sep 10, 2019

Since this is marked WIP and is likely to be open for a while, I'd like to keep it up-to-date occasionally by rebasing instead of merging master, to keep the history from becoming a mess. If anyone strongly objects to that please let me know.

Loading

@kubukoz
Copy link
Member

@kubukoz kubukoz commented Sep 11, 2019

Just FYI, on 2.13, there's now -Yimports that allows you to define cats.implicits or cats.instances.all as the default import and everything from it will end up in scope. Probably doesn't interest 2.12 users and it might not be supported in IDEs for a while, but it's good to know.

Loading

@travisbrown travisbrown force-pushed the topic/implicit-scopification branch 3 times, most recently from f70b791 to f76c95e Sep 15, 2019
@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Sep 30, 2019

I've published a blog post with some more background and some updated compile-time measurements here.

Loading

@neko-kai
Copy link

@neko-kai neko-kai commented Sep 30, 2019

If the change is binary compatible, is there a specific reason to wait for cats-3.0? Could this be merged wholesale as part of a minor release, instead? 🤔

Loading

@Krever
Copy link

@Krever Krever commented Sep 30, 2019

Nice! One (potentially huge) advantage of this is Intellij performance. import cats.implicits._ makes it basically unusable for me and so I have to use syntax._ + ala carte instances. With this change it should work just as with implicits._ but much faster.
Wondering if there is a way to measure this.

Loading

@travisbrown travisbrown force-pushed the topic/implicit-scopification branch from 196605f to 6071501 Oct 10, 2019
@djspiewak
Copy link
Member

@djspiewak djspiewak commented Oct 10, 2019

As a random warning regarding @neko-kai's technique, it's really really cool, but optional dependencies play havoc with eviction detection. If this kind of thing becomes widespread, we're going to have a lot of silent diamond dependency problems in downstream projects due to the fact that conflicting dependencies that are only accessed via optional pathways are not detected. Maybe that's something we can push for fixes in sbt at the least, but it's still a point of concern. (edit: my warning is still true, but it appears that Kai's trick depends more upon phantom type components than on optional dependencies, so honestly I think it should scale reasonably well even if everyone starts using it; the trick is that the extension points are fixed, but this seems like something that will resolve itself via convention)

Regarding the OP proposal… I think the "put the stdlib stuff into an imported scope" concept comes from Scalaz 7, and more specifically the push at the time to modularize the hierarchy. Scalaz 6 (and prior versions) was more or less an "all or nothing" type library. You imported Scalaz._ and that was almost the only way to meaningfully use the framework. In Scalaz 7, a lot of work was done (and a lot of boilerplate was taken on) to split things apart to allow the a la carte import approach to be possible. Part of that was pulling the stdlib instances out of companions (where they had been previously housed) and into explicit imports. This also made instance locations a bit more uniform, since it avoided putting any non-inductive instances on class companions, which has some aesthetic appeal.

I think that particular design decision has run its course though, and particularly since the trend has been to move away from a la carte imports (at least for instances and syntax), it seems fair to revisit the design ideals of yesteryear. Not that I'm saying we should completely do away with support for a la carte access, but rather that it is fair to privilege the stdlib instances by putting them on class companions. So 👍 from me.

Loading

@djspiewak
Copy link
Member

@djspiewak djspiewak commented Jan 1, 2020

@travisbrown This feels like it has matriculated a bit. What's the latest on it? Do you feel it's ready to potentially move into master? You've definitely done the due diligence on all the metrics I can think of. So long as binary compatibility is satisfied, I'm 👍 on having this in 2.2.0.

For a change this large, I feel like we should get a quorum of a few more than usual the number of sign-offs. If you feel the PR is ready to be non-WIP, I'll give an official Approval.

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Jan 1, 2020

@djspiewak Thanks! I was planning to set up some automated checking this week to make sure all of the instances in cats.instances are included in the companion objects, but I have been thinking that we should try to get this into 2.2.0. I don't think this should take more than a couple of hours tomorrow or Friday.

Loading

@LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Jan 2, 2020

I'm also convinced we should move forward with this 👍 We should definitely go through a milestone/release candidate process, since this is quite a fundamental change. I also don't have issues just calling it 3.0 at that point and make 3.0 a fully backwards compatible release, similar to how 2.0 was also completely binary compatible (except for cats-laws). WDYT?

Loading

@neko-kai
Copy link

@neko-kai neko-kai commented Jan 2, 2020

@LukaJCB There was still some breakage in 2.0, even if in one module. All code that uses explicit instance imports should probably continue to compile because imported instances have much higher priority than instance from companion object, so I expect source breakage to also be minimal. As such, I don't see a big reason for naming it 3.0 if full binary compatibility is maintained...

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 2, 2020

Whatever is called 3.0 will have big ramifications through the ecosystem, either triggering rereleases of everything, or spending hours on Gitter telling people not to worry about it. This seems firmly 2.2 to me.

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Jan 2, 2020

@LukaJCB I don't feel too strongly either way about whether to call the next release 2.2 or 3.0, but have a slight preference for 2.2, for a few reasons.

  • It will be 100% binary-compatible and source-compatible. With 2.0 we were signaling some significant source-compatibility breakage in cats-core (especially Parallel), but I don't know of any incoming changes like that.
  • Personally I'd like to see a fairly quick deprecation cycle here: 2.2.0 includes this change, 2.3.0 deprecates cats.instances, and 3.0.0 (late 2020 or early 2021?) is a big breaking release that cleans up both the deprecated instances and the years of bincompat cruft we've accumulated. We could do that as 3.0.0, 3.1.0, and 4.0.0, but that compresses our current ~yearly major release cadence (also it feels a little weird to me to have major deprecations planned for a x.1.0).
  • We will need a breaking release at some point (for easier maintenance, smaller jars, faster compilation for both Cats itself and downstream projects, etc.), and in my view it'd be best to have a consistent story about the relationship between bincompat guarantees and versions. Saying that 2.x was a special case for cats-core and that subsequent major version releases are breaking seems easier than having to communicate that 1, 2, and 3 are backwards bincompat but 4 isn't, etc.

Loading

@LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Jan 2, 2020

You all make good points, I'm fine with 2.2.0 :)

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Jan 14, 2020

Finally getting back to this again. I've just pushed a small change that does two things:

  • There were some Show instances that were redefined in the companion object instead of just pointing to the cats.instances instances like everything else. I've changed these for consistency.
  • There were a few companion object instances defined as implicit val, which results in a field and an accessor method, even though the values are already instantiated in cats.instances. I've changed these to implicit def both for consistency and for efficiency.

Loading

@travisbrown travisbrown mentioned this pull request Jan 21, 2020
@travisbrown travisbrown removed this from the 2.2.0-M1 milestone Feb 24, 2020
@travisbrown travisbrown added this to the 2.2.0-M1 milestone Feb 27, 2020
@travisbrown travisbrown changed the title Move standard library type class instances into implicit scope [WIP] Move standard library type class instances into implicit scope Feb 27, 2020
@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Feb 27, 2020

@mrdziuban was just asking about the status of this PR on Gitter:

thanks, having everything in implicit scope would be a great motivator for me to finish migrating everything to cats. are the comments on #3043 to be believed, it might make it into 2.2.0?

I just merged master again and removed the [WIP] from the title—after the couple of small fixes I made last week I think it's ready to merge and be included in a 2.2.0-M1 release.

We'll probably want to spend a little more time than usual in the 2.2.0 milestones before 2.2.0 final, and we'll definitely put a louder warning than usual in the milestone release notes, but its been five months, I've tested this in a number of places, and a lot of people have looked at it, and I don't think we've come up with any reason not to move forward with it.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

I tried this one locally on a couple large projects and didn't hit any source incompatibilities. As long as we roll it out in a milestone where weird issues can surface, I think it's a positive change.

Loading

@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Mar 25, 2020

Thanks @rossabaker! If there are no objections I'm going to rebase this branch before merging since the history is a mess. I've split the changes up into two commits, one that makes the change itself, and one that removes cats.instances from the tests The tests pass on both, which is more evidence for full source compatibility. The branch is here, and I'll update this PR later today: https://github.com/travisbrown/cats/tree/topic/implicit-scopification-again

Loading

@travisbrown travisbrown dismissed stale reviews from rossabaker and djspiewak via 3d89018 Mar 25, 2020
@travisbrown travisbrown force-pushed the topic/implicit-scopification branch from acdeb03 to 3d89018 Mar 25, 2020
@travisbrown
Copy link
Contributor Author

@travisbrown travisbrown commented Mar 28, 2020

Test failure was unrelated, restarting.

[info] Compiling 2 Scala sources to /home/travis/build/typelevel/cats/kernel-laws/jvm/target/scala-2.12/test-classes ...
[info] Done compiling.
[info] Tests:
failing seed for lowerBounded.antisymmetry is h2SBKFLv0rP3D3K7LRblPGAp8NtKZ77GB8PKki9DB3M=
[info] - LowerBounded[FiniteDuration].lowerBounded.antisymmetry *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (LawTests.scala:215)
[info]     Falsified after 80 successful property evaluations.
[info]     Location: (LawTests.scala:215)
[info]     Occurred when passed generated values (
[info]       arg0 = -130 days,
[info]       arg1 = -3120 hours,
[info]       arg2 = org.scalacheck.GenArities$$Lambda$528/620321705@983d98a
[info]     )
[info]     Label of failing property:
[info]       Expected: true
[info]   Received: false
[info] ScalaTest

Loading

@travisbrown travisbrown force-pushed the topic/implicit-scopification branch from 3d89018 to b9b4afb Mar 28, 2020
Copy link
Member

@rossabaker rossabaker left a comment

👍 after rebase

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet