cats icon indicating copy to clipboard operation
cats copied to clipboard

Add FunctionN.liftN, parLiftN

Open kubukoz opened this issue 3 years ago • 14 comments

liftN is just like mapN, but it should provide better type inference - if you have 6 effects to put in a tuple, you can make a mistake in one and suddenly the whole tuple no longer has a mapN methods. With liftN, you put the function first, and only then pass the (wrapped) elements.

Consider this:

case class User(name: String, age: Int)

def validateName: ValidatedNel[String, String]
def validateAge: ValidatedNel[String, Int]

User.apply.liftN(validateName, validateAge): ValidatedNel[String, User]

~~tupledF is the same thing, but when your tuples are already in an effect - this can happen if you're unable to do .tupled (e.g. combining cats-parse's Parser + Parser0 - cc @johnynek) or you're just not in control over how the tuple is produced in the effect.~~ tupledF has been removed from this PR, we can revisit it in the future.

kubukoz avatar Nov 12 '22 23:11 kubukoz

I think I'd like to add the parallel versions too, but I don't want to invest additional time without some buy-in from the maintainers: let's talk about these first :)

kubukoz avatar Dec 23 '22 01:12 kubukoz

I like the idea.

I don't think I like the tupledF name. I think we should iterate on that a bit.

Maybe mapTuple? I don't know.

johnynek avatar Dec 23 '22 01:12 johnynek

I'm fine with pretty much any names ;)

kubukoz avatar Dec 23 '22 01:12 kubukoz

Related: https://github.com/typelevel/kittens?tab=readme-ov-file#lift-examples

joroKr21 avatar Jan 03 '24 17:01 joroKr21

Related: typelevel/kittens#lift-examples

that's pretty cool, I didn't know about it. Frankly I'd still like to have something like this PR in cats because not everybody uses kittens, plus liftN has slightly better UX because of:

  • no tupling
  • no Applicative[TypeConstructor], you start with the function you want to apply, just like if you were to apply it directly with no effects

kubukoz avatar Jan 03 '24 18:01 kubukoz

Sure, I'm not arguing against this PR - just pointing out one could use kittens in the meantime. Is there any difference between f.liftN.tupled and f.tupledF - i.e. do we need the extra boilerplate and strange name?

joroKr21 avatar Jan 03 '24 18:01 joroKr21

Is there any difference between f.liftN.tupled and f.tupledF - i.e. do we need the extra boilerplate and strange name?

@joroKr21 f.liftN[F].tupled (which is probably (f.liftN[F] _).tupled without -Xsource:3 in pre-3.x Scalas) is ((F[A], F[B])) => F[C], f.tupledF[F] is F[(A, B)] => C.

That said, tupledF offers less of an improvement over a simple map:

  • (??? : F[(String, Int, Boolean)]).map(fapply3) fails with found : (A, B, C) => T, required: ((String, Int, Boolean)) => ?
  • fapply3.tupledF(??? : F[(String, Int, Boolean)]) fails with found : F[(String, Int, Boolean)], required: ?F[(A, B, C)]

They may both have merit, though: ftuple.map(f) focuses on the inputs and lets you pick a function, whereas f.tupledF(ftuple) focuses on the function, letting you pick the arguments.

I don't have a strong opinion on whether tupledF needs to stay, personally I'm in it mostly for liftN. Perhaps it's best to focus on one thing and let the other one go until we find a more compelling reason to have it.

kubukoz avatar Jan 03 '24 22:01 kubukoz

Added parLiftN for good measure.

kubukoz avatar Jan 04 '24 01:01 kubukoz

Ah ok, the implementation is Functor[F].map(t)(f.tupled) which is the same as t.map(f.tupled) or f.tupled.liftN. Also related to Functor.lift which has no syntax though. It seems like too much hassle for minor details of syntax.

joroKr21 avatar Jan 04 '24 07:01 joroKr21

+1 on this to re-raise attention :)

TonioGela avatar Feb 14 '24 13:02 TonioGela

Removed tupledF so now we only have liftN and parLiftN - shall we get this merged?

kubukoz avatar Feb 27 '24 14:02 kubukoz

merging with main apparently broke something, I'll take a look.

kubukoz avatar Feb 27 '24 15:02 kubukoz

cats.jvm.tests.FutureSuite failed, but I don't think it uses the new functions. Could it be a flaky test?

