remove non null oneof option (open-toast#281)

README.md

@@ -1,4 +1,4 @@
-# protokt 
+# protokt
 
 [![Github Actions](https://github.com/open-toast/protokt/actions/workflows/ci.yml/badge.svg)](https://github.com/open-toast/protokt/actions/workflows/ci.yml)
 [![Maven Central](https://img.shields.io/maven-central/v/com.toasttab.protokt/protokt-runtime)](https://search.maven.org/artifact/com.toasttab.protokt/protokt-runtime)
@@ -23,21 +23,21 @@ is represented as `String?`, etc.
 CodedOutputStream for best performance
 - (JS) Built on protobufJS for best performance
 - gRPC [method descriptor and service descriptor generation](#grpc-code-generation)
-for use with [grpc-java](#integrating-with-grpcs-java-api), 
+for use with [grpc-java](#integrating-with-grpcs-java-api),
 [grpc-kotlin](#integrating-with-grpcs-kotlin-api), and
 [grpc-node](#integrating-with-grpcs-nodejs-api) (experimental) (see examples  in [examples](examples))
 
 ### Not yet implemented
 
 - Kotlin/Native support
-- Protobuf JSON support 
+- Protobuf JSON support
 
 ### Compatibility
 
 The Gradle plugin requires Java 8+ and Gradle 5.6+. It runs on recent versions of
 MacOS, Linux, and Windows.
 
-The runtime and generated code are compatible with Kotlin 1.8+, Java 8+, and Android 4.4+. 
+The runtime and generated code are compatible with Kotlin 1.8+, Java 8+, and Android 4.4+.
 
 ## Usage
 
@@ -354,7 +354,7 @@ while using the lite runtime:
 protokt {
     generate {
         descriptors = false
-    } 
+    }
 }
 ```
 
@@ -639,8 +639,8 @@ Wrapper types that wrap protobuf messages are nullable. For example,
 can be made non-nullable by using the non-null option described below.
 
 Wrapper types that wrap protobuf primitives, for example `java.util.UUID`
-which wraps `bytes`, are nullable when they cannot wrap their wrapped type's 
-default value. Converters must override `acceptsDefaultValue` to be `false` in 
+which wraps `bytes`, are nullable when they cannot wrap their wrapped type's
+default value. Converters must override `acceptsDefaultValue` to be `false` in
 these cases. For example, a UUID cannot wrap an empty byte array and each of
 the following declarations will produce a nullable property:
 
@@ -774,15 +774,17 @@ message ImplementsWithDelegate {
 ```
 
 Note that the `by` clause references the field by its lower camel case name.
+Properties on delegate interfaces must be nullable since fields themselves
+may not be present on the wire.
 
 #### Oneof Fields
 
-Oneof fields can declare that they implement an interface with the 
+Oneof fields can declare that they implement an interface with the
 `(protokt.v1.oneof).implements` option. Each possible field type of the oneof must
 also implement the interface. This allows access of common properties without a
 `when` statement that always ultimately extracts the same property.
 
-Suppose you have a domain object MyObjectWithConfig that has a non-null configuration
+Suppose you have a domain object MyObjectWithConfig that has a configuration
 that specifies a third-party server for communication. For flexibility, this
 configuration will be modifiable by the server and versioned by a simple integer.
 To hasten subsequent loading of the configuration, a client may save a resolved
@@ -816,7 +818,6 @@ message MyObjectWithConfig {
   ];
 
   oneof Config {
-    option (protokt.v1.oneof).non_null = true;
     option (protokt.v1.oneof).implements = "com.toasttab.example.Config";
 
     ServerSpecified server_specified = 2;
@@ -836,9 +837,7 @@ message ServerSpecified {
 message ClientResolved {
   option (protokt.v1.class).implements = "com.toasttab.example.Config by config";
 
-  ServerSpecified config = 1 [
-    (protokt.v1.property).non_null = true
-  ];
+  ServerSpecified config = 1;
 
   bytes last_known_address = 2 [
     (protokt.v1.property).wrap = "java.net.InetAddress"
@@ -853,7 +852,7 @@ Protokt will generate:
 public class MyObjectWithConfig private constructor(
   @GeneratedProperty(1)
   public val id: UUID?,
-  public val config: Config,
+  public val config: Config?,
   public val unknownFields: UnknownFieldSet = UnknownFieldSet.empty()
 ) : AbstractMessage() {
 
@@ -906,18 +905,10 @@ accessing the property via a `when` expression:
 
 ```kotlin
 fun printVersion(config: MyObjectWithConfig.Config) {
-    println(config.version)
+    println(config?.version)
 }
 ```
 
-This pattern is dangerous: since the oneof must be marked non-nullable, you
-cannot compatibly add new implementing fields to a producer before a consumer
-is updated with the new generated code. The old consumer will attempt to
-deserialize the new field as an unknown field and the non-null assertion on the
-oneof field during the constructor call will fail.
-
-This functionality will likely be removed.
-
 ### BytesSlice
 
 When reading messages that contain other serialized messages as `bytes` fields,
@@ -946,7 +937,7 @@ protokt {
     generate {
         grpcDescriptors = true
         grpcKotlinStubs = true
-    } 
+    }
 }
 ```
 
@@ -1061,7 +1052,7 @@ Protokt generates complete server and client stub implementations for use with N
 The generated implementations are nearly the same as those generated by grpc-kotlin and
 are supported by an analogous runtime library in ServerCalls and ClientCalls objects.
 
-These implementations are alpha-quality and for demonstration only. External contributions 
+These implementations are alpha-quality and for demonstration only. External contributions
 to harden the implementation are welcome. They use the same `grpcDescriptors` and
 `grpcKotlinStubs` plugin options to control code generation.
 
@@ -1104,7 +1095,7 @@ protokt % protoc \
 
 ## Contribution
 
-Community contributions are welcome. See the 
+Community contributions are welcome. See the
 [contribution guidelines](CONTRIBUTING.md) and the project
 [code of conduct](CODE-OF-CONDUCT.md).
 

extensions/protokt-extensions-lite/api/protokt-extensions-lite.api

@@ -593,13 +593,12 @@ public final class protokt/v1/MethodOptions$Deserializer : protokt/v1/AbstractDe
 
 public final class protokt/v1/OneofOptions : protokt/v1/AbstractMessage {
 	public static final field Deserializer Lprotokt/v1/OneofOptions$Deserializer;
-	public synthetic fun <init> (ZLjava/lang/String;Ljava/lang/String;Lprotokt/v1/UnknownFieldSet;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public synthetic fun <init> (Ljava/lang/String;Ljava/lang/String;Lprotokt/v1/UnknownFieldSet;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun copy (Lkotlin/jvm/functions/Function1;)Lprotokt/v1/OneofOptions;
 	public static fun deserialize (Lprotokt/v1/Reader;)Lprotokt/v1/OneofOptions;
 	public fun equals (Ljava/lang/Object;)Z
 	public final fun getDeprecationMessage ()Ljava/lang/String;
 	public final fun getImplements ()Ljava/lang/String;
-	public final fun getNonNull ()Z
 	public final fun getUnknownFields ()Lprotokt/v1/UnknownFieldSet;
 	public fun hashCode ()I
 	public static final fun invoke (Lkotlin/jvm/functions/Function1;)Lprotokt/v1/OneofOptions;
@@ -613,11 +612,9 @@ public final class protokt/v1/OneofOptions$Builder {
 	public final fun build ()Lprotokt/v1/OneofOptions;
 	public final fun getDeprecationMessage ()Ljava/lang/String;
 	public final fun getImplements ()Ljava/lang/String;
-	public final fun getNonNull ()Z
 	public final fun getUnknownFields ()Lprotokt/v1/UnknownFieldSet;
 	public final fun setDeprecationMessage (Ljava/lang/String;)V
 	public final fun setImplements (Ljava/lang/String;)V
-	public final fun setNonNull (Z)V
 	public final fun setUnknownFields (Lprotokt/v1/UnknownFieldSet;)V
 }
 

extensions/protokt-extensions-lite/src/extensions-proto/protokt/v1/protokt.proto

@@ -125,22 +125,7 @@ extend google.protobuf.FieldOptions {
 }
 
 message OneofOptions {
-  // Makes a oneof field non-nullable in generated Kotlin code. Beware that
-  // deserialization will NPE if the field is missing from the protobuf payload.
-  // Adding a non-null field to an existing message is a backwards-incompatible
-  // change.
-  //
-  // For example:
-  //
-  // message Message {
-  //   oneof some_field_name {
-  //     option (protokt.v1.oneof).non_null = true;
-  //
-  //     string id = 1;
-  //   }
-  // }
-  //
-  bool non_null = 1;
+  reserved 1;
 
   // Make the sealed class implement an interface, enforcing the presence of a
   // property in each possible variant. Scoping rules  are the same as those

protokt-codegen/src/main/kotlin/protokt/v1/codegen/generate/Implements.kt

@@ -16,18 +16,24 @@
 package protokt.v1.codegen.generate
 
 import com.squareup.kotlinpoet.ClassName
+import com.squareup.kotlinpoet.FunSpec
+import com.squareup.kotlinpoet.KModifier
+import com.squareup.kotlinpoet.PropertySpec
 import com.squareup.kotlinpoet.TypeSpec
+import com.squareup.kotlinpoet.asTypeName
 import protokt.v1.codegen.generate.CodeGenerator.Context
+import protokt.v1.codegen.generate.Nullability.nullable
 import protokt.v1.codegen.util.Message
 import protokt.v1.codegen.util.StandardField
+import kotlin.reflect.KClass
 
 internal object Implements {
     fun StandardField.overrides(
         ctx: Context,
         msg: Message
     ) =
         msg.superInterface(ctx)
-            ?.let { fieldName in ctx.info.context.classLookup.properties(it.canonicalName) }
+            ?.let { fieldName in ctx.info.context.classLookup.properties(it.canonicalName).map { p -> p.name } }
             ?: false
 
     fun TypeSpec.Builder.handleSuperInterface(implements: ClassName?, v: OneofGeneratorInfo? = null) =
@@ -43,27 +49,78 @@ internal object Implements {
 
     fun TypeSpec.Builder.handleSuperInterface(msg: Message, ctx: Context) =
         apply {
-            if (msg.options.protokt.implements.isNotEmpty()) {
-                if (msg.options.protokt.implements.delegates()) {
-                    addSuperinterface(
-                        ClassName.bestGuess(msg.options.protokt.implements.substringBefore(" by ")),
-                        msg.options.protokt.implements.substringAfter(" by ")
-                    )
-                } else {
-                    addSuperinterface(msg.superInterface(ctx)!!)
+            val superInterface = msg.superInterface(ctx)
+            if (superInterface != null) {
+                addSuperinterface(superInterface.`interface`)
+                if (superInterface.delegate != null) {
+                    // don't delegate because message types may be nullable
+                    delegateProperties(msg, ctx, superInterface.canonicalName, superInterface.delegate)
                 }
             }
         }
 
-    private fun String.delegates() =
-        contains(" by ")
+    private fun TypeSpec.Builder.delegateProperties(msg: Message, ctx: Context, canonicalName: String, fieldName: String) {
+        val fieldsByName = msg.fields.filterIsInstance<StandardField>().associateBy { it.fieldName }
 
-    private fun Message.superInterface(ctx: Context) =
-        options.protokt.implements.let {
-            if (it.isNotEmpty() && !it.delegates()) {
-                inferClassName(it, ctx)
-            } else {
-                null
-            }
+        val interfaceFields =
+            ctx.info.context.classLookup
+                .properties(canonicalName)
+                .associateBy { it.name }
+
+        val implementFields = interfaceFields.values.filter { it.name !in fieldsByName.keys }
+
+        implementFields.forEach { field ->
+            val standardFieldNullable = fieldsByName.getValue(fieldName).nullable
+            addProperty(
+                PropertySpec.builder(
+                    field.name,
+                    (field.returnType.classifier as KClass<*>).asTypeName()
+                        .let {
+                            if (standardFieldNullable) {
+                                it.copy(nullable = true)
+                            } else {
+                                it
+                            }
+                        }
+                )
+                    .addModifiers(KModifier.OVERRIDE)
+                    .getter(
+                        FunSpec.getterBuilder()
+                            .addCode(
+                                "return %L" +
+                                    if (standardFieldNullable) {
+                                        "?"
+                                    } else {
+                                        ""
+                                    } +
+                                    ".%L",
+                                fieldName,
+                                field.name
+                            )
+                            .build()
+                    )
+                    .build()
+            )
+        }
+    }
+
+    private class SuperInterface(
+        val `interface`: ClassName,
+        val delegate: String?
+    ) {
+        val canonicalName = `interface`.canonicalName
+    }
+
+    private fun Message.superInterface(ctx: Context): SuperInterface? {
+        val implements = options.protokt.implements
+        return when {
+            implements.isEmpty() -> null
+            implements.contains(" by ") ->
+                SuperInterface(
+                    inferClassName(implements.substringBefore(" by "), ctx),
+                    implements.substringAfter(" by ")
+                )
+            else -> SuperInterface(inferClassName(implements, ctx), null)
         }
+    }
 }

protokt-codegen/src/main/kotlin/protokt/v1/codegen/generate/Nullability.kt

@@ -24,15 +24,10 @@ import protokt.v1.reflect.FieldType
 
 internal object Nullability {
     val Field.hasNonNullOption
-        get() =
-            when (this) {
-                is StandardField -> options.protokt.nonNull
-                is Oneof -> options.protokt.nonNull
-            }
+        get() = this is StandardField && options.protokt.nonNull
 
     val Field.nullable
-        get() =
-            isKotlinRepresentationNullable && !hasNonNullOption
+        get() = isKotlinRepresentationNullable && !hasNonNullOption
 
     private val Field.isKotlinRepresentationNullable
         get() =

protokt-codegen/src/test/kotlin/protokt/v1/codegen/ImplementDelegationTest.kt

@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2023 Toast, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package protokt.v1.codegen
+
+import org.junit.jupiter.api.Test
+
+class ImplementDelegationTest : AbstractProtoktCodegenTest() {
+    @Test
+    fun `delegate to interface with non-null property`() {
+        runPlugin("implement_by_delegate_with_non_null_property.proto").orFail()
+    }
+}
+
+@Suppress("UNUSED")
+interface Model {
+    val id: String
+}