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

Embed cats.syntax.all._ into IO. #3532

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

kamilkloch
Copy link
Contributor

@kamilkloch kamilkloch commented Apr 13, 2023

Work on #3531.

@kamilkloch kamilkloch closed this Apr 13, 2023
@kamilkloch kamilkloch reopened this Apr 13, 2023
@armanbilge armanbilge marked this pull request as draft April 13, 2023 15:37
@djspiewak
Copy link
Member

Wow. Lol. This is quite a lot. I think it's extremely worth doing though. The unfortunate tradeoff is it significantly pollutes the IO. namespace, albeit with names which are likely to be immediately ignored in any autocomplete scenario. The advantage we gain from being able to avoid import cats.syntax.all._ in common cases is immense.

@kamilkloch
Copy link
Contributor Author

It might be worth the effort to go through the imported syntax and ruthlessly drop any distractions - as an example, I do not find any meaningful extensions in Applicative, Apply, FlatMap, Functor. And in the end - cats.implicits._ will prevail either way (traverse ;) )

@armanbilge
Copy link
Member

And in the end - cats.implicits._ will prevail either way

FYI cats.syntax.all._ should be preferred; cats.implicits._ is obsolete and de facto (soft-)deprecated.

@kamilkloch
Copy link
Contributor Author

kamilkloch commented Apr 18, 2023

I went method by method through the proposed extensions. My belief is that we should not go the (easiest) generic way - the majority of the methods is (arguably) not needed at all and only adds polution to the namespace. Additionally, the syntax methods do not contain any documentation, and we do need scaladoc for something like parUnorderedSequence.

To make the discussion concrete, take a look at the tables below.
For each extension the corresponding table enumerates all the methods and indicates if the method is already present in IO, and if if it is (subjectively) nice to have.

MonadErrorSyntax

Method in IO nice to have
ensure
ensureOr
reject
adaptError ✔️
redeemWith ✔️ ✔️
attemptTap ✔️
rethrow ✔️ ✔️

FoldableSyntax

Method in IO nice to have
sequence_ ✔️
foldK
foldl
foldr
foldA
foldSmash
mkString_
collectFirstSomeM
findM
collectFold
foldMapK
partitionBifold
partitionBifoldM
partitionEitherM
sliding

UnorderedFoldableSyntax

Method in IO nice to have
contains_
count

TraverseSyntax

Method in IO nice to have
traverse ❌ not possible
traverseTap
flatTraverse
sequence ✔️
flatSequence ✔️
mapAccumulate
mapWithIndex
traverseWithIndexM
zipWithIndex
zipWithLongIndex
traverseWithLongIndexM
mapWithLongIndex
updated_

UnorderedTraverseSyntax

Method in IO nice to have
unorderedTraverse ❌ not possible
unorderedSequence ❌ not possible

TraverseFilterSyntax

Method in IO nice to have
sequenceFilter
traverseCollect

TupleParallelSyntax

Method in IO nice to have
parMapN ✔️
parTupled ✔️
parFlatMapN ✔️

ParallelSyntax

Method in IO nice to have
parTraverse ❌ not possible
parSequence ✔️
parReplicateA ✔️
parReplicateA_ ✔️
&> ✔️ ✔️
<& ✔️ ✔️
parProductL ✔️
parProductR ✔️
parProduct ✔️

ParallelFoldMapASyntax

Method in IO nice to have
parFoldMapA ❌ not possible

ParallelFlatSyntax

Method in IO nice to have
parFlatTraverse ❌ not possible
parFlatSequence ✔️

ParallelTraverseSyntax

Method in IO nice to have
parTraverse_ ❌ not possible
parSequence_ ✔️

ParallelUnorderedTraverseSyntax

Method in IO nice to have
parUnorderedTraverse ❌ not possible
parUnorderedFlatTraverse ❌ not possible
parUnorderedSequence ✔️
parUnorderedFlatSequence ✔️

ParallelTraverseFilterSyntax

Method in IO nice to have
parTraverseFilter ❌ not possible
parFilterA ❌ not possible
parSequenceFilter ✔️

cats.effect.syntax.AllSyntax - done (all methods are already in IO)

