PolymerLabs / arcs

Arcs
BSD 3-Clause "New" or "Revised" License
58 stars 35 forks source link

Idiomatic Kotlin Particle Base Class #3665

Open piotrswigon opened 5 years ago

piotrswigon commented 5 years ago

Goal

Achieve the idiomatic way to write Arcs Particle in Kotlin.

Deliverable

A new Kotlin particle base class that extends the bare-metal Particle.kt class aims to bring the idiomatic Kotlin way of programming to particles.

Alternatively (or maybe preferably!) a set of extensions that can be used in the Particle.kt to make writing particles less verbose.

The new subclass should be compatible with both Dev and Rel Experiences of Arcs.

Resources

  1. @cromwellian's Kotlin Idiomatic Arcs SDK Mapping document.
  2. Learn Kotlin. Write a particle. Fix friction. Repeat.
  3. Review / discuss / engage with Jason Feinstein.
piotrswigon commented 5 years ago

I'll use this issue to explore ideas for leveraging Kotlin language features in particles.

piotrswigon commented 5 years ago

Delegated properties for easier access to handle data

_(Below I assume we've moved from ParticleNameConnectionName naming to just ConnectionName for generate entity classes)

e.g. instead of:

    val input = Singleton { Person() }

    init {
        registerHandle("input", input)
    }

    fun something(x) {
        val person = input.get()
        input.set(Person(...))
    }

One could imagine:

    val input: Person by Singleton()
    // or like this:
    val person: Person by Singleton("input")

    fun something() {
        // These call singleton.get() and set() under the hood.
        val person = input
        input = Person(...);
    }
piotrswigon commented 5 years ago

Improve naming of generated entities

This is not really idiomatic kotlin api, but it is a major API improvement.

3663 has left us with entities generated as ParticleName_HandleName. It's functional, but not pretty. @alxrsngrtn looked into improving the state of affairs by placing the HandleName class in the particlename package, which would be an improvement.

We could also explore using type aliases. We could decrease a number of conflicts by making schema2kt tool run for a particular particle spec, instead of the entire file. To be explored.

piotrswigon commented 5 years ago

Utilities for Reactivity

This builds on "Delegated properties for easier access to handle data" idea above.

One could imagine something like below:

class MyParticle : ReactiveParticle() {
    // Handle Data
    val person: Person by singleton(reactive = true)
    val favoriteFood: Food by singleton(reactive = false)
    val location: Location by collection(reactive = true)

    // Internal State
    val count by reactive(0); // reactive-enabled Int
    val localName = ""; // reactive-disabled String

    fun react() {
      // work with person, favoriteFood, location, count, localName as you normally would
    }
}

What would happen, is that singleton, collection and reactive are methods on the ReactiveParticle which are used to create delegated properties, but also register change listeners (optionally, e.g. based on reactive parameter?) 'which cause the react() method to be called.

Some other variation of this could be to have this work in regular Particle class with something like:

class MyParticle : Particle() {
    // Secret sauce, object that collects change listeners
    val react = React()

    // Handle Data
    val person: Person by singleton(react)
    val favoriteFood: Food by singleton()
    val location: Location by collection(react)

    // Internal State
    val count by react on 0; // reactive-enabled Int, infix call, just having fun with Kotlin :P
    val localName = ""; // reactive-disabled String

    init {
      // Below is called whenever person, location or count change:
      react.do {
        // Business logic.
      }
    }
}

It's worth noting that implementing reactive behavior cannot be done in a naive way. Changing any of the reactive properties from inside react {} block would be re-entrant, and could be dangerous. It's best to consult @sjmiles on how he manages that in Xen, as he is the expert.

piotrswigon commented 5 years ago

@SHeimlich You seemed open to exploring this space - I wrote some ideas that may be worth exploring or at least getting inspiration from. Please write down your ideas on this issue whenever you have them :) Let me know if you would like to grab 1:1 time to discuss further :)

piotrswigon commented 5 years ago

@jasonwyatt you may be interested in this issue.

piotrswigon commented 5 years ago

Data classes for template interpolation

Building on Ray's idea from his doc, we could explore something like:


override fun getTemplate(slotName: String) = """
    <span>{{person}}</span>
    <span>{{count}}</span>
    <span>{{thing}}</span>""".trimIndent()

// This is verbose, but at least allows declaring fields and types.
data class TemplateModel(val map: MutableMap<String, Any?> = mutableMapOf()) {
  var person: String by map
  var count: Int by map
  var thing: String by map
}

override fun render(slotName: String) = TemplateModel().apply {
    // These are at least type checked... I'm not sure how much it helps
    person = "John"
    count = 42
    thing = "Bicycle"
}.map
jasonwyatt commented 5 years ago

Improve naming of generated entities

This is not really idiomatic kotlin api, but it is a major API improvement.

3663 has left us with entities generated as ParticleName_HandleName. It's functional, but not pretty. @alxrsngrtn looked into improving the state of affairs by placing the HandleName class in the particlename package, which would be an improvement.

We could also explore using type aliases. We could decrease a number of conflicts by making schema2kt tool run for a particular particle spec, instead of the entire file. To be explored.

Could HandleName just be a static inner class of ParticleName?

piotrswigon commented 5 years ago

That would be ideal @jasonwyatt, but HandleName class is code-generated from the manifest. Any suggestions how to namespace it?

alxmrs commented 5 years ago

@piotrswigon We've spoken about how to do this in the Wasm sync. I noted briefly how this should be done (after I get the higher priority issues implemented first): https://github.com/PolymerLabs/arcs/issues/3667#issuecomment-544617476 @jasonwyatt 's solution of inner classes is the way we should go.

piotrswigon commented 5 years ago

That is, the inner classes would be a way to go... if we could codegen them. Can we?