I forgot to write down the failing hash and the logs are now gone :(

kubukoz avatar Feb 27 '24 18:02 kubukoz

Here is the log: https://github.com/typelevel/cats/actions/runs/8066611292/job/22035078228#step:13:7049

==> X cats.jvm.tests.FutureSuite.Future: coflatMap.coflatten throughMap  3.013s munit.FailException: Failing seed: _syITpm-TcruL5pZb_9QYj5RRgP3qQgh3LgMILjdKuM=
You can reproduce this failure by adding the following override to your suite:

  override val scalaCheckInitialSeed = "_syITpm-TcruL5pZb_9QYj5RRgP3qQgh3LgMILjdKuM="

Exception raised on property evaluation.
> ARG_0: Future(Success(1))
> Exception: java.util.concurrent.TimeoutException: Future timed out after [3 seconds]
    at munit.ScalaCheckSuite.propToTry(ScalaCheckSuite.scala:98)
Caused by: java.util.concurrent.TimeoutException: Future timed out after [3 seconds]
    at scala.concurrent.impl.Promise$DefaultPromise.tryAwait0(Promise.scala:248)
    at scala.concurrent.impl.Promise$DefaultPromise.result(Promise.scala:261)
    at scala.concurrent.Await$.$anonfun$result$1(package.scala:201)
    at scala.concurrent.BlockContext$DefaultBlockContext$.blockOn(BlockContext.scala:62)
    at scala.concurrent.Await$.result(package.scala:124)
    at cats.jvm.tests.FutureSuite.cats$jvm$tests$FutureSuite$$$anonfun$eqfa$1(FutureSuite.scala:45)

joroKr21 avatar Feb 27 '24 18:02 joroKr21

Can we revive this? The flaky test above doesn't seem to be related, and I find myself wanting this feature at least on a weekly basis. I think it'd be a very useful addition.

kubukoz avatar Sep 25 '24 23:09 kubukoz

Personally, I do like the idea, but still not sure that it belongs here: since it is a bunch of extension methods for standard Scala functions, it looks more like what mouse is for.

We can put it straight into Cats for sure, but Mouse is a great library too and it would be definitely missing it )

satorg avatar Sep 26 '24 03:09 satorg

I don't really agree that it's just a bunch of extensions for standard Scala functions: I would rather say it's alternative syntax for the existing mapN / parMapN, which are very common in application code.

I believe liftN could become the alternative for mapN because it's more natural (very similar to function application, just with arguments wrapped in a type constructor), especially if it's used in the common pattern of e.g. validation:

Foo(
  name,
  42
)

// almost the same
Foo.apply.liftN(
  validateName,
  42.validNel[String]
)

// feels upside down
(
  validateName,
  42.validNel[String]
).mapN(Foo.apply)

so I don't think putting it in mouse would do it justice. I'm pretty sure the only reason we've stuck with mapN in the past is that (Foo.apply _).liftN was ugly and difficult to write, but -Xsource:3 and Scala 3's growing market share have cleaned that up.

kubukoz avatar Sep 26 '24 20:09 kubukoz

First of all, sorry if my "bunch" wording might look dismissively – I didn't mean that. I just meant that there are many of them, not just one.

I see you point and it totally makes sense to me. I'm just trying to think it through from other perspectives.

For example, if liftN is supposed to be an alternative for mapN, then what about flatMapN? Should we consider adding syntaxes for (A0, ..., AN) => F[T] here in Cats as well to make the following possible:

class Foo private { ... } // cannot be instantiated with `new`
object Foo {
  def create[F[_]](n: Int, s: String): F[Foo] = ???
}

def fetchNum: F[Int] = ???
def fetchStr: F[String] = ???

Foo.create.flatLiftN(fetchNum, fetchStr) // => F[Foo]
// instead of
(fetchNum, fetchStr).flatMapN(Foo.create)

I have a feeling that this case may come along pretty soon. Personally, I'd like that, I'm just questioning about better housing.

satorg avatar Sep 26 '24 21:09 satorg

hmm I think flatMapN isn't that common, but it's a thing worth considering.

It's interesting that the order of effects in flatLiftN is still the same as the order of evaluating parameters in fetchNum(), fetchStr()).

For argument's sake, let's say we merge just liftN and parLiftN and someone comes up in a couple months asking for flatLiftN - is it a problem if we add it then? Would that change our decision to keep these in Cats?

kubukoz avatar Sep 26 '24 21:09 kubukoz

