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

arrow.generic.Coproduct arities #954

Merged
merged 26 commits into from Aug 21, 2018
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3df4dff
Initial adding of Coproduct and Tests, I'm sure this will change
abergfeld Jul 27, 2018
b547976
Remove comment
abergfeld Jul 27, 2018
8b37abd
Add GenericHolder generation
abergfeld Aug 6, 2018
7231626
Leverage GenericHolders to preserve generics for the coproduct
abergfeld Aug 6, 2018
12a5894
Leverage other constructor methods
abergfeld Aug 6, 2018
8bec622
Remove GenericHolder concept and just generate Coproduct classes inst…
abergfeld Aug 6, 2018
887a8d0
Add type safe select where the generic has to be part of the Coproduc…
abergfeld Aug 6, 2018
0dab2ea
We don't need to test cases that don't compile anymore!
abergfeld Aug 6, 2018
cb1dac3
Create a generator that only uses strings and files
abergfeld Aug 6, 2018
664e7df
Remove KotlinPoet and old generator
abergfeld Aug 6, 2018
34fb264
Update tests
abergfeld Aug 6, 2018
dc23394
Add todo list
abergfeld Aug 6, 2018
c452385
Cleanup tests / generated code
abergfeld Aug 6, 2018
ab6a34a
Update to use sealed classes instead of Any? because the only way to …
abergfeld Aug 8, 2018
15406ca
Merge branch 'master' into coproducts
abergfeld Aug 8, 2018
3bd508c
I think this is failing the build?
abergfeld Aug 8, 2018
6f4a33f
Clean up generator. We actually don't need generics declared on the s…
abergfeld Aug 8, 2018
469cab3
Internal data class
abergfeld Aug 9, 2018
9e33461
No more map
abergfeld Aug 17, 2018
8ab2d98
Back to KotlinPoet
abergfeld Aug 17, 2018
5fcc872
Merge branch 'master' into coproducts
abergfeld Aug 17, 2018
8c62b46
Pull out some helpers
abergfeld Aug 17, 2018
d97ccc1
Suppress dummy params and hopefully pass detekt
abergfeld Aug 17, 2018
f25abc1
Add types for all the things
abergfeld Aug 17, 2018
569e6fd
Add test verifying that the generator generates code
abergfeld Aug 21, 2018
87ba31c
Merge branch 'master' into coproducts
abergfeld Aug 21, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
Expand Up @@ -25,6 +25,7 @@ buildscript {
daggerVersion = '2.15'
kotlinxCoroutinesVersion = '0.23.3'
kotlinxCollectionsImmutableVersion = '0.1'
kotlinPoetVersion='1.0.0-RC1'
}

repositories {
Expand Down
1 change: 1 addition & 0 deletions modules/core/arrow-annotations-processor/build.gradle
Expand Up @@ -5,6 +5,7 @@ apply plugin: 'kotlin-kapt'

dependencies {
compile "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion"
compile "com.squareup:kotlinpoet:$kotlinPoetVersion"
Copy link
Member

Choose a reason for hiding this comment

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

compileOnly?

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 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

Copy link
Member

Choose a reason for hiding this comment

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

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

compile project(':arrow-annotations')
compile 'me.eugeniomarletti.kotlin.metadata:kotlin-metadata:1.4.0'
compileOnly 'com.google.auto.service:auto-service:1.0-rc4'
Expand Down
@@ -0,0 +1,194 @@
package arrow.generic

import com.squareup.kotlinpoet.AnnotationSpec
import com.squareup.kotlinpoet.ClassName
import com.squareup.kotlinpoet.FileSpec
import com.squareup.kotlinpoet.FunSpec
import com.squareup.kotlinpoet.KModifier
import com.squareup.kotlinpoet.LambdaTypeName
import com.squareup.kotlinpoet.ParameterSpec
import com.squareup.kotlinpoet.ParameterizedTypeName
import com.squareup.kotlinpoet.PropertySpec
import com.squareup.kotlinpoet.TypeSpec
import com.squareup.kotlinpoet.TypeVariableName
import java.io.File
import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy

private val genericsToClassNames = mapOf(
"A" to "First",
"B" to "Second",
"C" to "Third",
"D" to "Fourth",
"E" to "Fifth",
"F" to "Sixth",
"G" to "Seventh",
"H" to "Eighth",
"I" to "Ninth",
"J" to "Tenth",
"K" to "Eleventh",
"L" to "Twelfth",
"M" to "Thirteenth",
"N" to "Fourteenth",
"O" to "Fifteenth",
"P" to "Sixteenth",
"Q" to "Seventeenth",
"R" to "Eighteenth",
"S" to "Nineteenth",
"T" to "Twentieth",
"U" to "TwentyFirst",
"V" to "TwentySecond"
)

fun generateCoproducts(destination: File) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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 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

Copy link
Member

Choose a reason for hiding this comment

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

  • 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

for (size in 2 until genericsToClassNames.size + 1) {
val generics = genericsToClassNames.keys.toList().take(size)

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

Choose a reason for hiding this comment

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

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

addCoproductOfConstructors(generics)
addCopExtensionConstructors(generics)
addSelectFunctions(generics)
addFoldFunction(generics)
}
.build()
.writeTo(destination)
}
}

