From 8bf2679a0a7d75850fb30cbbd7fc8f32cce55c79 Mon Sep 17 00:00:00 2001 From: Aleksander Rainko Date: Thu, 22 Aug 2024 19:46:06 +0200 Subject: [PATCH 1/2] move Factory summoning up to Planner --- .../ducktape/internal/ErrorMessage.scala | 9 ++++++ .../internal/FallibilityRefiner.scala | 2 +- .../internal/FalliblePlanInterpreter.scala | 4 +-- .../arainko/ducktape/internal/Plan.scala | 2 ++ .../ducktape/internal/PlanConfigurer.scala | 2 +- .../ducktape/internal/PlanInterpreter.scala | 4 +-- .../ducktape/internal/PlanTraverser.scala | 2 +- .../arainko/ducktape/internal/Planner.scala | 29 +++++++++++++++---- .../arainko/ducktape/internal/Structure.scala | 6 +++- .../ducktape/issues/Issue195Suite.scala | 27 +++++++++++++++++ 10 files changed, 73 insertions(+), 14 deletions(-) create mode 100644 ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/ErrorMessage.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/ErrorMessage.scala index 7d691685..242ac682 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/ErrorMessage.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/ErrorMessage.scala @@ -117,4 +117,13 @@ private[ducktape] object ErrorMessage { |You can make this work if you supply a deprioritized instance of Mode.FailFast for the same wrapper type.""".stripMargin } + final case class CollectionFactoryNotFound(struct: Structure.Collection, explanation: String) extends ErrorMessage { + val side: Side = Side.Dest + val span: Span | None.type = None + def render(using Quotes): String = { + s"""Couldn't derive a transformation between collections due to a missing instance of scala.Factory[${struct.paramStruct.tpe.repr.show}, ${struct.tpe.repr.show}]. + |Implicit search failure explanation: $explanation""".stripMargin + } + } + } diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FallibilityRefiner.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FallibilityRefiner.scala index edbbc326..5d828557 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FallibilityRefiner.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FallibilityRefiner.scala @@ -71,7 +71,7 @@ private[ducktape] object FallibilityRefiner { case BetweenNonOptionOption(source, dest, plan) => recurse(plan) - case BetweenCollections(source, dest, plan) => + case BetweenCollections(source, dest, _, plan) => recurse(plan) case BetweenFallibleNonFallible(source, dest, plan) => diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala index f393dc15..f4a31070 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala @@ -139,11 +139,11 @@ private[ducktape] object FalliblePlanInterpreter { Value.Wrapped('{ ${ F.value }.map(${ recurse(plan, source, F).wrapped(F, Type.of[dest]) }, Some.apply) }) } - case Plan.BetweenCollections(source, dest, plan) => + case Plan.BetweenCollections(source, dest, collFactory, plan) => (dest.tpe, source.paramStruct.tpe, dest.paramStruct.tpe) match { case ('[destCollTpe], '[srcElem], '[destElem]) => // TODO: Make it nicer, move this into Planner since we cannot be sure that a factory exists - val factory = Expr.summon[Factory[destElem, destCollTpe]].get + val factory = collFactory.asExprOf[Factory[destElem, destCollTpe]] factory match { case '{ type dest <: Iterable[`destElem`] diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala index 674e5f23..bfc3c986 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala @@ -6,6 +6,7 @@ import io.github.arainko.ducktape.internal.* import scala.collection.immutable.VectorMap import scala.quoted.* import scala.reflect.TypeTest +import scala.collection.Factory private[ducktape] object Fallible private[ducktape] type Fallible = Fallible.type @@ -149,6 +150,7 @@ private[ducktape] object Plan { case class BetweenCollections[+E <: Erroneous, +F <: Fallible]( source: Structure.Collection, dest: Structure.Collection, + factory: Expr[Factory[?, ?]], plan: Plan[E, F] ) extends Plan[E, F] diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanConfigurer.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanConfigurer.scala index ca8c3a2d..6e58f03b 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanConfigurer.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanConfigurer.scala @@ -40,7 +40,7 @@ private[ducktape] object PlanConfigurer { current: Plan[Erroneous, F] ): Plan[Erroneous, F] = current match { - case parent @ BetweenCollections(_, _, plan) => + case parent @ BetweenCollections(_, _, _, plan) => parent.copy(plan = recurse(plan, tail, parent, config)) case parent @ BetweenOptions(_, _, plan) => diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala index 7eeb84c0..6cc2daba 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala @@ -112,12 +112,12 @@ private[ducktape] object PlanInterpreter { '{ Some(${ transformation(sourceValue) }) } } - case Plan.BetweenCollections(source, dest, plan) => + case Plan.BetweenCollections(source, dest, collFactory, plan) => (dest.tpe, source.paramStruct.tpe, dest.paramStruct.tpe) match { case ('[destCollTpe], '[srcElem], '[destElem]) => val sourceValue = value.asExprOf[Iterable[srcElem]] // TODO: Make it nicer, move this into Planner since we cannot be sure that a facotry exists - val factory = Expr.summon[Factory[destElem, destCollTpe]].get + val factory = collFactory.asExprOf[Factory[destElem, destCollTpe]] def transformation(value: Expr[srcElem])(using Quotes): Expr[destElem] = recurse(plan, value).asExprOf[destElem] '{ $sourceValue.map(src => ${ transformation('src) }).to($factory) } } diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanTraverser.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanTraverser.scala index d84240d6..8da62830 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanTraverser.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanTraverser.scala @@ -29,7 +29,7 @@ private[ducktape] trait PlanTraverser[A] { recurse(plan :: next, foldOver(p, accumulator)) case p @ Plan.BetweenNonOptionOption(_, _, plan) => recurse(plan :: next, foldOver(p, accumulator)) - case p @ Plan.BetweenCollections(_, _, plan) => + case p @ Plan.BetweenCollections(_, _, _, plan) => recurse(plan :: next, foldOver(p, accumulator)) case plan: Plan.BetweenSingletons => recurse(next, foldOver(plan, accumulator)) diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala index 48e862b8..d2ce7ec7 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala @@ -8,6 +8,7 @@ import io.github.arainko.ducktape.internal.Summoner.UserDefined.{ FallibleTransf import scala.collection.immutable.VectorMap import scala.quoted.* import scala.util.boundary +import scala.collection.Factory private[ducktape] object Planner { import Structure.* @@ -20,6 +21,10 @@ private[ducktape] object Planner { recurse(source, dest) } + object test { + def unapply(a: Structure, b: Structure): Option[Int] = None + } + private def recurse[F <: Fallible]( source: Structure, dest: Structure, @@ -83,12 +88,24 @@ private[ducktape] object Planner { recurse(source, underlying) ) - case (source @ Collection(_, _, srcParamStruct)) -> (dest @ Collection(_, _, destParamStruct)) => - Plan.BetweenCollections( - source, - dest, - recurse(srcParamStruct, destParamStruct) - ) + case (source @ Collection(_, _, srcParamStruct)) -> (dest @ Collection('[destColl], _, destParamStruct @ Structure('[destElem]))) => + Implicits.search(TypeRepr.of[Factory[destElem, destColl]]) match { + case success: ImplicitSearchSuccess => + Plan.BetweenCollections( + source, + dest, + success.tree.asExprOf[Factory[destElem, destColl]], + recurse(srcParamStruct, destParamStruct) + ) + case failure: ImplicitSearchFailure => + Plan.Error( + source, + dest, + ErrorMessage.CollectionFactoryNotFound(dest, failure.explanation), + None + ) + + } case (source: Product, dest: Product) => planProductTransformation(source, dest) diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Structure.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Structure.scala index 72dd6449..1e2b1fc6 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Structure.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Structure.scala @@ -9,11 +9,13 @@ import scala.deriving.Mirror import scala.quoted.* import scala.reflect.TypeTest -private[ducktape] sealed trait Structure derives Debug { +private[ducktape] sealed trait Structure extends scala.Product derives Debug { def tpe: Type[?] def path: Path + def _1: Type[?] = tpe + final def force: Structure = this match { case lzy: Lazy => lzy.struct.force @@ -24,6 +26,8 @@ private[ducktape] sealed trait Structure derives Debug { } private[ducktape] object Structure { + def unapply(struct: Structure): Structure = struct + def toplevelAny(using Quotes) = Structure.Ordinary(Type.of[Any], Path.empty(Type.of[Any])) def toplevelNothing(using Quotes) = Structure.Ordinary(Type.of[Nothing], Path.empty(Type.of[Nothing])) diff --git a/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala b/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala new file mode 100644 index 00000000..239c1923 --- /dev/null +++ b/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala @@ -0,0 +1,27 @@ +package io.github.arainko.ducktape.issues + +import io.github.arainko.ducktape.* + +class Issue195Suite extends DucktapeSuite { + test("missing Factory instances are reported in a nice way") { + + assertFailsToCompileWith { + """ + import scala.collection.immutable.SortedSet + import io.github.arainko.ducktape.to as convertTo + import io.github.arainko.ducktape.Transformer + + final class MyClassA + final class MyClassB + + given Transformer[MyClassA, MyClassB] = ??? + + val x: SortedSet[MyClassA] = ??? + + val y: SortedSet[MyClassB] = x.convertTo[SortedSet[MyClassB]] + """ + }("""Couldn't derive a transformation between collections due to a missing instance of scala.Factory[MyClassB, scala.collection.immutable.SortedSet[MyClassB]]. +Implicit search failure explanation: no implicit values were found that match type scala.math.Ordering.AsComparable[MyClassB] @ SortedSet[MyClassB]""") + + } +} From 749ca913519c528e0a4f3ecb1fb31293910159eb Mon Sep 17 00:00:00 2001 From: Aleksander Rainko Date: Thu, 22 Aug 2024 19:58:20 +0200 Subject: [PATCH 2/2] improve other errors that don't go through the `Plan` machinery --- .../configuring_transformations.md | 4 ++-- docs/transformation_rules.md | 2 +- .../arainko/ducktape/internal/Configuration.scala | 2 +- .../ducktape/internal/FalliblePlanInterpreter.scala | 1 - .../github/arainko/ducktape/internal/Function.scala | 7 ++++++- .../io/github/arainko/ducktape/internal/Plan.scala | 2 +- .../arainko/ducktape/internal/PlanInterpreter.scala | 1 - .../io/github/arainko/ducktape/internal/Planner.scala | 6 +----- .../arainko/ducktape/issues/Issue195Suite.scala | 11 +++++++---- 9 files changed, 19 insertions(+), 17 deletions(-) diff --git a/docs/total_transformations/configuring_transformations.md b/docs/total_transformations/configuring_transformations.md index aa50c603..e8bd57c4 100644 --- a/docs/total_transformations/configuring_transformations.md +++ b/docs/total_transformations/configuring_transformations.md @@ -241,7 +241,7 @@ Docs.printCode( card .into[PaymentBand] .transform( - Field.computed(_.color, card => if (card.digits % 2 == 0) "green" else "yellow") + Field.computed(_.color, card => if card.digits % 2 == 0 then "green" else "yellow") ) ``` @:choice(generated) @@ -252,7 +252,7 @@ Docs.printCode( card .into[PaymentBand] .transform( - Field.computed(_.color, card => if (card.digits % 2 == 0) "green" else "yellow") + Field.computed(_.color, card => if card.digits % 2 == 0 then "green" else "yellow") ) ) ``` diff --git a/docs/transformation_rules.md b/docs/transformation_rules.md index d74f2c8f..906409fc 100644 --- a/docs/transformation_rules.md +++ b/docs/transformation_rules.md @@ -53,7 +53,7 @@ case class Positive private (value: Int) object Positive { given Transformer.Fallible[[a] =>> Either[String, a], Int, Positive] = - int => if (int < 0) Left("Lesser or equal to 0") else Right(Positive(int)) + int => if int < 0 then Left("Lesser or equal to 0") else Right(Positive(int)) } Mode.FailFast.either[String].locally { diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Configuration.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Configuration.scala index 95980412..5a185053 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Configuration.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Configuration.scala @@ -123,7 +123,7 @@ private[ducktape] object Configuration { Varargs .unapply(configs) - .get // TODO: Make it nicer + .getOrElse(report.errorAndAbort("All of the transformation configs need to be inlined", configs)) .map(expr => parser.applyOrElse(expr.asTerm, fallback)) .toList } diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala index f4a31070..0692aa1e 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/FalliblePlanInterpreter.scala @@ -142,7 +142,6 @@ private[ducktape] object FalliblePlanInterpreter { case Plan.BetweenCollections(source, dest, collFactory, plan) => (dest.tpe, source.paramStruct.tpe, dest.paramStruct.tpe) match { case ('[destCollTpe], '[srcElem], '[destElem]) => - // TODO: Make it nicer, move this into Planner since we cannot be sure that a factory exists val factory = collFactory.asExprOf[Factory[destElem, destCollTpe]] factory match { case '{ diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Function.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Function.scala index 21f9183a..01d04f2e 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Function.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Function.scala @@ -86,7 +86,12 @@ private[ducktape] object Function { )(using Quotes) = { import quotes.reflect.* - val func = fromExpr(function).getOrElse(report.errorAndAbort(s"Couldn't construct a function TODO ERROR MESSAGE")) + val func = fromExpr(function).getOrElse( + report.errorAndAbort( + s"Couldn't encode a function as a type. Please make sure you're passing in an eta-expanded method.", + function + ) + ) func.encodeAsType(initial) } diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala index bfc3c986..ae1b6875 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Plan.scala @@ -3,10 +3,10 @@ package io.github.arainko.ducktape.internal import io.github.arainko.ducktape.* import io.github.arainko.ducktape.internal.* +import scala.collection.Factory import scala.collection.immutable.VectorMap import scala.quoted.* import scala.reflect.TypeTest -import scala.collection.Factory private[ducktape] object Fallible private[ducktape] type Fallible = Fallible.type diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala index 6cc2daba..1ded8e5f 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/PlanInterpreter.scala @@ -116,7 +116,6 @@ private[ducktape] object PlanInterpreter { (dest.tpe, source.paramStruct.tpe, dest.paramStruct.tpe) match { case ('[destCollTpe], '[srcElem], '[destElem]) => val sourceValue = value.asExprOf[Iterable[srcElem]] - // TODO: Make it nicer, move this into Planner since we cannot be sure that a facotry exists val factory = collFactory.asExprOf[Factory[destElem, destCollTpe]] def transformation(value: Expr[srcElem])(using Quotes): Expr[destElem] = recurse(plan, value).asExprOf[destElem] '{ $sourceValue.map(src => ${ transformation('src) }).to($factory) } diff --git a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala index d2ce7ec7..c612badc 100644 --- a/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala +++ b/ducktape/src/main/scala/io/github/arainko/ducktape/internal/Planner.scala @@ -5,10 +5,10 @@ import io.github.arainko.ducktape.internal.Context.{ PossiblyFallible, Total } import io.github.arainko.ducktape.internal.Plan.{ Derived, UserDefined } import io.github.arainko.ducktape.internal.Summoner.UserDefined.{ FallibleTransformer, TotalTransformer } +import scala.collection.Factory import scala.collection.immutable.VectorMap import scala.quoted.* import scala.util.boundary -import scala.collection.Factory private[ducktape] object Planner { import Structure.* @@ -21,10 +21,6 @@ private[ducktape] object Planner { recurse(source, dest) } - object test { - def unapply(a: Structure, b: Structure): Option[Int] = None - } - private def recurse[F <: Fallible]( source: Structure, dest: Structure, diff --git a/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala b/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala index 239c1923..b889b821 100644 --- a/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala +++ b/ducktape/src/test/scala/io/github/arainko/ducktape/issues/Issue195Suite.scala @@ -5,7 +5,7 @@ import io.github.arainko.ducktape.* class Issue195Suite extends DucktapeSuite { test("missing Factory instances are reported in a nice way") { - assertFailsToCompileWith { + assertFailsToCompileContains { """ import scala.collection.immutable.SortedSet import io.github.arainko.ducktape.to as convertTo @@ -20,8 +20,11 @@ class Issue195Suite extends DucktapeSuite { val y: SortedSet[MyClassB] = x.convertTo[SortedSet[MyClassB]] """ - }("""Couldn't derive a transformation between collections due to a missing instance of scala.Factory[MyClassB, scala.collection.immutable.SortedSet[MyClassB]]. -Implicit search failure explanation: no implicit values were found that match type scala.math.Ordering.AsComparable[MyClassB] @ SortedSet[MyClassB]""") - + }( + // TODO: this is weird, the type in the explanation is different in CI ('java.util.Comparator[MyClassB]') as opposed to running it locally ('scala.math.Ordering.AsComparable[MyClassB]') + """Couldn't derive a transformation between collections due to a missing instance of scala.Factory[MyClassB, scala.collection.immutable.SortedSet[MyClassB]]. +Implicit search failure explanation: no implicit values were found that match type""" + ) + } }