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

`arrow.generic.Coproduct` arities #954

Merged
merged 26 commits into from Aug 21, 2018

Conversation

Projects
None yet
4 participants
@abergfeld
Contributor

abergfeld commented Jul 27, 2018

This is my first open source PR / first PR to Arrow so I'm not quite sure how to do some / most of this stuff haha. Any help is greatly appreciated!

The intent is to add a Coproduct Data type that abstracts over arity. I got some starting guidance from Raul and Jorge that I deviated from a bit, see the comments in the code for details.

Starting point from Raul:

import arrow.core.None
import arrow.core.Option
import arrow.core.Some

sealed class Coproduct
data class Inl<out H, out T : Coproduct>(val head : H) : Coproduct()
data class Inr<out H, out T : Coproduct>(val tail : T) : Coproduct()
sealed class CNil : Coproduct()

/* type aliases as phantom types guarantee static dispatching */
typealias Coproduct2<A, B> = Coproduct
typealias Coproduct3<A, B, C> = Coproduct

/** Coproduct2 factories (should be codegen) **/
fun <A, B> coproductOf(a: A): Coproduct2<A, B> = Inl<A, CNil>(a)
fun <A, B> coproductOf(b: B, dummy: Unit = Unit): Coproduct2<A, B> = Inr<A, Coproduct>(Inl<B, CNil>(b))

/** Coproduct3 factories (should be codegen)  **/
fun <A, B, C> coproductOf(a: A, dummy: Unit = Unit, dummy2: Unit = Unit, dummy3: Unit = Unit): Coproduct3<A, B, C> = Inl<A, CNil>(a)
fun <A, B, C> coproductOf(b: B, dummy: Unit = Unit, dummy2: Unit = Unit, dummy3: Unit = Unit, dummy4: Unit = Unit): Coproduct3<A, B, C> = Inr<A, Coproduct>(Inl<B, CNil>(b))
fun <A, B, C> coproductOf(c: C, dummy: Unit = Unit, dummy2: Unit = Unit, dummy3: Unit = Unit, dummy4: Unit = Unit, dummy5: Unit = Unit): Coproduct3<A, B, C> = Inr<A, Coproduct>(Inr<B, Coproduct>(Inl<C, CNil>(c)))

// uses explicit local mutation because inline reified functions can't be recursive
/** Extracts values from a Coproduct **/
inline fun <reified A> Coproduct.select(): Option<A> {
  var current = this
  while (current is Inr<*, *>) {
    current = current.tail
  }
  return when (current) {
    is Inl<*, *> -> if (current.head is A) Some(current.head as A) else None
    else -> None
  }
}

