Improve Optics DSL with support for sum types and enable Each/At #803

nomisRev · 2018-04-17T17:06:05Z

Quite a lot of stuff here :)

This will enable generation of DSL for sum types.

i.e. A (part of) JSON ADT

@optics sealed class Json 
@optics data class JsString(val string: string): Json()
@optics sealed class JsNumber : Json()  
@optics data class JsInt(val int: Int) : Json() 
@optics data class JsDouble(val double: Double) : Json() 
@optics data class JsArray(val value: List<Json>) : Json()

val jsArrayEach: Each<JsArray, Json> = object : Each<JsArray, Json> {
  override fun each(): Traversal<JsArray, Json> = jsArrayIso() compose ListTraversal()
}

val json: Json = JsArray(listOf(
  JsInt(1),
  JsString("Hello"),
  JsInt(2),
  JsInt(3),
  JsDouble(4.0)
))

json.setter().jsArray.every(jsArrayEach).jsInt.int.modify { it + 1 }
//JsArray(value=[JsInt(int=2), JsString(value=Hello), JsInt(int=3), JsInt(int=4), JsDouble(double=4.0)])

This also already shows the power of every which relies on Each. The name every comes from a discussion a while back for Helios, back then we still had lookups. Also in Helios we can hide Each from the user. I am however unsure if we should still name it every over each since the use of Each is much more prominent now and I'd prefer not to rename the Each typeclass. Thoughts?

Use of At:

@optics data class Db(val content: MapK<Keys, String>)

sealed class Keys
object One : Keys()
object Two : Keys()
object Three : Keys()
object Four : Keys()

val db = Db(mapOf(
  One to "one",
  Two to "two",
  Three to "three",
  Four to "four"
).k())

db.setter().content.at(MapK.at(), One).some.modify(String::reversed)
//Db(content=MapK(map={arrow.optics.One@a09ee92=eno, arrow.optics.Two@30f39991=two, arrow.optics.Three@452b3a41=three, arrow.optics.Four@4a574795=four}))

Finally I also did an attempt to improve error messages in the code gen.

@pakoito I did not add any way to enable each.run { data.list.every() } since I thought it didn't belong in the Eachtypeclass.

codecov · 2018-04-17T17:23:18Z

Codecov Report

Merging #803 into master will increase coverage by 0.12%.
The diff coverage is 58.9%.

@@             Coverage Diff              @@
##             master     #803      +/-   ##
============================================
+ Coverage     43.44%   43.57%   +0.12%     
- Complexity      582      584       +2     
============================================
  Files           282      284       +2     
  Lines          7211     7245      +34     
  Branches        812      809       -3     
============================================
+ Hits           3133     3157      +24     
- Misses         3791     3802      +11     
+ Partials        287      286       -1

Impacted Files	Coverage Δ	Complexity Δ
...tics/src/main/kotlin/arrow/optics/instances/set.kt	`100% <ø> (ø)`	`0 <0> (?)`
...ics/src/main/kotlin/arrow/optics/typeclasses/At.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...src/main/java/arrow/common/utils/ProcessorUtils.kt	`18.07% <0%> (-0.23%)`	`0 <0> (ø)`
...rc/main/kotlin/arrow/optics/instances/sequencek.kt	`45.45% <0%> (-25.98%)`	`0 <0> (ø)`
...src/main/java/arrow/optics/BoundSetterGenerator.kt	`20% <0%> (ø)`	`2 <0> (ø)`	⬇️
...tics/src/main/kotlin/arrow/optics/syntax/option.kt	`0% <0%> (ø)`	`0 <0> (?)`
...s/src/main/kotlin/arrow/optics/typeclasses/Each.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...tics/src/main/kotlin/arrow/optics/instances/try.kt	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...s/src/main/kotlin/arrow/optics/instances/option.kt	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...ics/src/main/kotlin/arrow/optics/instances/list.kt	`90.47% <100%> (ø)`	`0 <0> (?)`
... and 14 more

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 0ce64f7...ac707b1. Read the comment docs.

JorgeCastilloPrz · 2018-04-17T21:36:30Z

modules/core/arrow-annotations-processor/src/main/java/arrow/common/utils/ProcessorUtils.kt

    return metadata.asClassOrPackageDataWrapper(classElement)
-      ?: knownError("This annotation can't be used on this element")
+      ?: knownError("Arrow's annotation can't be used on $classElement")