@alexandru
Copy link
Member

Hi all,

I hope I'm not misunderstanding what the PR does, but I looked at the tests. Here's my 2 cents ...

I think this adds a lot of unnatural garbage to IO's API, also generating confusion. Because people that expect this syntax also know to import cats.syntax.all (and one obvious question here is if there's any possibility for conflicts).

(A, B).parMapN { ... }

List(A, B).parSequence

This syntax isn't natural for Scala. We only live with it, for the most part, because there are no better options for generic code, like a syntax meant for applicatives. And the problem is that it's not easy to discover. And the IDE has to struggle to suggests those methods regardless of where they comes from. If IntelliJ IDEA can do it, that's only a testament to how smart it is, given the implicit conversions going on for those ops.

But when working with IO directly, it isn't generic, which is good, as it removes confusion in some cases. And the natural syntax for both of those cases is this one:

IO.parSequence(List(A, B))

IO.parMapN(A, B) { ... }

I.e., functions operating on more that 1 object go in the companion object (static methods in Java). That's where people look. This syntax also has prior art, including Future.sequence.

And personally I'd really like IO.parSequence precisely to avoid Cats' syntax, whenever I can, because I just had a bug related to tuples implementing Foldable in a totally awkward way.

@kamilkloch
Copy link
Contributor Author

@alexandru let me try to clarify.

The main goal is to fulfill the following statement: "average user shall not need to import any magic implicits for typical IO operations". Personally, I am willing to die on this hill ;) Magical extensions through imports are really bad:

  • they are beginners-unfriendly
  • they pollute the namespace with (arguably) esoteric cats stuff
  • they (significantly) slow down and confuse the IDE (I can speak for IJ, where adding cats.implicits._ used to freeze the whole IDE)

This migration away from the imported implicits has already started - IO natively implements cats.effect.syntax.AllSyntax - nobody should have to import anything for IO#recover, for example.

It is my belief that enabling the following:

(IO(1), IO(2)).parMapN(...)
(IO(1), IO(2)).parTupled
List(IO(1), IO(1)).sequence
List(IO(1), IO(1)).parSequence

will be of big benefit for every user. Agreed, we shall add the IO.sequence, IO.traverse etc. variations as well.

To sum up - add a carefully chosen list of extension methods, with simplified signatures and a menaningful scaladoc.

@djspiewak
Copy link
Member

@kamilkloch Your tables are amazing thank you!

@alexandru Overall, I see the point of this PR as "improve ergonomics". I don't want to create a lot of redundant ways of doing the same thing, ideally. Instead, I'd like to first reduce the friction and improve the discoverability of the approaches we already have. There are kind of three layers to this, starting from least to most disruptive:

  1. For syntax which is directly on F[_] where F = IO, make it a method on IO itself (e.g. attemptTap). These implementation should trivially delegate to the typeclass implementation in Cats to avoid redundancy.
  2. For syntax where IO is involved in the type and users think of it as being "an IO thing" (e.g. parMapN), but it isn't directly on IO, pull the implicit enrichments into the IO companion so that users don't need to import cats.syntax.all._ for common cases
  3. For syntax where IO is inferred (and not directly involved in the resolution) but people have a hard time discovering the implicitly enriched form, consider creating redundant mechanisms on the IO companion (traverse and its relatives are basically it here)

Category 3 is where you start creating redundancy, and I'm very leery about it. Let's do the first two first and then decide if we still really want the third one.

@alexandru
Copy link
Member

alexandru commented Apr 20, 2023

For syntax where IO is involved in the type and users think of it as being "an IO thing" (e.g. parMapN), but it isn't directly on IO, pull the implicit enrichments into the IO companion so that users don't need to import cats.syntax.all._ for common cases

If users find about parMapN, and they find that they have to do (io1, io2).parMapN { ... }, won't they also know to import cats.syntax.all._?

This syntax isn't discoverable by typing . either on IO objects or their companion. And for those missing the import, IntelliJ IDEA is almost smart enough to suggest it.

So, what category of users is this helping?

UPDATE: and I think we should also worry about the performance of the compiler. Finding and sorting out those implicits isn't easy. What if all monadic types in the Typelevel ecosystem did this?