private fun FileSpec.Builder.addCoproductClassDeclaration(generics: List<String>) {
Copy link
Member

Choose a reason for hiding this comment

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

nice use of receiver extension functions

addType(
TypeSpec.classBuilder("Coproduct${generics.size}")
.addModifiers(KModifier.SEALED)
.addTypeVariables(generics.map { TypeVariableName(it) })
.build()
)

for (generic in generics) {
addType(
TypeSpec.classBuilder(genericsToClassNames[generic]!!)
.addModifiers(KModifier.INTERNAL, KModifier.DATA)
.addTypeVariables(generics.toTypeParameters())
.superclass(parameterizedCoproductNClassName(generics))
.addProperty(
PropertySpec.builder(generic.toLowerCase(), TypeVariableName(generic))
.initializer(generic.toLowerCase())
.build()
)
.primaryConstructor(
FunSpec.constructorBuilder()
.addParameter(generic.toLowerCase(), TypeVariableName(generic))
.build()
)
.build()
)
}
}

private fun FileSpec.Builder.addCoproductOfConstructors(generics: List<String>) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (generic in generics) {
val additionalParameterCount = generics.indexOf(generic)

addFunction(
FunSpec.builder("coproductOf")
.addAnnotations(additionalParameterSuppressAnnotation(additionalParameterCount))
.addTypeVariables(generics.toTypeParameters())
.addParameter(generic.toLowerCase(), TypeVariableName(generic))
.addParameters(additionalParameterSpecs(additionalParameterCount))
.addStatement("return ${genericsToClassNames[generic]}(${generic.toLowerCase()})")
.returns(parameterizedCoproductNClassName(generics))
.build()
)
}
}

private fun FileSpec.Builder.addCopExtensionConstructors(generics: List<String>) {
for (generic in generics) {
val additionalParameterCount = generics.indexOf(generic)

addFunction(
FunSpec.builder("cop")
.addAnnotations(additionalParameterSuppressAnnotation(additionalParameterCount))
.receiver(TypeVariableName(generic))
.addTypeVariables(generics.toTypeParameters())
.addParameters(additionalParameterSpecs(additionalParameterCount))
.addStatement("return coproductOf<${generics.joinToString(separator = ", ")}>(this)")
.returns(parameterizedCoproductNClassName(generics))
.build()
)
}
}

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

Choose a reason for hiding this comment

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

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)

addImport("arrow.core", "toOption")

for (generic in generics) {
val receiverGenerics = generics
.map { if (it == generic) TypeVariableName(generic) else TypeVariableName("*") }
.toTypedArray()
val additionalParameterCount = generics.indexOf(generic)

addFunction(
FunSpec.builder("select")
.addAnnotations(additionalParameterSuppressAnnotation(additionalParameterCount))
.addTypeVariable(TypeVariableName(generic))
.receiver(ClassName("", "Coproduct${generics.size}").parameterizedBy(*receiverGenerics))
.addParameters(additionalParameterSpecs(additionalParameterCount))
.returns(ClassName("arrow.core", "Option").parameterizedBy(TypeVariableName(generic)))
.addStatement("return (this as? ${genericsToClassNames[generic]})?.${generic.toLowerCase()}.toOption()")
.build()
)
}
}

private fun FileSpec.Builder.addFoldFunction(generics: List<String>) {
addFunction(
FunSpec.builder("fold")
.receiver(parameterizedCoproductNClassName(generics))
.addTypeVariables(generics.toTypeParameters() + TypeVariableName("RESULT"))
.addParameters(
generics.map {
ParameterSpec.builder(
it.toLowerCase(),
LambdaTypeName.get(
parameters = listOf(ParameterSpec.unnamed(TypeVariableName(it))),
returnType = TypeVariableName("RESULT")
)
).build()
}
)
.returns(TypeVariableName("RESULT"))
.apply {
beginControlFlow("return when (this)")
for (generic in generics) {
addStatement("is ${genericsToClassNames[generic]} -> ${generic.toLowerCase()}(this.${generic.toLowerCase()})")
}
endControlFlow()
}
.build()
)
}