Just for the record, we already have Functor#lift.

https://github.com/typelevel/cats/blob/d1b08300c8b92137b6355cefaa643418cabd087c/core/src/main/scala/cats/Functor.scala#L76-L89

armanbilge avatar Sep 27 '24 05:09 armanbilge

@armanbilge , Btw, this part is quite clumsy:

    * Example: 
    * {{{ 
    * scala> import cats.Functor 
    * scala> import cats.implicits.catsStdInstancesForOption 

I don't think such an import should ever be suggested in any of Cats examples )

UPD: #4659

satorg avatar Sep 28 '24 17:09 satorg

@kubukoz , I think I'm fine with landing liftN in Cats. (The Arman's point above helped me to lean towards this a bit)

A couple more thoughts on the PR:

  1. Would you consider adding extends AnyVal to the *Ops classes? E.g.

    final class Function1ApplyOps[T, A0](private val f: Function1[A0, T]) extends AnyVal with Serializable {
      def liftN[F[_]: Functor](a0: F[A0]): F[T] = Functor[F].map(a0)(f)
      // ...
    }
    

    and similarly to the N-arity ones. Looks like it helps to avoid one extra memory allocation.

  2. Not an argument, just thinking aloud. The proposed liftN functions are not really pure "lifters" but also do "apply" when called. Although it is still possible to convert a pure "lift" into one with "apply" and vice versa, but due to Scala type inference details the syntax changes on the call site. Just in case, there are examples down below for both variants of liftN – pure and applied:

    final class Function3ApplyOps[T, A0, A1, A2](private val f: Function3[A0, A1, A2, T]) extends AnyVal with Serializable {
    
      // Pure "lift" – returns a new function out of the `f` parameter.
      def liftN_pure[F[_]: Functor: Semigroupal]: (F[A0], F[A1], F[A2]) => F[T] = Semigroupal.map3(_, _, _)(f)
    
      // Lifts `f` then applies `a0..a2` parameters at once.
      def liftN_apply[F[_]: Functor: Semigroupal](a0: F[A0], a1: F[A1], a2: F[A2]): F[T] = Semigroupal.map3(a0, a1, a2)(f)
    }
    

    And here is how they both can be called for both purposes – getting a lifter and applying a function all together:

    def validatedInt: Option[Int] = Some(123)
    def validatedStr: Option[String] = Some("hello")
    def validatedBool: Option[Boolean] = Some(true)
    
    // Get a lifter. Requires `[F]` to be provided.
    val liftPureToFun = Foo.apply.liftN_pure[Option]
    val res11 = liftPureToFun(validatedInt, validatedStr, validatedBool)
    
    // Apply the function. Requires `[F]` to be provided as well.
    val res12 = Foo.apply.liftN_pure[Option].apply(validatedInt, validatedStr, validatedBool)
    
    // Get a lifter. Requires `[F]` to be provided along with `.apply` (followed by  `_` in Scala 2).
    val liftApplyToFun = Foo.apply.liftN_apply[Option].apply _
    val res21 = liftApplyToFun(validatedInt, validatedStr, validatedBool)
    
    // Apply the function. Does not require `[F]` to be provided.
    val res22 = Foo.apply.liftN_apply(validatedInt, validatedStr, validatedBool)
    

    Apparently, the "lift+apply" variant is generally a bit more convenient to use. So perhaps it's better to keep it this way.

satorg avatar Sep 29 '24 03:09 satorg

I think the variant with lift+apply fits in both cases, especially with the cross-compilation flags I mentioned earlier (and Scala 3).

I'm adding the AnyVals now!

kubukoz avatar Sep 29 '24 18:09 kubukoz

@kubukoz ,

especially with the cross-compilation flags I mentioned earlier (and Scala 3).

Oh, you're right, this line from my example compiles on Scala 2.13 with -Xsource:3 (even without the trailing underscore added):

val liftApplyToFun = Foo.apply.liftN_apply[Option].apply

It is just my IntelliJ that seems not recognizing such a syntax and marking it as an error.

satorg avatar Sep 29 '24 19:09 satorg

It is just my IntelliJ that seems not recognizing such a syntax and marking it as an error.

good old IntelliJ doing its thing... 😅

kubukoz avatar Sep 30 '24 00:09 kubukoz

thanks for the merge, can't wait to see the release!

kubukoz avatar Sep 30 '24 00:09 kubukoz