can't you get the potential binary name for this element (class name) from the TypeElement to include it in the error instead of the TypeElement instance reference? (ProcessingEnvironment.getElementUtils().getBinaryName(TypeElement)).

Actually for

@instance(String::class) interface StringEqInstance : Eq<String> { override fun String.eqv(b: String): Boolean = this == b }

it prints e: error: Arrow's annotations can only be used on Kotlin classes. Not valid for java.lang.String. Not sure if that's what you wanted?

toString() is correctly implemented for Element and it thus prints the class names.

no, I was expecting to get the same but for the annotated class itself, like arrow.StringEqInstance. Are you calling it for the classElement argument?

Yes, that makes sense. We need to improve error message.

I tried to write an Each instance for String and got This annotation can't be used on this element. So I made a small change to improve what we currently have. Problem with a general error message like this is that we can't point the user in the right direction. So I would go as far as provide clear custom messages for each error case.

In this case something like.

e: error: @instance cannot be used for java.lang.String (or even better kotlin.String) interface StringEqInstance : Eq<String> ^ Cannot generate instance method for arrow.data.instances.StringEqInstance.

JorgeCastilloPrz · 2018-04-17T21:40:03Z

modules/core/arrow-annotations-processor/src/main/java/arrow/optics/BoundSetterGenerator.kt

        |/**
        | * @receiver [${annotatedOptic.sourceClassName.removeBackticks()}] the instance you want to bind the dsl on.
        | * @return [$boundSetter] an intermediate optics that is bound to the instance.
        | */
        |fun ${annotatedOptic.sourceClassName}.setter() = $boundSetter(this, arrow.optics.PSetter.id())
        |""".trimMargin()

-  fun processBoundSetter(sourceClassName: String, targetName: String, targetClassName: String, sourceName: String) = """
-      |inline val <T> $boundSetter<T, $sourceClassName>.$targetName: $boundSetter<T, $targetClassName>
+  private fun processBoundSetter(sourceClassName: String, targetName: String, targetClassName: String, sourceName: String) = """


maybe reorder to sourceName, sourceClassName, targetName, targetClassName ? The args feel quite disordered now

JorgeCastilloPrz · 2018-04-17T21:42:01Z

modules/core/arrow-annotations-processor/src/main/java/arrow/optics/OpticsProcessor.kt

-
-        normalizedTargets.forEach { target ->
-
+        element.normalizedTargets().forEach { target ->


nice cleaning

JorgeCastilloPrz · 2018-04-17T21:44:23Z

modules/core/arrow-annotations-processor/src/main/java/arrow/optics/OpticsProcessor.kt

  }

+  private val Element.otherClassTypeErrorMessage


Not a blocker, but could be better to move these to a separated ErrorMessages.ktor ErrorSyntax.kt file, IMO. After all, these functions don't seem highly tied / related to the processor class itself.

JorgeCastilloPrz · 2018-04-17T21:47:36Z

modules/core/arrow-instances-core/src/main/kotlin/arrow/instances/either.kt

@@ -74,7 +74,7 @@ fun <G, A, B, C> EitherOf<A, B>.traverse(f: (B) -> Kind<G, C>, GA: Applicative<G
 interface EitherTraverseInstance<L> : EitherFoldableInstance<L>, Traverse<EitherPartialOf<L>> {

  override fun <G, A, B> Kind<EitherPartialOf<L>, A>.traverse(AP: Applicative<G>, f: (A) -> Kind<G, B>): Kind<G, Kind<EitherPartialOf<L>, B>> =
-    fix().traverse(f, AP)
+    traverse(f, AP)


is this due to typeclass methods defined as extfuns on top of kinds now?

JorgeCastilloPrz · 2018-04-17T21:48:22Z

modules/core/arrow-instances-core/src/main/kotlin/arrow/instances/try.kt

@@ -99,7 +99,7 @@ interface TryFoldableInstance : Foldable<ForTry> {
    fix().foldRight(lb, f)
 }

-fun <A, B, G> Try<A>.traverse(f: (A) -> Kind<G, B>, GA: Applicative<G>): Kind<G, Try<B>> = GA.run {
+fun <A, B, G> TryOf<A>.traverse(f: (A) -> Kind<G, B>, GA: Applicative<G>): Kind<G, Try<B>> = GA.run {


Ok, I guess it is.

Actually, it's fixed in #793.

JorgeCastilloPrz · 2018-04-17T21:49:32Z

modules/docs/arrow-docs/docs/docs/optics/syntax/README.md

-data class Company(val name: String, val address: Address)
-@optics
-data class Employee(val name: String, val company: Company?)
+@optics data class Street(val number: Int, val name: String)


Is it mandatory to annotate each one of the implementations of the sum type (sealed class) to generate the optics for each ? Shouldn't we be able to generate optics for all the implementations on the sealed hierarchy if you just annotate the parent sealed class itself? Just opening discussion, not a blocker.

Yes, it is. Otherwise, it might get complex really quickly.

Given the network example, what is generated?

@optics sealed class NetworkResult data class Success(val content: String): NetworkResult() sealed class NetworkError: NetworkResult() data class HttpError(val message: String): NetworkError() object TimeOut: NetworkError()

Does it generate Prism<NetworkResult, Success>, Prism<NetworkResult, NetworkError> and everything applicable to Success? Or does it also generate everything for HttpError and Prisms for NetworkError?

What happens if I pass arguments to @optics? i.e. what happens if I change it to @optics([LENS])? Does it fail because I cannot generate Lens for NetworkResult? Or does it generate a Lens for Success and doesn't complain? What happens in same scenario when sealed class only contains object and thus cannot generate any Lens?

We will create a ton of edge cases which I think will be extremely confusing for the users.

Fair enough. I wasn't aware of the complexity around optics generation so it was probably a bit blind question.

JorgeCastilloPrz · 2018-04-17T21:53:50Z

modules/docs/arrow-docs/docs/docs/optics/syntax/README.md

+We can rewrite this code with our generated dsl.
+
+```kotlin:ank
+networkResult.setter().networkError.httpError.message.modify(f)


why is it setter().networkError.httpError and not setter().httpError ? How networkError is required here if we just want to set the new modified message value when the type is HttpError ?

That is because of the nested hierarchy of the sealed classes.

We need to compose Prism<NetworkResult, NetworkError> with Prism<NetworkError, HttpError> to get Prism<NeworkResult, HttpError>.

Yeah I've noticed now that there's a second level of inheritance there.

JorgeCastilloPrz · 2018-04-17T21:56:34Z

modules/optics/arrow-optics/src/main/kotlin/arrow/optics/docs_wip.kt

@@ -0,0 +1,76 @@
+package arrow.optics


why is this docs helper file (or that's how it looks like) inside of the optics module and not the docs one?

JorgeCastilloPrz · 2018-04-18T08:00:35Z

modules/core/arrow-annotations-processor/src/main/java/arrow/common/utils/ProcessorUtils.kt

@@ -47,7 +47,8 @@ interface ProcessorUtils : KotlinMetadataUtils {
  }

  fun getClassOrPackageDataWrapper(classElement: TypeElement): ClassOrPackageDataWrapper {
-    val metadata = classElement.kotlinMetadata ?: knownError("Arrow's annotations can only be used on Kotlin classes. Not valid for $classElement")
+    val metadata = classElement.kotlinMetadata


what's the code style we're using for this repo?

Not sure. I reformatted project with 2 spaces in my last commit and it moved this on two lines. Since I was unsure what the project style is for this case I left it so.

JorgeCastilloPrz

ty!

nomisRev · 2018-04-18T09:48:46Z

@raulraja I added scoped syntax as discussed on Slack. https://github.com/arrow-kt/arrow/pull/803/files#diff-43ed86ff2970e9668758930fe702b695R81

The short version of discussion:
I initially didn't add any scoped syntax for Each and At because it creates a dependency on BoundSetter. Which can be argued that it might not belong in a typeclass. However, we decided that the benefit outweighs the possible downside.

raulraja · 2018-04-18T20:52:54Z

Awesome cleanup 👏

JorgeCastilloPrz · 2018-04-18T21:09:28Z

Looks good @nomisRev :)

…ow-kt#803) * Add support for Each and At in DSL * Rewrite Each and Traversal to work on concrete type instead of kind. * Improve error message * Generate DSL for sealed classes * Docs * Add scoped syntax for Each * Add scoped syntax for At

nomisRev added 6 commits April 17, 2018 18:59

Add Each and At to DSL

d1d37a4

Add support for Each and At in DSL

654b580

Rewrite Each and Traversal to work on actual type instead of Kinds.

f2ae8a5

Improve error message

a1c6996

Fix instances

30fcdda

Generate DSL for sealed classes and improve error messages

267389c

nomisRev changed the title ~~[WIPImprove Optics DSL with support for sum types and enable Each/At~~ [WIP] Improve Optics DSL with support for sum types and enable Each/At Apr 17, 2018

nomisRev added the work in progress label Apr 17, 2018

Docs

ec609a2

nomisRev changed the title ~~[WIP] Improve Optics DSL with support for sum types and enable Each/At~~ Improve Optics DSL with support for sum types and enable Each/At Apr 17, 2018

nomisRev removed the work in progress label Apr 17, 2018

Docs sum types

cfb5fe6

JorgeCastilloPrz requested changes Apr 17, 2018

View reviewed changes

nomisRev added 6 commits April 18, 2018 09:05

Remove playground file

6e8596c

Remove changes to instances in favor of #793

de280d9

Move errors to seperate file

4e5b515

Reorder params so it makes more sense

766a878

Add KDoc

a8f0d4c

Reformat with 2 spaces

b824969

JorgeCastilloPrz reviewed Apr 18, 2018

View reviewed changes

JorgeCastilloPrz approved these changes Apr 18, 2018

View reviewed changes

nomisRev added 3 commits April 18, 2018 11:19

Add docs for DSL Each and At

4682cb2

Add scoped syntax for Each

e2aeebf

Add scoped syntax for At

bfb26a6

Merge branch 'master' into simon-optics-dsl

626dfa9

raulraja approved these changes Apr 18, 2018

View reviewed changes

Merge branch 'master' into simon-optics-dsl

ac707b1

nomisRev merged commit 1c1114a into master Apr 19, 2018

nomisRev deleted the simon-optics-dsl branch April 19, 2018 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Optics DSL with support for sum types and enable Each/At #803

Improve Optics DSL with support for sum types and enable Each/At #803

nomisRev commented Apr 17, 2018 •

edited by raulraja

Loading

codecov bot commented Apr 17, 2018 •

edited

Loading

JorgeCastilloPrz Apr 17, 2018

nomisRev Apr 18, 2018

JorgeCastilloPrz Apr 18, 2018

nomisRev Apr 18, 2018 •

edited

Loading

JorgeCastilloPrz Apr 17, 2018

JorgeCastilloPrz Apr 17, 2018

JorgeCastilloPrz Apr 17, 2018

JorgeCastilloPrz Apr 17, 2018

JorgeCastilloPrz Apr 17, 2018

nomisRev Apr 18, 2018 •

edited

Loading

JorgeCastilloPrz Apr 17, 2018 •

edited

Loading

nomisRev Apr 18, 2018

JorgeCastilloPrz Apr 18, 2018 •

edited

Loading

JorgeCastilloPrz Apr 17, 2018

nomisRev Apr 18, 2018

JorgeCastilloPrz Apr 18, 2018

JorgeCastilloPrz Apr 17, 2018

JorgeCastilloPrz Apr 18, 2018

nomisRev Apr 18, 2018

JorgeCastilloPrz left a comment

nomisRev commented Apr 18, 2018 •

edited

Loading

raulraja commented Apr 18, 2018

JorgeCastilloPrz commented Apr 18, 2018


		normalizedTargets.forEach { target ->

		element.normalizedTargets().forEach { target ->

Improve Optics DSL with support for sum types and enable Each/At #803

Improve Optics DSL with support for sum types and enable Each/At #803

Conversation

nomisRev commented Apr 17, 2018 • edited by raulraja Loading

codecov bot commented Apr 17, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomisRev Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomisRev Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

JorgeCastilloPrz Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorgeCastilloPrz Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorgeCastilloPrz left a comment

Choose a reason for hiding this comment

nomisRev commented Apr 18, 2018 • edited Loading

raulraja commented Apr 18, 2018

JorgeCastilloPrz commented Apr 18, 2018

nomisRev commented Apr 17, 2018 •

edited by raulraja

Loading

codecov bot commented Apr 17, 2018 •

edited

Loading

nomisRev Apr 18, 2018 •

edited

Loading

nomisRev Apr 18, 2018 •

edited

Loading

JorgeCastilloPrz Apr 17, 2018 •

edited

Loading

JorgeCastilloPrz Apr 18, 2018 •

edited

Loading

nomisRev commented Apr 18, 2018 •

edited

Loading