diff --git a/README.md b/README.md index fe098320e..fccfb64fa 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,21 @@ Add Scalafix to your project's build by [following the instructions](https://sca } ``` +### `AddCatsTaglessInstances` + +The `AddCatsTaglessInstances` rule finds generated Thrift service traits and adds implicit instances of +`ThriftService[Kleisli[F, ThriftService[Future], *]]` and `FunctorK[ThriftService]` to each service's +companion object. + +Twitter's Scrooge project changed the way it generates code for Thrift services, removing the +higher-kinded service trait used by this library, leaving only the `MethodPerEndpoint` trait +that used to extend the higher-kinded service trait, setting the type parameter to `com.twitter.util.Future`. +The `AddCatsTaglessInstances` rule now addresses this as well, rewriting `MethodPerEndpoint` to +`{Name}Service` and reintroducing the type parameter. (A new `MethodPerEndpoint` is also added, +going back to how it used to `extend {Name}Service[Future]`.) + +This Scalafix rule should be idempotent, so it can be rerun many times. + ## Artifacts The Group ID for each artifact is `"com.dwolla"`. All artifacts are published to Maven Central. diff --git a/build.sbt b/build.sbt index 1692b3d1c..875c0ed92 100644 --- a/build.sbt +++ b/build.sbt @@ -232,7 +232,8 @@ lazy val `scalafix-rules` = (projectMatrix in file("scalafix/rules")) moduleName := "finagle-tagless-scalafix", libraryDependencies ++= Seq( "ch.epfl.scala" %% "scalafix-core" % _root_.scalafix.sbt.BuildInfo.scalafixVersion, - + "org.scalameta" %% "munit" % "0.7.29" % Test, + "com.eed3si9n.expecty" %% "expecty" % "0.15.4" % Test, ), scalacOptions ~= { _.filterNot(_ == "-Xfatal-warnings") }, ) diff --git a/scalafix/output/src/main/scala/example/thrift/SimpleService.scala b/scalafix/output/src/main/scala/example/thrift/SimpleService.scala index ec73471ad..9bd9c2cb7 100644 --- a/scalafix/output/src/main/scala/example/thrift/SimpleService.scala +++ b/scalafix/output/src/main/scala/example/thrift/SimpleService.scala @@ -737,9 +737,9 @@ object SimpleService extends _root_.com.twitter.finagle.thrift.GeneratedThriftSe type makeRequest$result = MakeRequest.Result - trait MethodPerEndpoint extends _root_.com.twitter.finagle.thrift.ThriftService { + trait SimpleService[F[_]] extends _root_.com.twitter.finagle.thrift.ThriftService { - def makeRequest(request: example.thrift.SimpleRequest): Future[example.thrift.SimpleResponse] + def makeRequest(request: example.thrift.SimpleRequest): F[example.thrift.SimpleResponse] /** * Used to close the underlying `Service`. * Not a user-defined API. @@ -747,6 +747,15 @@ object SimpleService extends _root_.com.twitter.finagle.thrift.GeneratedThriftSe def asClosable: _root_.com.twitter.util.Closable = _root_.com.twitter.util.Closable.nop } + object SimpleService { + implicit def SimpleServiceInReaderT[F[_]]: SimpleService[({type Λ[β0] = _root_.cats.data.ReaderT[F, SimpleService[F], β0]})#Λ] = + _root_.cats.tagless.Derive.readerT[SimpleService, F] + + implicit val SimpleServiceFunctorK: _root_.cats.tagless.FunctorK[SimpleService] = _root_.cats.tagless.Derive.functorK[SimpleService] + } + + trait MethodPerEndpoint extends SimpleService[Future] + object MethodPerEndpoint { def apply(servicePerEndpoint: ServicePerEndpoint): MethodPerEndpoint = { diff --git a/scalafix/rules/src/main/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstances.scala b/scalafix/rules/src/main/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstances.scala index 949412552..ff77a88fb 100644 --- a/scalafix/rules/src/main/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstances.scala +++ b/scalafix/rules/src/main/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstances.scala @@ -1,36 +1,213 @@ package com.dwolla.scrooge.scalafix +import com.dwolla.scrooge.scalafix.AddCatsTaglessInstances._ import scalafix.v1._ import scala.meta._ +import scala.meta.tokens.Token.Ident +import GuardedPatchBuilder.toGuardedPatch -class AddCatsTaglessInstances extends SemanticRule("AddCatsTaglessInstances") { - override def fix(implicit doc: SemanticDocument): Patch = - Patch.fromIterable(addImplicitsToCompanionObjects) +object ThriftServiceTrait { + def unapply(subtree: Tree): Option[(String, Defn.Trait)] = + PartialFunction.condOpt(subtree) { + case term@Defn.Trait(_, Type.Name(name), Nil, _, ExtendsThriftService(_)) => (name, term) + } +} + +object ExtendsThriftService { + def unapply(subtree: Tree): Option[Template] = + TemplateExtends("ThriftService")(subtree) +} + +object ExtendsGeneratedThriftService { + def unapply(subtree: Tree): Option[Template] = + TemplateExtends("GeneratedThriftService")(subtree) +} + +object InitsContainName { + def apply(name: String) + (inits: List[Init]): Option[Init] = + inits.find { + _.tpe.collect { + case Type.Select(_, Type.Name(`name`)) => () + case Type.Name(`name`) => () + }.nonEmpty + } +} - private def buildThriftServiceFromTrait(alg: Defn.Trait) - (implicit doc: SemanticDocument): ThriftService = { +object TemplateExtends { + def apply(name: String): Tree => Option[Template] = subtree => { + object TemplateInitsContainName { + def unapply(inits: List[Init]): Option[Init] = + InitsContainName(name)(inits) + } + + PartialFunction.condOpt(subtree) { + case t@Template(_, TemplateInitsContainName(_), _, _) => t + } + } +} + +object AddCatsTaglessInstances { + private def buildThriftServiceFromTrait(tree: Tree, alg: Defn.Trait): ThriftService = { val name = alg.name.value val companion = - doc - .tree + tree .collect { case obj@Defn.Object(_, Term.Name(`name`), _) => obj } .headOption - ThriftService(alg, companion) + ThriftService(alg, companion) } - private def addImplicitsToCompanionObjects(implicit doc: SemanticDocument): List[Patch] = - doc - .tree + def addImplicitsToCompanionObjects(tree: Tree): List[Patch] = + tree .collect { case TraitWithTypeConstructor(name) => name } - .map(buildThriftServiceFromTrait) + .map(buildThriftServiceFromTrait(tree, _)) .map(_.toPatch) + + def addServiceTrait(tree: Tree): List[Patch] = + tree + .collect { + // find each object that extends the com.twitter.finagle.thrift.GeneratedThriftService interface + // these are the root objects that contain all the generated traits we need to modify and augment + case t@Defn.Object(_, _, ExtendsGeneratedThriftService(_)) => t + } + .flatMap { companionObjectTree => + companionObjectTree + .collect { + // find each trait that extends the com.twitter.finagle.thrift.ThriftService interface + // these are the interfaces we want to modify to add a higher kinded type parameter F[_], + // and replace Future with the new F + case ThriftServiceTrait("MethodPerEndpoint", defn) => + ThriftServiceTrait(companionObjectTree, defn) + } + .map(_.toPatch) + } +} + +case class ThriftServiceTrait(private val generatedThriftServiceObject: Defn.Object, + private val thriftServiceTrait: Defn.Trait) { + private object ExtendsServiceTrait { + def unapply(subtree: Tree): Option[Template] = + TemplateExtends(generatedThriftServiceObject.name.value)(subtree) + } + + private def allInstances: List[ImplicitInstance] = List( + ImplicitInstance.AlgebraInKleisli(generatedThriftServiceObject.name.value), + ImplicitInstance.FunctorK(generatedThriftServiceObject.name.value), + ) + + private def code(instances: List[ImplicitInstance]): String = + instances.map(_.code).mkString("", "\n", "") + + private val companionObjectWithImplicits = + s"""| + | + | object ${generatedThriftServiceObject.name.value} { + |${code(allInstances)} + | } + |""".stripMargin + + private def renameMethodPerEndpoint(methodPerEndpoint: Type.Name): Patch = + methodPerEndpoint.tokens.find { + case Ident("MethodPerEndpoint") => true + case _ => false + } + .map(Patch.replaceToken(_, s"${generatedThriftServiceObject.name.value}[F[_]]")) + .onlyIfMissing { + case Defn.Trait(_, Type.Name(name), _, _, _) if name == generatedThriftServiceObject.name.value => () + } + .in(generatedThriftServiceObject) + + private val addServiceCompanionObjectWithImplicits: Patch = + Patch.addRight(thriftServiceTrait, companionObjectWithImplicits) + .onlyIfMissing { + case Defn.Object(_, Term.Name(name), Template(_, List(), _, _)) if name == generatedThriftServiceObject.name.value => () + } + .in(generatedThriftServiceObject) + + private val addReplacementMethodPerEndpointTrait: Patch = + Patch.addRight(thriftServiceTrait, + s""" + | trait MethodPerEndpoint extends ${generatedThriftServiceObject.name.value}[Future]""".stripMargin) + .onlyIfMissing { + case Defn.Trait(_, Type.Name("MethodPerEndpoint"), _, _, ExtendsServiceTrait(_)) => () + } + .in(generatedThriftServiceObject) + + def toPatch: Patch = + Patch.fromIterable { + thriftServiceTrait + .collect { + // rename MethodPerEndpoint to {Name}Service[F[_]], add {Name}Service companion object with + case t@Type.Name("MethodPerEndpoint") => + Patch.fromIterable { + List( + renameMethodPerEndpoint(t), + addServiceCompanionObjectWithImplicits, + addReplacementMethodPerEndpointTrait, + ) + } + + // replace Future[*] with F[*] + case t@Type.Name("Future") => + t.parent.fold(Patch.empty) { + Patch.replaceToken(t.tokens.head, "F") + .onlyIfMissing { + case Type.Apply(Type.Name(name), _) if name == generatedThriftServiceObject.name.value => () + } + .in(_) + } + } + } +} + +class GuardedPatch(private val tuple: (Patch, PartialFunction[Tree, _])) extends AnyVal { + def in(tree: Tree): Patch = { + val empty = tree.collect(tuple._2).isEmpty + if (empty) tuple._1 + else Patch.empty + } +} + +class GuardedPatchBuilder(private val patch: Patch) extends AnyVal { + def onlyIfMissing(pf: PartialFunction[Tree, _]) = new GuardedPatch(patch -> pf) +} + +object GuardedPatchBuilder { + implicit def toGuardedPatch(patch: Patch): GuardedPatchBuilder = new GuardedPatchBuilder(patch) + implicit def toGuardedPatch(patch: Option[Patch]): GuardedPatchBuilder = new GuardedPatchBuilder(patch.getOrElse(Patch.empty)) +} + +sealed trait IdempotentPatch { + val patch: Patch +} +case object AlreadyApplied extends IdempotentPatch { + val patch: Patch = Patch.empty +} +case class Unapplied(patch: Patch) extends IdempotentPatch + +object IdempotentPatch { + def apply(patch: Patch) + (tree: Tree) + (pf: PartialFunction[Tree, _]): IdempotentPatch = + if (tree.collect(pf).nonEmpty) AlreadyApplied + else Unapplied(patch) + + def apply(patch: Option[Patch]) + (tree: Tree) + (pf: PartialFunction[Tree, _]): IdempotentPatch = + IdempotentPatch(patch.getOrElse(Patch.empty))(tree)(pf) +} + +class AddCatsTaglessInstances extends SemanticRule("AddCatsTaglessInstances") { + override def fix(implicit doc: SemanticDocument): Patch = + Patch.fromIterable(addServiceTrait(doc.tree) ++ addImplicitsToCompanionObjects(doc.tree)) } case class ThriftService(alg: Defn.Trait, @@ -82,14 +259,14 @@ sealed trait ImplicitInstance { object ImplicitInstance { case class AlgebraInKleisli(name: String) extends ImplicitInstance { def code: String = - s""" implicit def ${name}InReaderT[F[_]]: $name[({type Λ[β0] = _root_.cats.data.ReaderT[F, $name[F], β0]})#Λ] = - | _root_.cats.tagless.Derive.readerT[$name, F] + s""" implicit def ${name}InReaderT[F[_]]: $name[({type Λ[β0] = _root_.cats.data.ReaderT[F, $name[F], β0]})#Λ] = + | _root_.cats.tagless.Derive.readerT[$name, F] |""".stripMargin } case class FunctorK(name: String) extends ImplicitInstance { def code: String = - s" implicit val ${name}FunctorK: _root_.cats.tagless.FunctorK[$name] = _root_.cats.tagless.Derive.functorK[$name]" + s" implicit val ${name}FunctorK: _root_.cats.tagless.FunctorK[$name] = _root_.cats.tagless.Derive.functorK[$name]" } } diff --git a/scalafix/rules/src/test/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstancesTest.scala b/scalafix/rules/src/test/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstancesTest.scala index 19987474a..de30936fe 100644 --- a/scalafix/rules/src/test/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstancesTest.scala +++ b/scalafix/rules/src/test/scala/com/dwolla/scrooge/scalafix/AddCatsTaglessInstancesTest.scala @@ -1,50 +1,97 @@ package com.dwolla.scrooge.scalafix +import com.eed3si9n.expecty.Expecty.expect +import munit.FunSuite import scalafix.Patch import scala.meta._ -object AddCatsTaglessInstancesTest extends App { +class AddCatsTaglessInstancesTest extends FunSuite { - val input = - f"""trait SimpleService[F[_]] - |object SimpleService extends _root_.com.twitter.finagle.thrift.GeneratedThriftService { self => - | val annotations: immutable$$Map[String, String] = immutable$$Map.empty + private val input = + f"""object SimpleService extends _root_.com.twitter.finagle.thrift.GeneratedThriftService { self => + | trait MethodPerEndpoint extends _root_.com.twitter.finagle.thrift.ThriftService { | - | implicit def SimpleServiceInReaderT[F[_]]: SimpleService[({type Λ[β0] = _root_.cats.data.ReaderT[F, SimpleService[F], β0]})#Λ] = - | _root_.cats.tagless.Derive.readerT[SimpleService, F] + | def makeRequest(request: example.thrift.SimpleRequest): Future[example.thrift.SimpleResponse] + | /** + | * Used to close the underlying `Service`. + | * Not a user-defined API. + | */ + | def asClosable: _root_.com.twitter.util.Closable = _root_.com.twitter.util.Closable.nop + | } + |} + | + |// this section simulates an input that has already run the scalafix once, to ensure that the rules don't accumulate during each compile + |object SimpleService2 extends _root_.com.twitter.finagle.thrift.GeneratedThriftService { self => + | trait SimpleService2[F[_]] extends _root_.com.twitter.finagle.thrift.ThriftService { + | def makeRequest(request: example.thrift.SimpleRequest): F[example.thrift.SimpleResponse] + | /** + | * Used to close the underlying `Service`. + | * Not a user-defined API. + | */ + | def asClosable: _root_.com.twitter.util.Closable = _root_.com.twitter.util.Closable.nop + | } + | + | object SimpleService2 { + | implicit def SimpleService2InReaderT[F[_]]: SimpleService2[({type Λ[β0] = _root_.cats.data.ReaderT[F, SimpleService2[F], β0]})#Λ] = + | _root_.cats.tagless.Derive.readerT[SimpleService2, F] + | + | implicit val SimpleService2FunctorK: _root_.cats.tagless.FunctorK[SimpleService2] = _root_.cats.tagless.Derive.functorK[SimpleService2] + | } | - | implicit val SimpleServiceFunctorK: _root_.cats.tagless.FunctorK[SimpleService] = _root_.cats.tagless.Derive.functorK[SimpleService] + | trait MethodPerEndpoint extends SimpleService2[Future] |} - |trait Bar - |trait Baz[F] - |trait Boo[+MM[_]] |""".stripMargin - val tree: Tree = input.parse[Source].get + private val tree: Tree = input.parse[Source].get + + private val patches = AddCatsTaglessInstances.addServiceTrait(tree) + + private val patch = Patch.fromIterable(patches) + + private def range(str: String) + (f: String => Int): String = + s"${f(str)}..${f(str) + str.length}" + + test("rename MethodPerEndpoint to SimpleService[F[_]]") { + val expectedPatchContent = + s"Add(MethodPerEndpoint, MethodPerEndpoint [${range("MethodPerEndpoint")(input.indexOf)}), SimpleService[F[_]])" + + expect(patch.toString.contains(expectedPatchContent)) + } - val symbols: List[Defn.Trait] = - tree - .collect { - case TraitWithTypeConstructor(name) => name - } + test("replace Future[*] with F[*]") { + val expectedPatchContent = + s"Add(Future, Future [${range("Future")(input.indexOf)}), F)" - val thriftServices = - symbols - .map { alg => - val name = alg.name.value + expect(patch.toString.contains(expectedPatchContent)) + } - val companion = - tree - .collect { - case obj@Defn.Object(_, Term.Name(`name`), _) => obj - } - .headOption + test("add SimpleService companion object with implicits") { + val expectedPatchContent = + """Add(}, } [467..468), } + | + | object SimpleService { + | implicit def SimpleServiceInReaderT[F[_]]: SimpleService[({type Λ[β0] = _root_.cats.data.ReaderT[F, SimpleService[F], β0]})#Λ] = + | _root_.cats.tagless.Derive.readerT[SimpleService, F] + | + | implicit val SimpleServiceFunctorK: _root_.cats.tagless.FunctorK[SimpleService] = _root_.cats.tagless.Derive.functorK[SimpleService] + | } + |)""".stripMargin - ThriftService(alg, companion) - } + expect(patch.toString.contains(expectedPatchContent)) + } - val patches = thriftServices.map(_.toPatch) + test("add replacement MethodPerEndpoint that extends SimpleService[Future]") { + val expectedPatchContent = + """Add(}, } [467..468), } + | trait MethodPerEndpoint extends SimpleService[Future])""".stripMargin - println(Patch.fromIterable(patches)) + expect(patch.toString.contains(expectedPatchContent)) + } + + test("the patches don't touch SimpleService2, which has already been fixed") { + expect(!patch.toString.contains("SimpleService2")) + expect(!patch.toString.contains(s"Add(Future, Future [${range("Future")(input.lastIndexOf)}), F)")) + } }