Move standard library type class instances into implicit scope #3043
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
I'm very much in favour of moving instances to the implicit scope whenever possible. Imports make prioritization much harder. |
|
Big 👍 from me. |
|
@travisbrown @milessabin @mpilquist
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 CatsMtlFunctorInstancesImplementation 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@7abbd2faRemember 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@76e09de1The 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. |
|
@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. |
|
FWIW, I think that mechanism is doing more or less the same work as the |
|
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 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. |
|
This gets a big 👍 from me. |
|
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. |
|
Just FYI, on 2.13, there's now |
travisbrown
force-pushed
the
topic/implicit-scopification
branch
3 times, most recently
from
f70b791
to
f76c95e
Compare
|
I've published a blog post with some more background and some updated compile-time measurements here. |
|
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? 🤔 |
|
Nice! One (potentially huge) advantage of this is Intellij performance. |
travisbrown
force-pushed
the
topic/implicit-scopification
branch
from
196605f
to
6071501
Compare
|
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 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. |
travisbrown
force-pushed
the
topic/implicit-scopification
branch
from
90c196a
to
1d732de
Compare
|
@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. |
|
@djspiewak Thanks! I was planning to set up some automated checking this week to make sure all of the instances in |
|
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? |
|
@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... |
|
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. |
|
@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.
|
|
You all make good points, I'm fine with 2.2.0 :) |
|
Finally getting back to this again. I've just pushed a small change that does two things:
|
travisbrown
changed the title
|
@mrdziuban was just asking about the status of this PR on Gitter:
I just merged master again and removed the 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. |
|
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 |
travisbrown
force-pushed
the
topic/implicit-scopification
branch
from
acdeb03
to
3d89018
Compare
|
Test failure was unrelated, restarting. |
travisbrown
force-pushed
the
topic/implicit-scopification
branch
from
3d89018
to
b9b4afb
Compare
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:
While in Cats now you'd need something like this:
…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.instancespackages. I've changed all code in cats-core, cats-free, and the tests so that there is no use ofcats.instancesimports, thecats.implicits._import, or any instances brought into scope by theInstancestraits.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
instancesinstances, 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:
instancestraits and packages.instancestraits 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
MonadErrorinstance forOptionin theMonadErrorcompanion object, it won't be found in a search for aFunctor[Option]instance.As far as I can tell there are two viable approaches to making instances like this available for subtype searches:
Functor[F]if you have aMonad[F]in implicit scope.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:compileAnd 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:
And on current master:
So less than a percent in all cases (+0.71% for 2.13, which has the biggest change).