Simplify FunctionK.lift macro #3402

joroKr21 · 2020-04-25T08:25:42Z

Add FunctionK.liftFunction that uses an abstract type member
to emulate a polymorphic function type.

Because liftFunction has worse type inference,
keep the macro based lift that now delegates to liftFunction,
instead of creating a new FunctionK instance every time.
Also accept eta-expanded functions in the macro implementation.

Fixes #3400

codecov-io · 2020-04-25T08:47:03Z

Codecov Report

Merging #3402 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3402   +/-   ##
=======================================
  Coverage   92.70%   92.71%           
=======================================
  Files         379      379           
  Lines        7981     7984    +3     
  Branches      230      216   -14     
=======================================
+ Hits         7399     7402    +3     
  Misses        582      582

Flag	Coverage Δ
#scala_version_212	`92.74% <100.00%> (+<0.01%)`	⬆️
#scala_version_213	`92.49% <100.00%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/arrow/FunctionK.scala	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cb9df6...7d93e4d. Read the comment docs.

djspiewak · 2020-06-07T19:50:49Z

core/src/main/scala/cats/arrow/FunctionK.scala

+   * Note: The weird `τ[F, G]` parameter is there to compensate for
+   * the lack of polymorphic function types in Scala 2.
+   */
+  def liftFunction[F[_], G[_]](f: F[τ[F, G]] => G[τ[F, G]]): FunctionK[F, G] = new FunctionK[F, G] {


Are we entirely sure this is sound? I know that similar tricks involving a *-kinded tau are not, but I find it difficult to reason about scalac's behavior in these cases.

I took it from the discussion in #2553. We could also call it unsafeLift but I guess it would have to be public because it's called in the expanded macro.

AFAIU because F and G are type parameters of τ you cannot reuse it with nested FunctionKs which would be a way to exploit unsoundness if we had a bare τ without type parameters.

codecov-commenter · 2020-07-21T06:36:43Z

Codecov Report

Merging #3402 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3402   +/-   ##
=======================================
  Coverage   91.30%   91.30%           
=======================================
  Files         386      386           
  Lines        8565     8565           
  Branches      255      263    +8     
=======================================
  Hits         7820     7820           
  Misses        745      745

barambani · 2020-07-22T15:09:21Z

tests/src/test/scala/cats/tests/FunctionKSuite.scala

+
+  test("lift a function directly") {
+    def headOption[A](list: List[A]): Option[A] = list.headOption
+    val fHeadOption = FunctionK.liftFunction[List, Option](headOption)


A question for my understanding, could this be

Suggested change

val fHeadOption = FunctionK.liftFunction[List, Option](headOption)

val fHeadOption = FunctionK.lift[List, Option](headOption)

Isn't it forwarding to liftFunction anyway ? Or it's testing the public liftFunction explicitly on purpose ?
Apologies if this doesn't make sense :).

It's testing liftFunction explicitly. Do you think we should also try to add a dotty version? It has native polymorphic functions, but we don't provide a way to convert them to FunctionK and I think type inference is still not great.

It makes sense, thanks for clarifying. About the dotty version, I guess it's related to #2553 that's still an open issue for what I understand and specifically there is work going on as note here #2553 (comment). We could sync a dotty specific version with that effort.
So what do you thing (you and others) about letting this PR move on as it improves the current situation described here #3400 and might make some user's life easier ? My approval goes in that direction. Of course I happy to do otherwise if the general preference is different.

Yes I agree this can be merged and I can try to add a dotty version in another PR 👍

Brilliant, thank 🙇

LukaJCB

Thanks for this. Hopefully we can find a dotty solution soon as well

djspiewak · 2020-07-28T15:50:16Z

Thanks for this. Hopefully we can find a dotty solution soon as well

FYI I actually have a Dotty solution already, but it doesn't compile because higher-rank type inference isn't fully implemented. @smarter has mentioned that this is something he wants to do, but he hasn't had the time.

joroKr21 · 2020-07-29T11:36:51Z

FYI I actually have a Dotty solution already, but it doesn't compile because higher-rank type inference isn't fully implemented.

Yes, that's what I meant, type inference doesn't work. I'm not sure if this can be solved with a macro as in Scala 2 but we can at least have lift for Dotty that takes a polymorphic function.

barambani · 2020-08-04T16:22:28Z

@djspiewak , sorry for the ping, this PR has two approvals so if you're happy with the changes and the conflicts are resolved I would merge it in.

Add `FunctionK.liftFunction` that uses an abstract type member to emulate a polymorphic function type. Because `liftFunction` has worse type inference, keep the macro based `lift` that now delegates to `liftFunction`, instead of creating a new `FunctionK` instance every time. Also accept eta-expanded functions in the macro implementation.

barambani

👍

joroKr21 mentioned this pull request Apr 25, 2020

Replace FunctionK.lift macro #3401

Closed

djspiewak reviewed Jun 7, 2020

View reviewed changes

joroKr21 force-pushed the functionk-lift branch 2 times, most recently from bc1b7b3 to c204ae4 Compare July 21, 2020 06:11

barambani reviewed Jul 22, 2020

View reviewed changes

barambani previously approved these changes Jul 25, 2020

View reviewed changes

LukaJCB previously approved these changes Jul 28, 2020

View reviewed changes

joroKr21 dismissed stale reviews from LukaJCB and barambani via 78976a5 August 6, 2020 13:49

joroKr21 force-pushed the functionk-lift branch from c204ae4 to 78976a5 Compare August 6, 2020 13:49

barambani previously approved these changes Aug 6, 2020

View reviewed changes

Merge branch 'master' into functionk-lift

669e59e

joroKr21 dismissed barambani’s stale review via 669e59e October 21, 2020 17:56

larsrh added 2 commits December 6, 2020 10:52

Merge branch 'master' into functionk-lift

12c025f

move FunctionKSuite to Scala 2 tests

a2274ee

larsrh approved these changes Dec 6, 2020

View reviewed changes

larsrh merged commit 8e8be51 into typelevel:master Dec 6, 2020

joroKr21 deleted the functionk-lift branch December 6, 2020 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify FunctionK.lift macro #3402

Simplify FunctionK.lift macro #3402

joroKr21 commented Apr 25, 2020

codecov-io commented Apr 25, 2020 •

edited

Loading

djspiewak Jun 7, 2020

joroKr21 Jun 7, 2020

joroKr21 Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jul 21, 2020 •

edited

Loading

barambani Jul 22, 2020

joroKr21 Jul 23, 2020

barambani Jul 25, 2020

joroKr21 Jul 26, 2020

barambani Jul 26, 2020

LukaJCB left a comment

djspiewak commented Jul 28, 2020

joroKr21 commented Jul 29, 2020

barambani commented Aug 4, 2020

barambani left a comment

	val fHeadOption = FunctionK.liftFunction[List, Option](headOption)
	val fHeadOption = FunctionK.lift[List, Option](headOption)

Simplify FunctionK.lift macro #3402

Simplify FunctionK.lift macro #3402

Conversation

joroKr21 commented Apr 25, 2020

codecov-io commented Apr 25, 2020 • edited Loading

Codecov Report

djspiewak Jun 7, 2020

Choose a reason for hiding this comment

joroKr21 Jun 7, 2020

Choose a reason for hiding this comment

joroKr21 Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 21, 2020 • edited Loading

Codecov Report

barambani Jul 22, 2020

Choose a reason for hiding this comment

joroKr21 Jul 23, 2020

Choose a reason for hiding this comment

barambani Jul 25, 2020

Choose a reason for hiding this comment

joroKr21 Jul 26, 2020

Choose a reason for hiding this comment

barambani Jul 26, 2020

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

djspiewak commented Jul 28, 2020

joroKr21 commented Jul 29, 2020

barambani commented Aug 4, 2020

barambani left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 25, 2020 •

edited

Loading

joroKr21 Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jul 21, 2020 •

edited

Loading