Replace type class composition with Nested data type

[adelbertc]

We currently use cats.data.Prod for product composition -
for consistency, and for reasons outlined in #968, I think
we should use a cats.data.Compose data type for nested
composition.

I did the usual type classes first. No tests are added
intentionally, putting this up as a proof of concept. If
nobody yells at me for a couple of days I'll assume everything
is OK and I'll clean things up, add comments, tests, etc.

EDIT: I also realized we have cats.arrow.Compose.. what do we want to do about that? (assuming we want this to move forward)

EDIT2: Maybe we call it Nested.. like nested composition.. and Prod for product composition..

[adelbertc]

Tests not passing is expected since I removed a bunch of stuff, I'll add them assuming folks are OK with this proof of concept.

[ceedubs]

I don't have a super strong opinion on this, but I think that I like it. I'd be interested to see some real-worldish examples of using it. For example how verbose do you need to be to use it with F[_, _]-shaped types? Is it much different in that regard than the previous approach?

I admit that I pretty much never use type class compose methods in their current form since it always seems to be more verbose than just making nested calls.

[adelbertc]

So the reasons I've wanted this in in contexts where I'm working with a nested structure and I'm doing something like traverse with it, I either have to pass in the instance explicitly (e.g. foo.traverse(blah)(Applicative[F].compose[G])) or I have to declare a local implicit. Passing it in explicitly is straightforward in the simple case, but I recall working with methods that call methods that require constraints, or some fancier case where passing it explicitly becomes less wieldy. I'm also not a fan of declaring a local implicit because it seems vaguely orphan type class-y to me.

For the F[_, _] shaped types, it'll need another data type - that's why I had to write a BiProd when we already have a Prod. I have some proposals dealing with that in #968.

[codecov-io]

Current coverage is 89.32%

Merging #1021 into master will increase coverage by 1.17%

1. 2 files (not in diff) in ...cats/laws/discipline were deleted. more
2. 2 files (not in diff) in ...main/scala/cats/laws were deleted. more
3. 7 files (not in diff) in .../src/main/scala/cats were deleted. more
4. 3 files in .../src/main/scala/cats were modified. more
   • Misses -4
   • Hits -1
5. 4 files (not in diff) in ...main/scala/cats/laws were modified. more
   • Hits -3
6. 12 files (not in diff) in ...cala/cats/kernel/std were modified. more
7. 6 files (not in diff) in ...in/scala/cats/kernel were modified. more
   • Misses -3
   • Hits +3
8. 2 files (not in diff) in ...main/scala/cats/free were modified. more
   • Hits -3
9. 2 files (not in diff) in ...in/scala/cats/syntax were modified. more
10. 5 files (not in diff) in .../main/scala/cats/std were modified. more
    • Misses -7
    • Hits -5

@@             master      #1021   diff @@
==========================================
  Files           224        216     -8   
  Lines          2842       2752    -90   
  Methods        2785       2691    -94   
  Messages          0          0          
  Branches         52         58     +6   
==========================================
- Hits           2505       2458    -47   
+ Misses          337        294    -43   
  Partials          0          0          

Powered by Codecov. Last updated by 7f112dd...667f3b9

[adelbertc]

Don't understand this build fail thing, but yay +0.24%!

[ceedubs]

👏 I haven't done a thorough review yet, but at first glance this looks promising.

[non]

So I like the ability to wrap a data structure in Nested to get this behavior, but does it conflict with compose? In other words, is it impossible to work with nested structures without wrapping them? If possible, I might prefer to leave the compose methods alone and just add Nested.

[adelbertc]

The idea is if you have F[G[A]] and you just want the Functor for F[_] (e.g. fga.map((ga: G[A]) => ...)) then nothing changes, but if you want the inner A then you wrap in Nested which will resolve the Functor to the F[G[?]] one, allowing you to fga.map((a: A) => ...). Behavior with compose should be the same. However since I've removed the compose methods if someone wants to work with nested structures and get the composed instance, they need to wrap.

