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

Kotlin generator #5341

Closed
wants to merge 4 commits into from
Closed

Kotlin generator #5341

wants to merge 4 commits into from

Conversation

oleksiyp
Copy link

@oleksiyp oleksiyp commented Nov 12, 2018

protoc that is able to generate convenient Kotlin builders on top of generated Java classes.

I submit this as minimal implementation for early review. So that it can be discussed and correct direction is chosen from the start.

Example proto file:

syntax = "proto2";

package tutorial;

option java_package = "com.example.tutorial";
option java_outer_classname = "AddressBookProtos";

message Person {
  required string name = 1;
  required int32 id = 2;
  optional string email = 3;

  enum PhoneType {
    MOBILE = 0;
    HOME = 1;
    WORK = 2;
  }

  message PhoneNumber {
    required string number = 1;
    optional PhoneType type = 2 [default = HOME];
  }

  repeated PhoneNumber phones = 4;
  optional PhoneNumber dedicatedPhone = 5;
}

message AddressBook {
  repeated Person people = 1;
}

Example code using builders:

package example

import com.example.tutorial.*
import com.example.tutorial.AddressBookProtosDSL.PhoneNumber.buildPhoneNumber
import com.example.tutorial.AddressBookProtosDSL.buildAddressBook
import com.example.tutorial.AddressBookProtosDSL.buildPerson

fun main(args: Array<String>) {
    println(buildPerson {
        id = 2
        name = "fff"
    })

    println(buildPhoneNumber {
        number = "123"
    })

    println(buildAddressBook {
        people {
            addPerson {
                id = 1234
                name = "John Doe"
                email = "[email protected]"
                dedicatedPhone {
                    type = AddressBookProtos.Person.PhoneType.WORK
                    name = "abc"
                    number = "2222-3333"
                }
                phones {
                    addPhoneNumber {
                        type = AddressBookProtos.Person.PhoneType.HOME
                        name = "def"
                        number = "555-4321"
                    }
                    addPhoneNumber {
                        type = AddressBookProtos.Person.PhoneType.WORK
                        name = "ghi"
                        number = "555-3421"
                    }
                }
            }
        }
    })
}

Example generated code:

// Generated by the protocol buffer compiler.  DO NOT EDIT!
// source: tutorial.proto

package com.example.tutorial;

object AddressBookProtosDSL {
  fun buildPerson(
    block: com.example.tutorial.AddressBookProtos.Person.Builder.() -> Unit
  ) = com.example.tutorial.AddressBookProtos.Person.newBuilder()
    .apply(block)
    .build()

  object PhoneNumber {
    fun buildPhoneNumber(
      block: com.example.tutorial.AddressBookProtos.Person.PhoneNumber.Builder.() -> Unit
    ) = com.example.tutorial.AddressBookProtos.Person.PhoneNumber.newBuilder()
      .apply(block)
      .build()

  }

  fun buildAddressBook(
    block: com.example.tutorial.AddressBookProtos.AddressBook.Builder.() -> Unit
  ) = com.example.tutorial.AddressBookProtos.AddressBook.newBuilder()
    .apply(block)
    .build()


  // @@protoc_insertion_point(outer_dsl_object_scope)
}

fun com.example.tutorial.AddressBookProtos.Person.Builder.phones(
  block: MutableList<com.example.tutorial.AddressBookProtos.Person.PhoneNumber>.() -> Unit
) {
  addAllPhones(
    mutableListOf<com.example.tutorial.AddressBookProtos.Person.PhoneNumber>()
      .apply(block)
  )
}

fun com.example.tutorial.AddressBookProtos.Person.Builder.dedicatedPhone(
    block: com.example.tutorial.AddressBookProtos.Person.PhoneNumber.Builder.() -> Unit
) {
  dedicatedPhoneBuilder.apply(block)
}

fun com.example.tutorial.AddressBookProtos.AddressBook.Builder.people(
  block: MutableList<com.example.tutorial.AddressBookProtos.Person>.() -> Unit
) {
  addAllPeople(
    mutableListOf<com.example.tutorial.AddressBookProtos.Person>()
      .apply(block)
  )
}

fun MutableList<com.example.tutorial.AddressBookProtos.Person>.addPerson(
    block: com.example.tutorial.AddressBookProtos.Person.Builder.() -> Unit
) {
    add(
      com.example.tutorial.AddressBookProtos.Person.newBuilder()
       .apply(block)
       .build()
    )
}