@alexandru
Copy link
Member

alexandru commented Apr 20, 2023

BTW, Scala 3 is also smart enough to suggest an import, it seems:


Screenshot 2023-04-20 at 11 33 23

~ % scala-cli repl --dep "org.typelevel::cats-effect::3.4.9" --scala 3.2.2
Welcome to Scala 3.2.2 (17.0.4.1, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> import cats.effect._

scala> (IO(1), IO(2)).parMapN(_ + _)
-- [E008] Not Found Error: -----------------------------------------------------
1 |(IO(1), IO(2)).parMapN(_ + _)
  |^^^^^^^^^^^^^^^^^^^^^^
  |value parMapN is not a member of (cats.effect.IO[Int], cats.effect.IO[Int]), but could be made available as an extension method.
  |
  |One of the following imports might fix the problem:
  |
  |  import cats.implicits.catsSyntaxTuple2Parallel
  |  import cats.syntax.all.catsSyntaxTuple2Parallel
  |  import cats.syntax.parallel.catsSyntaxTuple2Parallel
  |
1 error found

@kamilkloch
Copy link
Contributor Author

kamilkloch commented Apr 20, 2023

If users find about parMapN, and they find that they have to do (io1, io2).parMapN { ... }, won't they also know to import cats.syntax.all._?

No :), and that is a fact. Number one recurring question is the missing import.

This syntax isn't discoverable by typing . either on IO objects or their companion. And for those missing the import, IntelliJ IDEA is almost smart enough to suggest it.

The syntax will be perfectably discoverable when typing (IO(1), IO(2))., that is the whole point.

So, what category of users is this helping?

Almost all users, in almost all use-cases. Please take a look at the tables above and see for yourserlf.

UPDATE: and I think we should also worry about the performance of the compiler.

I do not get it - what kind of perforemance overhead, if we add a handful of methods directly to IO and and one or two extension classes? The real performance problem is the import cats.syntax.all._

@alexandru
Copy link
Member

alexandru commented Apr 20, 2023

If users find about parMapN, and they find that they have to do (io1, io2).parMapN { ... }, won't they also know to import cats.syntax.all._?

No :), and that is a fact. Number one recurring question is the missing import.

If that indeed happens, and I'd love to see some data on it, then it misses the forest from the trees, as the problem is the … freakishly odd syntax. Which also appears to be solvable via smarter tooling, like the Scala 3 compiler, or IntelliJ IDEA.

The syntax will be perfectably discoverable when typing (IO(1), IO(2))., that is the whole point.

How do people know to construct a tuple when operating on 2 IO values, thinking they'd find an extension method for it?
What thought process, or documentation, or culture can lead them there?

I don't think I've ever seen this in any other ecosystem. The only infamous example that comes close is ", ".join(list) from Python, and everyone hates it precisely because the expression starts with the wrong noun.

I do not get it - what kind of perforemance overhead, if we add a handful of methods directly to IO and and one or two extension classes? The real performance problem is the import cats.syntax.all._

People working in the Typelevel ecosystem won't get rid of import cats.syntax.all, and the compiler will have the extra job of figuring out priorities for what are, essentially, duplicated definitions and whether they are in conflict or not.

Duplicating the extension methods, that work via implicit conversions & higher-kinded types, for the entire functor hierarchy, plus Traverse/Parallel, doesn't seem light at all, sorry if I'm misreading this:

    with cats.syntax.MonadErrorSyntax
    with cats.syntax.FoldableSyntax
    with cats.syntax.UnorderedFoldableSyntax
    with cats.syntax.TraverseSyntax
    with cats.syntax.UnorderedTraverseSyntax
    with cats.syntax.TraverseFilterSyntax
    with cats.syntax.ParallelSyntax
    with cats.syntax.ParallelFoldMapASyntax
    with cats.syntax.ParallelFlatSyntax
    with cats.syntax.ParallelTraverseSyntax
    with cats.syntax.ParallelUnorderedTraverseSyntax
    with cats.syntax.ParallelTraverseFilterSyntax
    with cats.effect.syntax.AllSyntax

@kamilkloch
Copy link
Contributor Author