Among the other motivations for replacing compose with Nested is because we don't have product methods, instead delegating that work to a Prod data type. e.g. we have no Apply[Lambda[A => (F[A], G[A])]] but instead Apply[Prod[F, G, ?]]. To make it more consistent and to avoid having to pass in instances explicitly/create local implicits, I went for this one. A bit more info outlined here: #968

[ceedubs]

I guess one difference between this and the product case is that in the product case you have to return a different type (something like Prod or Tuple2). Here you can get by without a separate type.

I like Nested, but I'm also not opposed to keeping around the older compose methods (perhaps renamed to nest?) if people would like to. The former could probably even be implemented in terms of the latter? I'm fine either way on this.

[non]

My worry is that if someone has something like List[Tree[Set[A]]] and they want to fold over all of that, that they might end up having to do something like Nested(Nested(trees)) which just seems ugly. In those cases I can imagine preferring to build an instance explicitly, using some kind of .nest / .compose combinator.

[adelbertc]

Ah fair point - I will try to get that work in sometime this week. I think my plan will be:

1. Change the Nested instances to not use Nested[F, G, ?] but Lambda[A => F[G[A]] (but keep them in the Nested.scala file?
2. Add back the .nest combinator to type classes
3. Nested instances will just delegate to work to the Lambda[A => F[G[A]]] instances.

Thoughts?

[adelbertc]

Done!

[non]

Hey @adelbertc! This looks really good. I like the design but one thing I found confusing was the distinction between the anonymous type classes (in terms of Nested) and the type classes whose names start with "Nested". E.g. NestedFoldable versus implicit def nestedFoldable.

Could we split Nested.scala into two files: Nested.scala would have the Nested type and all its instances, and Composed.scala would have the composed instances (in terms of type lambdas)? How does that sound? Could we also rename the composed classes to be ComposedFoldable and rename the basic method back to .compose? I think this still accomplishes your goal of making composition using Nested easy, but helps preserve the distinction between nesting and composition.

[adelbertc]

Sounds good, I will make the change this week.

[adelbertc]

@non The only thing that bothers me slightly is having the methods called compose but the data type that gives the instance Nested. I would call it Compose but we already have cats.arrow.Compose.. would folks be OK with cats.data.Compose too? Or should we just throw our hands up and say compose if you want the un-wrapped ones and Nested if you want it wrapped?

[adelbertc]

It occurs to me maybe Composed (note the ending d) would work? Would also be consistent with the Composed* naming. I'll try to work on this tonight assuming nobody yells at me

[non]

I guess my feeling is that there are two things here and I would like to keep them distinct:

1. A case class that wraps F[G[A]] values, to make it easier to get the type class instances you want.
2. Methods to compose T[F] and T[G] instances into a T[λ[α => F[G[α]]]] instance.

I would prefer not to use very similar names for both of these, since they are somewhat different. I was happy with using nest/nested for (1) and compose/composed for (2), but if you don't like those names let's figure something else out.

[adelbertc]

I can buy into that, I'll try to work on it tonight or tomorrow :-)

[adelbertc]

Good to go!

[ceedubs]

👍 thanks @adelbertc!

[peterneyens]

I'm not sure, but since the compose methods ended up staying, shouldn't we also keep their tests (like in FunctorTests and ComposeTests, which maybe could be renamed to ComposedTests) ?

[adelbertc]

The reason I didn't keep them in their respective type class tests was because I didn't want them all scattered around

[peterneyens]

OK, that seems a good idea, but don't we need something like ComposedTests which contains the old tests from the type class tests and the ones from ComposeTests ? Or did I miss it ?

[adelbertc]

So I did a perhaps questionable trick - the Nested instances delegate to the "unwrapped" instances so the NestedTests will cover those as well. That was done in the interest of not testing what would amount to the same thing multiple times.

[peterneyens]

Ah OK now I see. I clearly didn't gave the Nested implementation itself the close up it deserved 😊

Thank you for explaining !

[non]

👍 Thanks @adelbertc