Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FallibleTransformer derivation for coproducts #69

Merged
merged 6 commits into from
Jul 7, 2023

Conversation

gregor-i
Copy link
Contributor

@gregor-i gregor-i commented Jul 6, 2023

This PR adds derivation rules for fallible transforms of coproduct.

Closes #67

@arainko
Copy link
Owner

arainko commented Jul 6, 2023

This looks fantastic.

scalafix seems to be failing, could you please run scalafixAll inside your sbt shell (and probably scalafmtAll afterwards) to give it another go?

Copy link
Owner

@arainko arainko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just those two nitpicks, otherwise it looks gorgeous, thank you for spending your time on this 😄

}
}

private def ifStatement(using Quotes)(branches: List[IfBranch]): quotes.reflect.Term = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we use duplicates of this, IfBranch and IsInstanceOf inside CoproductTransformations as well, would you mind extracting these into a separate and reusable object? eg. IfStatement that'd house IfBranch and other things related to this inside

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extracted the IfBranch logic into its own reusable object ExhaustiveCoproductMatching. Maybe I've went a bit to far there. What do you say?

(source.tpe -> dest.tpe) match {
case '[src] -> '[dest] =>
val value =
source.fallibleTransformerTo[F](dest).map {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an optimization we can do here where we match on the summoned transformer and if it is a FallibleTransformer.fallibleFromTotal we can strip apart the total transformer with LiftTransformation.liftTransformation which will make it so that the body of the transformer is inlined instead of creating a transformer instance at runtime. Example of how to do it is here:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've achieved what you had suggested. I had problems getting a test case to match FallibleTransformer.fallibleFromTotal. But I've added an optimization to use total transformers, if available. The cost for that optimization is a second implicit search. I'm not sure if that's worth it.

* Extract duplicated If-Branches construct into its own object
* Optimize transformation logic if an total transformer is available
Copy link
Owner

@arainko arainko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just this one thing to delete and I think we're golden 😸

'{ $F.pure[Dest]($singleton.asInstanceOf[Dest]) }
}

tryTotalTransformation
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think we don't need this as this is supposed to be covered in 'FallibleTransformer.fallibleFromTotal` as for why you couldn't get it to actually work... Maybe I just screwed up instance prioritization for these, I faintly remember having some issues when priotitizing 'fallibleFromTotal' over proper fallible transformers.

Let's get rid of the tryTotalTransformation case and only leave out tryFallibleTransformation and I'll try to take a look at the prioritization issues afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Owner

@arainko arainko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you VERY much for your contribution.

@arainko arainko merged commit 674641a into arainko:master Jul 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FallibleTransformer derivation for coproducts
2 participants