cats foldable syntax unnecessarity has Foldable implicit in its signature

[SimY4]

when you import:

cats.syntax.foldable._

You got two methods which look very similar in terms in what they do:

.foldMap[B](f: A => B)(implicit B: Monoid[B]): B
.collectFold[B](f: A => B)(implicit F: Foldable[F], M: Monoid[M]): B

In case of a second one, it captures Foldable at the point of invocation not necessarily. Because Foldable typeclass was already established prior to this point. It also makes providing monoid instance explicitly harder because of additional Foldable instance that I have also supply.

[JosephMoniz]

I think either I might be interpreting the code in master wrong or I'm having a hard time understanding the issue. @SimY4 could you please link to the problematic lines of source more explicitly?

[SimY4]

@JosephMoniz There are two sources for foldable syntax. One is generated by simulacrum here:

https://github.com/typelevel/cats/blob/61a4a4641f084e8ad8d9fd5a52aed71b5022250d/core/src/main/scala/cats/Foldable.scala#L924-L940

The other one is custom:

https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/syntax/foldable.scala#L9

In custom one foldable evidence is provided at the moment of foldable syntax lookup as well as on each method which seems unnecessary

[JosephMoniz]

ah, I see it now.

From a first glance, it would appear to be the case that the reason for not capturing the foldable instance on syntax lookup would be to preserve the AnyVal semantics of the FoldableOps enhancement class. Adding an additional capture would cause AnyVal to break and boxing to always happen.

The tradeoff here is ergonomics vs allocation minimization.

In this case though, if the optimization isn't worth simulacrum's time, it probably shouldn't be worth our team to hand roll it in.

I would +1 doing something similar to what simulacrum is doing instead.

[joroKr21]

To be honest I always wondered if that is better or worse. What if the instance itself incurs allocations (implicit def), wouldn't we then allocate it twice?

[kubukoz]

What if the instance itself incurs allocations (implicit def), wouldn't we then allocate it twice?

@joroKr21 we will do that regardless of whether the constraint is on the class or on the method - call that def as many times as many extension-method-calls are made.

To give my 3 cents, I would aim for constraints on methods in the wrapping class, because it's easier to solve a missing implicit F: Foldable[F] than method foldMap not found.

[joroKr21]

@kubukoz I mean that for each call we will allocate Foldable (if it's an implicit def) twice rather than once:

catsSyntaxFoldOps(foo)(foldable1).fold(foldable2, monoid)

And I tend to agree that a missing implicit error is easier to solve. On the other hand it will cause some pollution because you will have those methods available for any F[A] but I think that is the lesser evil.

[kubukoz]

@kubukoz I mean that for each call we will allocate Foldable (if it's an implicit def) twice rather than once:

catsSyntaxFoldOps(foo)(foldable1).fold(foldable2, monoid)

Oh, I see. Yeah, that'd be twice and it's not cool

And I tend to agree that a missing implicit error is easier to solve. On the other hand it will cause some pollution because you will have those methods available for any F[A] but I think that is the lesser evil.

I suppose this can be avoided by using selective syntax imports, which many IDEA users do (not as many Metals users AFAIK, personally I have seen the performance impact is very low and it doesn't hurt as much to see more methods to choose)