avoid deprecated macro APIs post 2.10 #2534
Conversation
| @@ -139,7 +139,7 @@ private[arrow] object FunctionKMacros extends MacroCompat { | |||
|
|
|||
| private[this] def punchHole(tpe: Type): Tree = tpe match { | |||
There was a problem hiding this comment.
I would like to take a moment to appreciate the name of this method.
|
The removed trait is |
|
It looks to me that this change should be bin compat (although I am not really sure since macro is involved). Regardless, I think it would be a good practice to add a test to verify that the intended end usage is still bin compat when we add a Mima exception. The PR to add such tests and related infrastructure is #2509 @larsrh would you take a look? Once that is merged, we can add one for Another thing, maybe I should mention it first, is that @milessabin suggested that we don't really need macro here. we can just do /**
* Lifts function `f` of `F[A] => G[A]` into a `FunctionK[F, G]`.
*
* {{{
* scala> import cats.arrow.FunctionK
* scala> def headOption[A](list: List[A]): Option[A] = list.headOption
* scala> val lifted: FunctionK[List, Option] = FunctionK.lift(headOption)
* scala> lifted(List(1,2,3))
* res0: Option[Int] = Some(1)
* }}}
*
* Note: This method has a macro implementation that returns a new
* `FunctionK` instance as follows:
*
* {{{
* new FunctionK[F, G] {
* def apply[A](fa: F[A]): G[A] = f(fa)
* }
* }}}
*
* Additionally, the type parameters on `f` must not be specified.
*/
def lift[F[_], G[_]](f: (F[α] ⇒ G[α]) forSome { type α }): FunctionK[F, G] =
new FunctionK[F, G] {
def apply[A](fa: F[A]): G[A] = f.asInstanceOf[F[A] => G[A]](fa)
}Update: Mima didn't complain about this implementation change. So if we use this |
|
cc @andyscott who added the macro to see if he has any concerns over the |
|
Here's a problem with this approach: scala> import cats.arrow._
import cats.arrow._
scala> def sample[A](option: Option[A]): List[A] = option.toList
sample: [A](option: Option[A])List[A]
scala> FunctionK.lift(sample[String])
res0: cats.arrow.FunctionK[Option,List] = cats.arrow.FunctionK$$anon$5@53cd7d41This should supposedly be ruled out. |
|
How did the macro version respond, if you have tried? |
|
That's checked in the test suite with |
| @@ -124,7 +124,7 @@ private[arrow] object FunctionKMacros extends MacroCompat { | |||
| val G = punchHole(evG.tpe) | |||
|
|
|||
| q""" | |||
| new FunctionK[$F, $G] { | |||
| new _root_.cats.arrow.FunctionK[$F, $G] { | |||
There was a problem hiding this comment.
This is a hygiene issue that I fixed along the way.
| "org.scalatest" %%% "scalatest" % scalatestVersion(scalaVersion.value) % Test | ||
| ) | ||
| ) | ||
| .dependsOn(coreJVM % Test) |
There was a problem hiding this comment.
I didn't realize that the approach in #2509 would require a publishLocal first. This here may be simpler.
There was a problem hiding this comment.
right. I was testing against existing published jars, didn't realize that I can just do module dependency.
|
@kailuowang This is still the macro solution, with bincompat tests & filters on top. |
Codecov Report
@@ Coverage Diff @@
## master #2534 +/- ##
==========================================
- Coverage 95.18% 95.14% -0.05%
==========================================
Files 360 359 -1
Lines 6548 6548
Branches 279 279
==========================================
- Hits 6233 6230 -3
- Misses 315 318 +3
Continue to review full report at Codecov.
|
|
I definitely am in favor of removing the macro implementation! |
@andyscott Just to be clear, this PR doesn't do that, because the non-macro solution is unsafe. |
|
I'm pretty clueless wrt macros, so I'm not sure I'm a good authority to review this PR, but I trust you two to have done the correct thing, so I think we can probably merge this :) |
|
@LukaJCB I left the macro in place with minor changes, really nothing fancy happening here (@kailuowang did all the work on BC) 😄 |
|
For posterity: #2553 (comment) |
No description provided.