SHeimlich commented 5 years ago

As suggested by @jasonwyatt auto-generate the boiler plate at the end of Kotlin files: For example (from Tutorial 4)


@Retain
@ExportForCppRuntime()
fun _newGetPerson() = GetPersonParticle().toWasmAddress()
``
SHeimlich commented 5 years ago

Make registering handles easier.

Currently registering a handle requires defining a local object: private val person = Singleton { GetPerson_Person() } and then registering the handle: registerHandle("person", person)

This becomes verbose for larger particles.

SHeimlich commented 5 years ago

Be able to initialize entities obtained by handles in init.

cromwellian commented 5 years ago

Be able to initialize entities obtained by handles in init. You can already do something like

private val person = Singleton { GetPerson_Person(a, b, c) }

cromwellian commented 5 years ago

Eliminate dictionaries in APIs in favor of data classes.

For example, eventHandler() should look something like this:

@Serializable // Kotlinx.serialization
data class FooEvent(val field1: Type1, val field2: Type2)

class FooParticle : Particle {
   init {
     eventHandler("foo", ::onFoo)
   }

   fun onFoo(evt: FooEvent) { doSomethingWith(evt1.field1, evt2.field2) }
}

inline fun <reified T : Any> eventHandler(name: String, handler: (T) -> Unit) {
        eventHandlers[name] = {  eventData: Map<String, String> -> 
            handler(Mykson.parseDictionary(
                    T.serializer(),
                    eventData
                ))
}

Ideally, Particles have signatures with concrete types, not dictionaries or Strings masquerading as other types. However, that doesn't mean they can't be backed by a duck-typed map underneath (e.g. carry extra fields)

Kotlinx.serialization allows you to write custom wire format handlers, and already has parsers for JSON, protobuf, ASN.1, etc. I have some old Mykson encoder for kotlinx.serialization sitting around, they're not too hard to write.

cromwellian commented 5 years ago

Stage 2 would be to remove eventHandler/registerHandle in favor of KAPT annotations, e.g.

@Singleton
val foo : FooParticle_Entity

@Event
fun onFoo(eventData: FooEvent) { ... }
cromwellian commented 5 years ago

@jasonwyatt Should we make all Particle lifecycle methods suspend functions, so that any async work done in the methods doesn't require the developer to need to launch a coroutine scope, e.g.

suspend fun onMyFooEvent(eventData: MyFooEvent) {
   val classifiedText = classifyText(eventData.enteredText)
}

suspend fun classifyText(text: String) {
   return serviceCall("textClassifier.classifyText", ClassifierArgs(text).map)
}

e.g. no need to do launch . {} or async {} inside the classifyText() function (because serviceCall is inherently a coroutine)

SHeimlich commented 5 years ago

GetPerson_Person(a, b, c)

Ray and I just tried this, and it does not currently work. 😞

cromwellian commented 5 years ago

Minimal idealized Particle

@Serializable
data class Person(...)

@Serializable
data class Total(val value: Int)

// Comes from some UI package that provides elements and events
@Serializable
data class AddEvent(val value: Int)

@Particle // generates @ExportForCppRuntime via Compiler Plugin for WASM
class MyParticle : ExpenseService {
   // registerHandle invoked automatically by compiler plugin
   val person: Person by singleton
   val total: Total by singleton

  @Event // compiler plugin, invokes eventHandler("add", ::onAdd)
   suspend fun onAdd(evt: AddEvent) = total.value += evt.value

  override fun getTemplate(slotName: String) = """
    <span>{{person}}</span>
    <span>{{count}}</span>
    <span>{{thing}}</span>""".trimIndent()

  // This is verbose, but at least allows declaring fields and types.
  // (Ray) We can potentially generate from a compiler plugin somehow, or reuse Schema  as 
  // template models
  data class TemplateModel(val map: MutableMap<String, Any?> = mutableMapOf()) {
    var person: String by map
    var count: Int by map
    var thing: String by map
  }

  override fun render(slotName: String) = TemplateModel().apply {
      // These are at least type checked... I'm not sure how much it helps
      person = "John"
        count = 42
        thing = "Bicycle"
    }.map   
  }

// Provided by someone else
interface ExpenseService {
     @Serializable
     data class Expense(val value: Int)
     suspend fun checkExpenses(amt: Int) = serviceCall("expenseClassifier.classify", Expense(amt))
}

In the above, it is presumed the developer only writes the @Particle class { } code.

cromwellian commented 4 years ago

GetPerson_Person(a, b, c)

Ray and I just tried this, and it does not currently work. 😞

Seems like it is overwritten by the first syncHandle/updateHandle from a newly created handle. Perhaps we should not update fields which have never been assigned from a newly created handle?

Alternatively, @Cypher1 Has default schema value syntax ever been discussed, e.g.

schema Foo 
   Text blah = "Default Value"
   Number foo = 42

This could allow the schema2kotlin tool generate class like

data class Foo (var blah: String = "Default Value", var foo: Int = 42)
Cypher1 commented 4 years ago

Interesting idea Ray, that would mean that the parser would need some way of representing all the data we'd like to be able to pipe in, and we'd need to be able to convert that to the target language. Possibly useful, but I think we're better off not providing convenience features like this right now.

We have talked about a more 'executable' syntax before, which would suggest features like this. Its not on the short term road map.

SHeimlich commented 4 years ago

Someway to remove null checking for every entity field.

SHeimlich commented 4 years ago

Better error message if you don't register a handle in init()

mykmartin commented 4 years ago

Delegated properties for easier access to handle data

I'm fairly sure this doesn't work, as noted on #4226. The problem is that the particle reference (i.e. the thisRef passed to the delegate's getValue) needs to be known at construction time, and as far as I'm aware that isn't possible. I have removed the registerHandle call, though.