fun <A, B, C> A.cop(): Coproduct3<A, B, C> = coproductOf<A, B, C>(this)
fun <A, B, C> B.cop(dummy: Unit = Unit): Coproduct3<A, B, C> = coproductOf<A, B, C>(this)
fun <A, B, C> C.cop(dummy: Unit = Unit, dummy2: Unit = Unit): Coproduct3<A, B, C> = coproductOf<A, B, C>(this)
private const val alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
private const val packageName = "arrow.generic"
fun generateCoproducts(destination: File) {

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

This class uses KotlinPoet to generate coproductOf(a: A...N), A.cop<A, B...N>(), right biased map and fold for each of the type alias' up to Coproduct26.

It packages with coproductN to namespace methods because otherwise the dummy:Unit = Unit parameters would be rather large.

Questions:
Should I be using something besides KotlinPoet?
How many Coproducts should be generated?

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

I also feel like I'm doing something wrong with the generics here too:

            val coproduct2: Coproduct2<Long, String> = 6L.cop<Long, String>()

            coproduct2.map<String, String, Int> { it.length } shouldBe 6L.cop<Long, Int>()

This appears to be valid in the editor, but the A type is incorrect for the receiver, I'm not sure if there's a way to deal with that? I feel like I'm doing something wrong here lol

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member
  • As long as you move KotlinPoet to be compileOnly, I don't see any strong reason to not use it if it makes our lives easier on codegen.
  • About how many coproducts needed, I believe 26 (number of characters on the alphabet) is more than enough to start with.
  • The 6L.cop<Long, String>() syntax is a bit hard to understand at the beginning, I think it's just "casting" 6L to be a Coproduct<Long, String> (which would be Long for this case), is it? I'm not sure whether there's a better approach for these castings honestly, so just a comment.

This comment has been minimized.

@raulraja

raulraja Jul 30, 2018

Member

Generating up to 22 is fine, that is what we do for the rest at this time. We don't use Kotlin Poet and do the code the generation manually as in the other processors to avoid introducing extra deps at compile time that users may already have. Having said that I have no strong opinions but would be nice to be consistent with the rest and avoid parts of code written in different styles since we have no plans to migrate all of our processors to KotlinPoet.

This comment has been minimized.

@raulraja

raulraja Jul 30, 2018

Member

There is no biased side in Coproduct and there should be no right biased map or other biased methods. A map over this Coproduct would have to contemplate all cases so it's esentially a specialization of fold that preserves the container. For example: https://github.com/milessabin/shapeless/wiki/Feature-overview:-shapeless-2.0.0#coproducts-and-discriminated-unions The poly function there contemplates all cases in which the Coproduct may find itself.

This comment has been minimized.

@pakoito

pakoito Jul 30, 2018

Member

I'm with @raulraja in not bringing KotlinPoet for now. It's all a bit stringly typed, but it's been good enough for our needs so far :)

override fun onProcess(annotations: Set<TypeElement>, roundEnv: RoundEnvironment) {
val generatedDir = File(this.generatedDir!!, "").also { it.mkdirs() }
generateCoproducts(generatedDir)

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

I looked at other things as examples for this, I'm not sure if this is correct.

Questions:
Should this be used for the generation?
I added a coproduct annotation just to make this work, which felt wrong but I'm not sure how else to do it, is there a better way?

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

Annotations are used to find target elements on the source code that you end up validating and generating code related to those. But here we don't have any targets, but just generic generation. Ideally It should work and be okay to not add any supported annotations to the processor, but I think that's not working?

It's okay to add a dummy annotation if you need to. You can also just use one of the ones we already have for the generics module.

This comment has been minimized.

@abergfeld

abergfeld Aug 6, 2018

Contributor

I wasn't able to see any way to do it without the annotated Coproduct class so I just made Coproduct a private object solely to generate the code off of as it's not used anymore

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

Can we annotate the arrow.generic package instead or does it need to be a TypeElement?

This comment has been minimized.

@abergfeld

abergfeld Aug 17, 2018

Contributor

I'm not sure I follow entirely, I tried override fun getSupportedAnnotationTypes(): Set<String> = setOf("arrow.generic") if that's what you meant and that also didn't trigger generation unfortunately.

import arrow.core.toOption
@coproduct
data class Coproduct(val value: Any?)

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

This is what the sealed class in the description kind of boiled down to. Since we can't check types on instances I wasn't able to leverage anything on the Inl / Inr classes except for the actual value, which ended up needed to be type checked (is A, as A, as? A) and CNil ended up not being used except for generics on the other two classes.

Questions:
I'm not sure if I missed something from the example code? Is there a reason to keep the sealed class recursive structure that I'm not seeing?
Is there a way to make the value only usable inside of Arrow? I tried to do internal and protected but the method extensions complained about that.

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

This one could be answered by @raulraja or @pakoito for sure :)

This comment has been minimized.

@raulraja

raulraja Jul 30, 2018

Member

The encoding is not as important as the fact that people should not be able to use this contructor in code and use the coproductOf factories instead. The encoding I proposed does not allow you to create elements of the wrong type. What does the code expansion of this one look like for a coproduct of 3 elements?

@coproduct
data class Coproduct(val value: Any?)
inline fun <reified A> Coproduct.select(): Option<A> = (value as? A).toOption()

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

select is not generated as it always is the same regardless of generic

import org.junit.runner.RunWith
@RunWith(KTestJUnitRunner::class)
class CoproductTest : UnitSpec() {

This comment has been minimized.

@abergfeld

abergfeld Jul 27, 2018

Contributor

I added some basic tests here, with code generation I really wasn't super sure of what to test.

Questions:
Should I be testing any laws?
Do I need to add any like Instances for something like Coproduct? Basing this question of Option.functor() and things like that, sorry if that's a silly question haha

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

If it's supporting features like map, flatMap and fold shouldn't it at least be tested under Functor, Monad and Foldable laws probably?

@@ -5,6 +5,7 @@ apply plugin: 'kotlin-kapt'
dependencies {
compile "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion"
compile "com.squareup:kotlinpoet:$kotlinPoetVersion"

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

compileOnly?

This comment has been minimized.

@abergfeld

abergfeld Aug 17, 2018

Contributor

I tried it and it didn't generate the files, I'm not sure, I could have something setup incorrectly too because I have to manually add the generated dirs as source dirs still

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

it's fine to be compile because this is the annotation processor and itself it's compile only through kapt to it's dependent projects

private const val alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
private const val packageName = "arrow.generic"
fun generateCoproducts(destination: File) {

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member
  • As long as you move KotlinPoet to be compileOnly, I don't see any strong reason to not use it if it makes our lives easier on codegen.
  • About how many coproducts needed, I believe 26 (number of characters on the alphabet) is more than enough to start with.
  • The 6L.cop<Long, String>() syntax is a bit hard to understand at the beginning, I think it's just "casting" 6L to be a Coproduct<Long, String> (which would be Long for this case), is it? I'm not sure whether there's a better approach for these castings honestly, so just a comment.
override fun onProcess(annotations: Set<TypeElement>, roundEnv: RoundEnvironment) {
val generatedDir = File(this.generatedDir!!, "").also { it.mkdirs() }
generateCoproducts(generatedDir)

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

Annotations are used to find target elements on the source code that you end up validating and generating code related to those. But here we don't have any targets, but just generic generation. Ideally It should work and be okay to not add any supported annotations to the processor, but I think that's not working?

It's okay to add a dummy annotation if you need to. You can also just use one of the ones we already have for the generics module.

import arrow.core.toOption
@coproduct
data class Coproduct(val value: Any?)

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

This one could be answered by @raulraja or @pakoito for sure :)

import arrow.core.toOption
@coproduct
data class Coproduct(val value: Any?)

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

Does it make sense to have an empty coproduct ? (as in your constructor val value: Any?). Is that something we wanna allow or coprocuts should always contain one value for just one of the given N types?

import org.junit.runner.RunWith
@RunWith(KTestJUnitRunner::class)
class CoproductTest : UnitSpec() {

This comment has been minimized.

@JorgeCastilloPrz

JorgeCastilloPrz Jul 30, 2018

Member

If it's supporting features like map, flatMap and fold shouldn't it at least be tested under Functor, Monad and Foldable laws probably?

@JorgeCastilloPrz

This comment has been minimized.

Member

JorgeCastilloPrz commented Jul 30, 2018

I'd reeeeally love to get @pakoito & @raulraja reviewing this. As a disclaimer and just because we've discussed about this in private channels but here other maintainers are not aware of it:

The reason to not use the Coproduct already provided in Arrow, which is no different than the one in other libs like Cats, is because that one is just a binary constructor for 2 higher kinds based on Either. That's not what we're interested on with this.

We wanna have a Coproduct of basic types abstracting over arity, so you can do things like A or B or C or D regardless of where are those types coming from and avoiding the need to use sealed classes for it. For example, if we had 2 subsets of errors:

sealed class ErrorsA {
  Error1 : ErrorsA()
  Error2 : ErrorsA()
}

sealed class ErrorsB {
  Error3 : ErrorsB()
  Error4 : ErrorsB()
  Error5 : ErrorsB()
}

You could mix types of both of them and avoid nesting sealed classes to achieve the same, like:

Coproduct3<Error1, Error4, Error5>

The key point here is provide a powerful way of creating sealed unions without the need for an explicit sealed class (which has also some other constraints like not being able to reuse types on under different sealed hierarchies unless you chain sealed class nesting)

@pakoito pakoito self-requested a review Jul 30, 2018

@raulraja

This comment has been minimized.

Member

raulraja commented Jul 30, 2018

@abergfeld thanks so much for giving this a shot. This is going in the right direction but it's not clear to me how type safe this is in comparison with the original encoding proposed which is inline with what we already have for HList.
Some of the changes we would need are along the lines of:

Thanks!

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 3, 2018

@raulraja @JorgeCastilloPrz Sorry for the lack of activity on the PR, it's been a busy week haha.

In terms of the comparison to HList, one thing I noticed with both the code in the description and what's in the PR is that we're having to resort to checking instances (if (value is A)) at some point. Hlist is able to type alias HlistN to the actual classes HCons / HNil but with Coproduct we're type aliasing CoproductN to just Coproduct which is where I think we're losing the types. With what's in the PR I'm able to do "String".cop<String, Long, Float>().map<Unit, Unit, Unit, ResultType>(...) and I think it's because those generics on the type alias are lost because they're not used on the Coproduct class itself. I'm going to dig in some more to see what's possible, I'm wondering if concrete generated types for all the Coproducts would solve that problem but that's a lot to generate haha

I'll update the code gen to match the other processors and update map to operate like Shapeless' Coproduct does and address other comments after some more exploring, as the code gen potentially will be changing

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 6, 2018

@JorgeCastilloPrz @raulraja @pakoito
Okay so I think I've made some decent progress on this. Here's one of the generated classes:

package arrow.generic.coproduct3

import arrow.core.Option
import arrow.core.toOption
import java.lang.IllegalStateException
import kotlin.Unit

data class Coproduct3<A, B, C> internal constructor(val value: Any?)

fun <A, B, C> coproductOf(a : A) = Coproduct3<A, B, C>(a)
fun <A, B, C> coproductOf(b : B, dummy0: Unit = Unit) = Coproduct3<A, B, C>(b)
fun <A, B, C> coproductOf(c : C, dummy0: Unit = Unit, dummy1: Unit = Unit) = Coproduct3<A, B, C>(c)

fun <A, B, C> A.cop() = coproductOf<A, B, C>(this)
fun <A, B, C> B.cop(dummy0: Unit = Unit) = coproductOf<A, B, C>(this)
fun <A, B, C> C.cop(dummy0: Unit = Unit, dummy1: Unit = Unit) = coproductOf<A, B, C>(this)

inline fun <reified A> Coproduct3<A, *, *>.select(): Option<A> = (value as? A).toOption()
inline fun <reified B> Coproduct3<*, B, *>.select(dummy0: Unit = Unit): Option<B> = (value as? B).toOption()
inline fun <reified C> Coproduct3<*, *, C>.select(dummy0: Unit = Unit, dummy1: Unit = Unit): Option<C> = (value as? C).toOption()

inline fun <reified A, reified B, reified C, RESULT> Coproduct3<A, B, C>.fold(
   a: (A) -> RESULT,
   b: (B) -> RESULT,
   c: (C) -> RESULT
): RESULT {
   return when (value) {
       is A -> a(value)
       is B -> b(value)
       is C -> c(value)
       else -> throw IllegalStateException("Invalid Coproduct3 $this")
   }
}

inline fun <reified A, reified B, reified C, A1, B1, C1> Coproduct3<A, B, C>.map(
   a: (A) -> A1,
   b: (B) -> B1,
   c: (C) -> C1
): Coproduct3<A1, B1, C1> {
   return fold(
       { a(it).cop<A1, B1, C1>() },
       { b(it).cop<A1, B1, C1>() },
       { c(it).cop<A1, B1, C1>() }
   )
}

It's generated using just Strings written to Files, here's some highlight points:

  • Packaged by coproductN (arrow.generic.coproduct3) such that the methods don't need as many dummy0 parameters to make compile
  • Internal constructor for the Coproduct meaning only the code in this file can create one which should ensure that it's created valid as one of A, B, or C even though we hold the value as Any?. I think technically due to erasure that's what A, B, or C would be considered Any? at runtime anyway so as long as we're constructing it correctly everything should be okay with type checking in the other methods.
  • coproductOf / T.cop creator methods both allow ways to create valid Coproducts
  • select should be type safe now, select must be used with a generic listed on the Coproduct (more shown in the example link below)
  • map now acts like Coproduct<A, B, C> -> Coproduct<D, E, F> which I believe is how Shapless' Coproduct behaves as well
  • fold just folds lol

Here's an example I used to try and break the types with: https://gist.github.com/abergfeld/6f29c9103c8cc30be0eb94a5e2fe40c9

I'm still not sure about the testing aspect, should Coproduct be tested using any Law stuff that's built in?

Other than that I think I've address all the comments, let me know what you guys think. :)

abergfeld added some commits Aug 8, 2018

Update to use sealed classes instead of Any? because the only way to …
…make nullableLong.Coproduct<String?, Long?> work is if we know what "index" the value (Long?) is meant for if it's null since type checks (as?) will work for both cases. We also need it for cases where Type Erasure comes up Coproduct<Option<String>, Option<Long>>, at runtime both are just Option so we need better typing around holding values at certain "indexes". This is similar to what Either already does with Left and Right being part of the sealed class.
@codecov

This comment has been minimized.

codecov bot commented Aug 8, 2018

Codecov Report

Merging #954 into master will increase coverage by 0.7%.
The diff coverage is 87.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #954     +/-   ##
===========================================
+ Coverage     46.21%   46.92%   +0.7%     
- Complexity      680      684      +4     
===========================================
  Files           315      318      +3     
  Lines          7932     8072    +140     
  Branches        826      848     +22     
===========================================
+ Hits           3666     3788    +122     
- Misses         3937     3941      +4     
- Partials        329      343     +14
Impacted Files Coverage Δ Complexity Δ
...esources/arrow/ap/objects/coproduct2/Coproduct2.kt 36.36% <36.36%> (ø) 0 <0> (?)
.../src/main/java/arrow/generic/CoproductProcessor.kt 80% <80%> (ø) 4 <4> (?)
.../main/java/arrow/generic/CoproductFileGenerator.kt 92.74% <92.74%> (ø) 0 <0> (?)
...tics/src/main/kotlin/arrow/optics/instances/map.kt 89.18% <0%> (-2.71%) 0% <0%> (ø)

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 4c0eb39...87ba31c. Read the comment docs.

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 8, 2018

Update:
I noticed there were two pretty big issues with the previously mentioned approach:

  1. Multiple nullable types just flat out didn't work. null.cop<String?, Long?> would just always be treated as the String? case because the implementation is using as? which it looks like null can be cast to both String? and Long?
  2. Very similar issue but a little harder to deal with, Type Erasure. Considering the above is a problem, any Type with a generic, List<T>, Option<T> poses the same problem because as run time they're just cast as Option because the generics are gone.

There was only two ways I could see to deal with this, here was my train of thought:

  1. I thought about going back to the initial implementation (the one in the description) but then I remembered the root issues is that we can't typeAlias using Inr / Inl because unlike HList, we only have one value and based upon the "index" of the actual value in the Coproduct<A, B, C> we actually have different types Inl(A)> vs Inr<A, Inl(B)> vs Inr<A, Inr<B, Inl(C)>. I keep feeling like I'm missing something about that approach but I couldn't figure out how to make it work so I kept trying stuff.

  2. I thought about using the dreaded Reflection api and seeing if there was a way to do better type checks but I didn't find much and it seemed like a gross 🤢solution

  3. I kept coming back to the fact that Kotlin has this concept, sealed classes, just not a generic means of it. Either is a generic version of a sealed class with 2 Types (Left, Right) in it's hierarchy so I figured why not embrace that since it solves both problems above and that's really what Coproducts is trying to generify. It's definitely more generated code but so far it seems more solid than the previous attempts.

Here's some of the code that changed in the generator:

sealed class Coproduct3<A, B, C>
data class First<A, B, C>(val a: A): Coproduct3<A, B, C>()
data class Second<A, B, C>(val b: B): Coproduct3<A, B, C>()
data class Third<A, B, C>(val c: C): Coproduct3<A, B, C>()

fun <A, B, C> coproductOf(a : A): Coproduct3<A, B, C> = First<A, B, C>(a)
fun <A, B, C> coproductOf(b : B, dummy0: Unit = Unit): Coproduct3<A, B, C> = Second<A, B, C>(b)
fun <A, B, C> coproductOf(c : C, dummy0: Unit = Unit, dummy1: Unit = Unit): Coproduct3<A, B, C> = Third<A, B, C>(c)

inline fun <reified A> Coproduct3<A, *, *>.select(): Option<A> = (this as? First)?.a.toOption()
inline fun <reified B> Coproduct3<*, B, *>.select(dummy0: Unit = Unit): Option<B> = (this as? Second)?.b.toOption()
inline fun <reified C> Coproduct3<*, *, C>.select(dummy0: Unit = Unit, dummy1: Unit = Unit): Option<C> = (this as? Third)?.c.toOption()

inline fun <reified A, reified B, reified C, RESULT> Coproduct3<A, B, C>.fold(
   a: (A) -> RESULT,
   b: (B) -> RESULT,
   c: (C) -> RESULT
): RESULT {
   return when (this) {
       is First -> a(this.a)
       is Second -> b(this.b)
       is Third -> c(this.c)
   }
}

@abergfeld abergfeld changed the title from [WIP] Coproducts to Coproducts Aug 8, 2018

abergfeld added some commits Aug 8, 2018

Clean up generator. We actually don't need generics declared on the s…
…ubclasses in the coproductOf methods AND we don't need inline functions with generics anymore!
@JorgeCastilloPrz

This comment has been minimized.

Member

JorgeCastilloPrz commented Aug 16, 2018

I think we need @raulraja or @pakoito to bring some light here to try to get this closer to integration since it's getting a bit stuck. Coproduct seems promising and we'll get a big benefit of having it.

I can talk about the operations. I'd expect to get map, fold, flatMap (to sequence ops returning coproducts, not sure if requiring to be coproducts of the same arity?) and maybe combine so Coproduc2<A, B>.combine(Coproduct2<C, D>) can result on Coproduct4<A, B, C, D> (Semigroup instance). Which leads me to think about how this encoding can support providing Typeclass instances.

Is this encoding accepting the @higherkind annotation and the @instance for instances? It looks like it's a data type so it should be considered an HK and also be able to abstract over it like F<A, B, C, D> and make programs polymorphic over its form instead of it being a fixed type on those programs.

@raulraja

This comment has been minimized.

Member

raulraja commented Aug 16, 2018

Coproduct should not have flatMap or map because there is no bias. As for the new encoding it may work though is a bunch of classes and the ops would have to be replicated on each ADT so it may turn out to be verbose and bytecode heavy for a Cop22. @abergfeld what is the issue that you were finding with the encoding based on InL and InR maybe if you have some code snippets or something that shows the problem we can try to address it. Also I changed my opinion about KotlinPoet since I was able to reify an entire tree of a TypeElement into a TypeSpec and we can use already parsed KotlinPoet tree to bootstrap codegen. I plan on submitting some changes to the processors in the next week that will introduce a MetaProcessor that is able to parse TypeElement as TypeSpec.

abergfeld added some commits Aug 17, 2018

@JorgeCastilloPrz

Looks good to me. Great work @abergfeld 👏 👍

Thinking twice and just for next time, I'd probably pick hardcoded style instead of code generation when possible, as the one used for map or tupled in Applicative for example, (as @raulraja mentioned the other day).

The major rationale is that annotation processors increase compile times a lot. There's an extended complaint in the air lately on the Kotlin community where people is trying to move out from annotation processors as much as possible, (Not always possible), and libraries like Dagger are interestingly under higher "pressure" by devs because of that. I'm aware that we can't avoid using those for all the features (at least for now), still I'd think about this fact twice when possible.

Not a blocker for this one or anything, I believe we should merge this and iterate, therefore we can start using it on our project as soon as there's a new release.

@raulraja

Excelent work @abergfeld. Would be nice to have in the future some docs and usage example in it's own doc section similar to what we have for Product types https://arrow-kt.io/docs/generic/product/

Thanks for this awesome contribution. Left some comments but nothing blocking once CI is green.

Cheers! 🍻

@@ -5,6 +5,7 @@ apply plugin: 'kotlin-kapt'
dependencies {
compile "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion"
compile "com.squareup:kotlinpoet:$kotlinPoetVersion"

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

it's fine to be compile because this is the annotation processor and itself it's compile only through kapt to it's dependent projects

FileSpec.builder("arrow.generic.coproduct$size", "Coproduct$size")
.apply {
addCoproductClassDeclaration(generics)

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

We may want to move some of these functions down the road to a common place for other processors to have them available.

}
}
private fun FileSpec.Builder.addCoproductClassDeclaration(generics: List<String>) {

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

nice use of receiver extension functions

}
}
private fun FileSpec.Builder.addCoproductOfConstructors(generics: List<String>) {

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

please type all functions regardless of them being private or overrides. We prefer not to rely on type inference in Arrow since typed functions serve as documentation and are explicit as to what they do.

This comment has been minimized.

@abergfeld

abergfeld Aug 17, 2018

Contributor

Oh gotcha, yeah I can add the Unit returns on these. I suppose I could return this and axe the apply too

}
private fun FileSpec.Builder.addSelectFunctions(generics: List<String>) {
addImport("arrow.core", "Option")

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

For commonly used package like arrow.core would be nice to have constants or properties in some utilities so they are shared across the processors.

addImport(Packages.arrow.core)
private fun List<String>.toTypeParameters() = map { TypeVariableName(it) }
private fun parameterizedCoproductNClassName(generics: List<String>): ParameterizedTypeName =
ClassName("", "Coproduct${generics.size}")

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

I find the indentation excessive. Not sure if it's your IDE or detekt configuration.

override fun onProcess(annotations: Set<TypeElement>, roundEnv: RoundEnvironment) {
val generatedDir = File(this.generatedDir!!, "").also { it.mkdirs() }
generateCoproducts(generatedDir)

This comment has been minimized.

@raulraja

raulraja Aug 17, 2018

Member

Can we annotate the arrow.generic package instead or does it need to be a TypeElement?

@raulraja raulraja changed the title from Coproducts to `arrow.generic.Coproduct` arities Aug 17, 2018

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 17, 2018

@JorgeCastilloPrz @raulraja Thanks guys! I can try to write some docs if you'd like. I assume that'd be a separate PR?

@raulraja

This comment has been minimized.

Member

raulraja commented Aug 17, 2018

@abergfeld yeah, another PR is fine, thanks!

@JorgeCastilloPrz

This comment has been minimized.

Member

JorgeCastilloPrz commented Aug 17, 2018

Docs would be super awesome also but feel free to do it on a separate PR

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 17, 2018

@JorgeCastilloPrz @raulraja Best I can tell for the codecov checks, it's saying that the CoproductProcessor and CoproductFileGenerator are both new files / code that isn't covered. Think it's worth moving to hardcoded Coproducts and tests for each one and removing the code generation to get code coverage passing? I'm not seeing tests around other generators / processors, unless it knows that the generated code is tested?

@raulraja

This comment has been minimized.

Member

raulraja commented Aug 17, 2018

@abergfeld you can add some tests for the processors similar to these ones https://github.com/arrow-kt/arrow/blob/master/modules/core/arrow-annotations-processor/src/test/java/arrow/ap/tests/IsoTest.kt That would probably take care of the codecov

abergfeld added some commits Aug 21, 2018

@abergfeld

This comment has been minimized.

Contributor

abergfeld commented Aug 21, 2018

@raulraja @JorgeCastilloPrz I got a generator test implemented, had to add a hook to allow for specifying the location of the actual result file, but CI is passing 🎉 Let me know if there's anything else I should address :)

@raulraja

This comment has been minimized.

Member

raulraja commented Aug 21, 2018

@abergfeld great work and thanks so much for this contribution!

@raulraja raulraja merged commit dac4cf3 into arrow-kt:master Aug 21, 2018

3 checks passed

codecov/patch 87.85% of diff hit (target 46.21%)
Details
codecov/project 46.92% (+0.7%) compared to 4c0eb39
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JorgeCastilloPrz

This comment has been minimized.

Member

JorgeCastilloPrz commented Aug 21, 2018

Great work! 👏👏

@raulraja raulraja referenced this pull request Nov 2, 2018

Merged

Release 0.8.0 #1080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment