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

Please remove/deprecate Foldable for Tuples #4430

Open
alexandru opened this issue Apr 19, 2023 · 6 comments
Open

Please remove/deprecate Foldable for Tuples #4430

alexandru opened this issue Apr 19, 2023 · 6 comments

Comments

@alexandru
Copy link
Member

alexandru commented Apr 19, 2023

This issue was probably debated before, sorry if I'm bringing up an old issue.

I just bumped into a bug of my making — had to wait on the completion of 3 IO tasks. The code went through several iterations, first using race, then using parMapN, as I needed the results, but then signatures changed, so I wanted something that simply waited on them all and ignored the results:

(
  task1,
  task2,
  task3,
).parSequence_

The behavior of this code is perplexing, especially for those familiar with its prior use of parMapN. The compiler should throw an error, but it doesn't. And this happens because an instance of Foldable[(A0, A1, *)] is defined, which makes absolutely no sense to me.

I was told that the reason for why Foldable gets defined on tuples, is due to key-value pairs. I am assuming that people want to do stuff like:

import cats.syntax.all._

("key", Option("value")).sequence
//=> val res0: Option[(String, String)] = Some((key,value))

("key", Right("value"): Either[Throwable, String]).sequence
//=> val res1: Either[Throwable, (String, String)] = Right((key,value))

("key", List("v1", "v2")).sequence
//=> val res2: List[(String, String)] = List((key,v1), (key,v2))

// ☝️ This last one is already getting into the weeds, TBH

However, I'd argue that, even for tuples of 2 elements, the Foldable instance is still error-prone. That is because not all tuples of 2 elements are key-value pairs. This is a semantic that's only available in context. Because, obviously, this code is also wrong:

(task1, task2).sequence_

Interestingly, the instance is right-leaning. We define Foldable[(A0, *)] and not Foldable[(*, A0)]. We define Foldable[(A0, A1, *)] and not Foldable[(A0, *, A2)] or Foldable[(*, A1, A2)]. Why is that? Why are the semantics of Either available here as well? Is this some sort of associativity convention that's imported from Haskell? (e.g., type currying)

In Scala, I don't think it makes any sense, apart from just picking a convention that happens to be consistent with the ordering of key-value pairs, or of Either. I mean, in Either, one value is clearly more important than the other. It's the value that represents the happy-path and that doesn't short-circuit flatMap. Well, in a tuple, I don't see how you can say the same thing. As (ioTask1, ioTask2) is a very common tuple in Typelevel Scala codebases, and it's clearly not a key-value pair.

The biggest problem that I see with Foldable on tuples is that, visually, you expect that operation to hit all elements in your “collection”. A tuple is a collection, even if it's a heterogeneous one. If a foldLeft operation only works on the last element of our collection, then it invalidates our mental model for how Foldable behaves. Again, look at this piece of code:

(
  task1,
  task2,
  task3,
).parSequence_

I can't make too many assumptions for others, but my eyes first see a list of IO objects, separated by commas. Without much knowledge of the type system, I can only describe this as a list of things. I expect Foldable to describe loops that go over all those things. If it can't, due to the types not matching, then might as well not define Foldable for it. And that's because my eyes don't see different types, and the compiler isn't telling that this here is a tuple that might not be compatible with what I'm trying to do. Both my senses and my compiler are failing me here.

Whatever use-cases these instances have, they pale in comparison to the potential for bugs this generates. Especially since, in the case of IO we deal with Unit or other results we can ignore, therefore the _ versions (or the .void calls) are pretty common.

@armanbilge
Copy link
Member

(Just for the record for tuples should use .tupled or .parTupled instead of sequence).

This tuple issue recently caused me pain as well in armanbilge/calico#199 (comment) and I ended up removing the Foldable-based APIs and replacing with dedicated APIs for List and stuff.

@djspiewak
Copy link
Member

To add more fuel to the fire, Scala 3 now offers a map function on Tuple, but this conflicts directly with the map function which comes from Functor. You see where this is going. Now in this case, maybe the argument is less clear-cut than it is with Foldable, but still. These things can be pretty confusing.

On the flip side though, (A, *) is a functor, and (*, *) is a bifunctor, and it's easy to see how (A, *) is also a Traverse, and these sorts of things do have real value in non-obvious cases. For example, Free[(A, *), *] is a tree of As, and you can do fun things with that. You lose this entirely if we drop Traverse[(A, *)] (recall, Traverse <: Foldable).

@joroKr21
Copy link
Member

You could say the same about Ior

@saftacatalinmihai
Copy link

#3812

@Daenyth
Copy link
Contributor

Daenyth commented Jun 16, 2023

What if we let the instances remain defined but just made them no longer implicit? That way someone who wants to use the instance for useful things can grab it manually but it isn't a big footgun for day-to-day code

@ollyw
Copy link
Contributor

ollyw commented Jul 14, 2023

What if we let the instances remain defined but just made them no longer implicit? That way someone who wants to use the instance for useful things can grab it manually but it isn't a big footgun for day-to-day code

If they are left, implicit or not, it would be good to have some clear API documentation on the behaviour of keeping the last item

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

No branches or pull requests

7 participants