private fun List<String>.toTypeParameters() = map { TypeVariableName(it) }

private fun parameterizedCoproductNClassName(generics: List<String>): ParameterizedTypeName =
ClassName("", "Coproduct${generics.size}")
Copy link
Member

Choose a reason for hiding this comment

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

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

.parameterizedBy(*generics.map { TypeVariableName(it) }.toTypedArray())

private fun additionalParameterSuppressAnnotation(count: Int): List<AnnotationSpec> =
if (count > 0) {
listOf(
AnnotationSpec.builder(Suppress::class)
.addMember("\"UNUSED_PARAMETER\"")
.build()
)
} else {
emptyList()
}

private fun additionalParameterSpecs(count: Int): List<ParameterSpec> = List(count) {
ParameterSpec.builder("dummy$it", Unit::class)
.defaultValue("Unit")
.build()
}
@@ -0,0 +1,24 @@
package arrow.generic

import com.google.auto.service.AutoService
import arrow.common.utils.AbstractProcessor
import arrow.coproduct
import java.io.File
import javax.annotation.processing.Processor
import javax.annotation.processing.RoundEnvironment
import javax.lang.model.SourceVersion
import javax.lang.model.element.TypeElement

@AutoService(Processor::class)
class CoproductProcessor : AbstractProcessor() {

override fun getSupportedSourceVersion(): SourceVersion = SourceVersion.latestSupported()

override fun getSupportedAnnotationTypes(): Set<String> = setOf(coproduct::class.java.canonicalName)

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

generateCoproducts(generatedDir)
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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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

Copy link
Member

Choose a reason for hiding this comment

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

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

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'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.

}
}
@@ -0,0 +1,3 @@
package arrow

annotation class coproduct
@@ -0,0 +1,9 @@
package arrow.generic

import arrow.coproduct

/**
* This annotated class exists to trigger the code generation.
*/
@coproduct
private object Coproduct
@@ -0,0 +1,84 @@
package arrow.generic

import arrow.core.None
import arrow.core.Option
import arrow.core.Some
import arrow.core.some
import arrow.generic.coproduct2.Coproduct2
import arrow.generic.coproduct2.cop
import arrow.generic.coproduct2.coproductOf
import arrow.generic.coproduct2.fold
import arrow.generic.coproduct2.select
import arrow.generic.coproduct22.Coproduct22
import arrow.generic.coproduct3.cop
import arrow.generic.coproduct3.fold
import arrow.generic.coproduct3.select
import arrow.test.UnitSpec
import io.kotlintest.KTestJUnitRunner
import io.kotlintest.matchers.shouldBe
import org.junit.runner.RunWith

@RunWith(KTestJUnitRunner::class)
class CoproductTest : UnitSpec() {
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 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

Copy link
Member

Choose a reason for hiding this comment

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

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

init {
"Coproducts should be generated up to 22" {
var two: Coproduct2<Unit, Unit>? = null
var twentytwo: Coproduct22<Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit, Unit>? = null
}

"select should return None if value isn't correct type" {
val coproduct2 = "String".cop<String, Long>()

coproduct2.select<Long>() shouldBe None
}

"select returns Some if value is correct type" {
val coproduct2 = "String".cop<String, Long>()

coproduct2.select<String>() shouldBe Some("String")
}

"coproductOf(A) should equal cop<A, B>()" {
"String".cop<String, Long>() shouldBe coproductOf<String, Long>("String")
}

"Coproduct2 fold" {
val coproduct2 = 100L.cop<Long, Int>()

coproduct2.fold(
{ "Long$it" },
{ "Int$it" }
) shouldBe "Long100"
}

"Coproduct3 should handle multiple nullable types" {
val value: String? = null
val coproduct3 = value.cop<Long, Float?, String?>()

coproduct3.select<Long>() shouldBe None
coproduct3.select<Float?>() shouldBe None
coproduct3.select<String?>() shouldBe None

coproduct3.fold(
{ "First" },
{ "Second" },
{ "Third" }
) shouldBe "Third"
}

"Coproduct3 should handle multiple types with generics" {
val value: Option<String> = "String".some()
val coproduct3 = value.cop<Option<Long>, Option<Float>, Option<String>>()

coproduct3.select<Option<Long>>() shouldBe None
coproduct3.select<Option<Float>>() shouldBe None
coproduct3.select<Option<String>>() shouldBe value.some()

coproduct3.fold(
{ "First" },
{ "Second" },
{ "Third" }
) shouldBe "Third"
}
}
}