Duplicating the extension methods, that work via implicit conversions & higher-kinded types, for the entire functor hierarchy, plus Traverse/Parallel, doesn't seem light at all, sorry if I'm misreading this:

    with cats.syntax.MonadErrorSyntax
    with cats.syntax.FoldableSyntax
    with cats.syntax.UnorderedFoldableSyntax
    with cats.syntax.TraverseSyntax
    with cats.syntax.UnorderedTraverseSyntax
    with cats.syntax.TraverseFilterSyntax
    with cats.syntax.ParallelSyntax
    with cats.syntax.ParallelFoldMapASyntax
    with cats.syntax.ParallelFlatSyntax
    with cats.syntax.ParallelTraverseSyntax
    with cats.syntax.ParallelUnorderedTraverseSyntax
    with cats.syntax.ParallelTraverseFilterSyntax
    with cats.effect.syntax.AllSyntax

Let me update the code and we shall talk it over.

@alexandru
Copy link
Member

PS: I've said my 2 cents, and I don't think I can add more 🙂 so I'll stop now 😜

Sorry for bikeshedding, keep up the good work.

@alexandru
Copy link
Member

alexandru commented Apr 20, 2023

@kamilkloch Also, I agree that the work you've put in that table is wonderful 🙂

@djspiewak
Copy link
Member

Quick note: I'm pretty sure that this technique will have essentially zero impact on compile times. Happy to be proven wrong, but the compiler fixed implicit search performance in these types of cases a long time ago. The imports are far worse.

Regarding the weird syntax, I agree to an extent, but there are only bad answers here if we want to add additional forms of the same thing.

@armanbilge armanbilge added this to the v3.6.0 milestone May 25, 2023
@kamilkloch kamilkloch marked this pull request as ready for review September 19, 2023 13:08
@kamilkloch kamilkloch changed the title Add IOOps from cats.implicits. Embed cats.syntax.all._ into IO. Sep 19, 2023
@djspiewak
Copy link
Member

Okay bit of bikeshedding here…

I'm still on the fence about IO.traverse and friends. The discoverability is a lot better in a very important case, which is the only reason why I would even consider it, but it's also breaking the "one way to do things" guideline. I know where @alexandru stands on this. :D I'd love to hear from @mpilquist and other folks who generally prefer not creating this type of redundancy.

@kamilkloch Amazing work on this. IMO it's ready to go aside from this last bit of quibbling.

@kamilkloch
Copy link
Contributor Author

kamilkloch commented Nov 23, 2023

I'm still on the fence about IO.traverse and friends. The discoverability is a lot better in a very important case, which is the only reason why I would even consider it, but it's also breaking the "one way to do things" guideline.

For traverse the only other way to make it work == via the cats.syntax.all._ import, right? But with this import all the added methods are rendered unnecessary as well :)

Also, IO.parSequenceN is (arguably) already there in the companion object, despite the fact that List(IO(1)).parSequence is the canonic way to write it ;)

EDIT: List(1, 2).traverse works only with the import, whereas List(IO(1)).[par]Sequence[N] works via extension methods, same for IO(1).[par]Replicate[N]. Perhaps remove all sequence / replicate flavors from the companion object altogether?

Reuse existing syntax

Update TestIOOps.

Revert back to cherry-picked implementation, add missing methods.

Add missing header.

Add missing implementation.

Remove unused import.

Revert to Scala 2 import syntax.

Reuse existing extension classes.

Remove IO.parReplicateA, IOparReplicateA_.

Add tests for adaptError and attemptTap.

Remove TestIOOps.

Cleanup.

scalafmt.

Add tests.

Update tests.

Fix type constraints.

Add IO.traverse and IO.traverse_.

Reorganize tests.
@kamilkloch kamilkloch force-pushed the io-implicits branch 2 times, most recently from 5dee140 to 7cc6d71 Compare January 2, 2024 15:31
@djspiewak djspiewak merged commit df63f8b into typelevel:series/3.x Mar 5, 2024
62 of 66 checks passed
@djspiewak
Copy link
Member

Thanks for sticking with this through the whole saga, @kamilkloch! I think this'll be a great set of changes.

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.

4 participants