fun MutableList<com.example.tutorial.AddressBookProtos.Person.PhoneNumber>.addPhoneNumber(
    block: com.example.tutorial.AddressBookProtos.Person.PhoneNumber.Builder.() -> Unit
) {
    add(
      com.example.tutorial.AddressBookProtos.Person.PhoneNumber.newBuilder()
       .apply(block)
       .build()
    )
}

fun MutableList<com.example.tutorial.AddressBookProtos.AddressBook>.addAddressBook(
    block: com.example.tutorial.AddressBookProtos.AddressBook.Builder.() -> Unit
) {
    add(
      com.example.tutorial.AddressBookProtos.AddressBook.newBuilder()
       .apply(block)
       .build()
    )
}

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@oleksiyp
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@IRus
Copy link

IRus commented Nov 12, 2018

@oleksiyp What do you think about plain data classes instead of builders? Or provide both, use common interface, implement it with immutable data classes, and mutable classes with builders?

@oleksiyp
Copy link
Author

Redoing all from scratch is hard. A lot of stuff there + some apps would be nice to maintain Java interop. It is one of Kotlin principles as well. Data classes would be nice to have of course as one more option, possibly usable in multiplatform code.

@oleksiyp
Copy link
Author

oleksiyp commented Nov 12, 2018

So simply - not instead, but complementary

@RichoDemus
Copy link

I just wanted to voice my support for what IRus said, generating data classes feels a lot more Kotlin-esque

@jdemeulenaere
Copy link

jdemeulenaere commented Nov 12, 2018

Personal opinion: it would be super useful if the generated code was multiplatform, such that it can be used in common modules between different platforms.

@oleksiyp
Copy link
Author

I agree that it adds a lot of value being multiplatform. Need to think how complex it is

@RichoDemus
Copy link

Are data classes not available in in multiplatform Kotlin? or what is the issue?

@oleksiyp
Copy link
Author

@RichoDemus data classes are perfectly fine, but that's just a tip of an iceberg. Does not solve all the problems. Protobuf is quite complex and how to make it available for multiplatform use need to go through it's features and analyze how to bring them to Kotlin multiplatform

@RichoDemus
Copy link

Thanks for the clarification, I leave it up to the maintainers to figure out if they agree and which course they want to take

@oleksiyp
Copy link
Author

I think these convenient extension functions are fine, but the real problem is, of course, full support of Kotlin language i.e. having it multiplatform.

I will try to do the second iteration by copying all the java code generation code and converting it to Kotlin generation code while keeping it multiplatform. Not sure I understand how much effort is there and if I really going finish this.

@lifk
Copy link

lifk commented Nov 12, 2018

There is already a project out there that provides builders for protobuf messages in Kotlin, check https://github.com/marcoferrer/kroto-plus.

I use this generator for proto objects and gRPC calls a lot in my project already and I think would make sense that if support for Kotlin is gonna become official in protobuf then maybe efforts could be combined to create a more complete implementation.

About the point of generating Kotlin data classes maybe a simple implementation would be to just generate data classes with protobuf annotations for https://github.com/Kotlin/kotlinx.serialization. That way Kotlin multiplatform could be supported.

@marcoferrer
Copy link
Contributor

marcoferrer commented Nov 12, 2018

One thing to note is that without the @DslMarker annotation from the kotlin you run the risk of scope bleed which can lead to some difficult debugging situations.

As for the argument for creating truly kotlin first classes, there are some things to point out.

There are certain optimizations in java base proto classes that would need to be mimicked in the kotlin variants. ie memoized hashcode. Using the multiplatform serialization lib would be a good idea but I remember there being a discussion about it lacking support for proto3 syntax. That might have changed by now, or atleast might be feasible with the addition of inline classes.

Although I see the need for Kotlin specific code. I think that would need to be carefully planned and would most likely hold back this PR.

@oleksiyp
Copy link
Author

Yes, I know about "@DslMarker". I've spent on this implementation around 4-6 hours of today.

I implemented this because I do it manually and I do not want to generate all new protobuf definitions from scratch by kroto-plus or alike. I just want few additional extension functions for convenient usage. So I automated what I am doing manually. Actually, probably it was not best idea to do it as a PR to original protoc. But it was damn interesting to code again C++ 😄

Regarding optimizations, yes I've seen this generation code and it is quite advanced.

@oleksiyp oleksiyp closed this Nov 13, 2018
@oleksiyp oleksiyp mentioned this pull request May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants