Merge pull request #159 from znehrenberg-sc/main

Support Optionals as Optional Parameters in the Constructor

README.md

@@ -67,6 +67,8 @@ verify the build and binary from the command line.
  - DataView for copy free data passing
  - DateRef for copy free data passing with ownership
  - Generating string names for C++ enums
+ - Omit optional parameters from record constructors
+ - Records are default mutable across all platforms
  - Bug fixes
 
 ## Using new features
@@ -341,6 +343,38 @@ The C++ `Future` type has optional support for coroutines. If coroutines are
 availble (eg. compiling with C++20 or C++17 with -fcoroutines-ts), then you can
 use `co_await` on future objects.
 
+## Requiring Optional Parameters in Individual Records
+- By default, optionals will be omitted from the constructor in Djinni
+- The Djinni code generator will generate two constructors for a record with optionals: one with all values and one with optionals omitted. This will minimize code disruption
+- Optional behavior will be able to be switched via a usage flag for each platform. 
+- A new [deriving method](https://github.com/dropbox/djinni#derived-methods) specifier will be implemented so that individual records can still require all parameters
+
+### Deriving Record
+Any record can be made to have all parameters be required by specifying it as a `req` deriving record:
+```
+MyClass = record {
+  required: string;
+  optional: optional<string>;
+} deriving(req)
+```
+
+### Omitting Convenience Constructors
+Extra convenience constructors which require all parameters can be removed from optional ObjC records with a new compiler flag:
+```
+--objc-omit-full-convenience-constructor
+```
+
+## Reverting to Legacy Record Behavior
+Djinni records are now mutable and do not require optionals in reocrd constructors by default. In order to reverse this behavior and make Java and ObjC records immutable with full constructors only, the following generation flags can be used:
+
+```
+--cpp-legacy-records
+--java-legacy-records
+--objc-legacy-records
+```
+
+Note that for C++, the legacy flag will only remove the optional-omitting constructor. Records were already mutable within C++.
+
 ## FAQ
 
 Q. Do I need to use Bazel to build my project?

examples/generated-src/java/com/dropbox/textsort/ItemList.java

@@ -10,7 +10,7 @@
 /*package*/ final class ItemList {
 
 
-    /*package*/ final ArrayList<String> mItems;
+    /*package*/ ArrayList<String> mItems;
 
     public ItemList(
             @Nonnull ArrayList<String> items) {
@@ -22,6 +22,10 @@ public ArrayList<String> getItems() {
         return mItems;
     }
 
+    public void setItems(@Nonnull ArrayList<String> items) {
+        this.mItems = items;
+    }
+
     @Override
     public String toString() {
         return "ItemList{" +

examples/generated-src/objc/TXSItemList.h

@@ -9,6 +9,6 @@
 - (nonnull instancetype)initWithItems:(nonnull NSArray<NSString *> *)items NS_DESIGNATED_INITIALIZER;
 + (nonnull instancetype)itemListWithItems:(nonnull NSArray<NSString *> *)items;
 
-@property (nonatomic, readonly, nonnull) NSArray<NSString *> * items;
+@property (nonatomic, copy, nonnull) NSArray<NSString *> * items;
 
 @end

perftest/generated-src/java/com/snapchat/djinni/benchmark/RecordSixInt.java

@@ -9,17 +9,17 @@
 /*package*/ final class RecordSixInt {
 
 
-    /*package*/ final long mI1;
+    /*package*/ long mI1;
 
-    /*package*/ final long mI2;
+    /*package*/ long mI2;
 
-    /*package*/ final long mI3;
+    /*package*/ long mI3;
 
-    /*package*/ final long mI4;
+    /*package*/ long mI4;
 
-    /*package*/ final long mI5;
+    /*package*/ long mI5;
 
-    /*package*/ final long mI6;
+    /*package*/ long mI6;
 
     public RecordSixInt(
             long i1,
@@ -40,26 +40,50 @@ public long getI1() {
         return mI1;
     }
 
+    public void setI1(long i1) {
+        this.mI1 = i1;
+    }
+
     public long getI2() {
         return mI2;
     }
 
+    public void setI2(long i2) {
+        this.mI2 = i2;
+    }
+
     public long getI3() {
         return mI3;
     }
 
+    public void setI3(long i3) {
+        this.mI3 = i3;
+    }
+
     public long getI4() {
         return mI4;
     }
 
+    public void setI4(long i4) {
+        this.mI4 = i4;
+    }
+
     public long getI5() {
         return mI5;
     }
 
+    public void setI5(long i5) {
+        this.mI5 = i5;
+    }
+
     public long getI6() {
         return mI6;
     }
 
+    public void setI6(long i6) {
+        this.mI6 = i6;
+    }
+
     @Override
     public String toString() {
         return "RecordSixInt{" +

perftest/generated-src/objc/TXSRecordSixInt.h

@@ -19,16 +19,16 @@
                                         i5:(int64_t)i5
                                         i6:(int64_t)i6;
 
-@property (nonatomic, readonly) int64_t i1;
+@property (nonatomic) int64_t i1;
 
-@property (nonatomic, readonly) int64_t i2;
+@property (nonatomic) int64_t i2;
 
-@property (nonatomic, readonly) int64_t i3;
+@property (nonatomic) int64_t i3;
 
-@property (nonatomic, readonly) int64_t i4;
+@property (nonatomic) int64_t i4;
 
-@property (nonatomic, readonly) int64_t i5;
+@property (nonatomic) int64_t i5;
 
-@property (nonatomic, readonly) int64_t i6;
+@property (nonatomic) int64_t i6;
 
 @end

src/source/CppGenerator.scala

@@ -221,6 +221,7 @@ class CppGenerator(spec: Spec) extends Generator(spec) {
       writeCppTypeParams(w, params)
       w.w("struct " + actualSelf + cppFinal).bracedSemi {
         generateHppConstants(w, r.consts)
+
         // Field definitions.
         for (f <- r.fields) {
           writeDoc(w, f.doc)
@@ -243,18 +244,48 @@ class CppGenerator(spec: Spec) extends Generator(spec) {
           w.wl(s"friend bool operator>=(const $actualSelf& lhs, const $actualSelf& rhs);")
         }
 
-        // Constructor.
-        if(r.fields.nonEmpty) {
-          w.wl
-          if (r.fields.size == 1) {
-            w.wl("//NOLINTNEXTLINE(google-explicit-constructor)")
+        // Constructor generator
+        def writeConstructor(reqFields: Seq[Field])  {
+          // Only skip if there are no fields at all. If only optional
+          // fields exist, we should still create the trivial constructor
+          if(r.fields.nonEmpty) {
+            w.wl
+            if (reqFields.size == 1) {
+              w.wl("//NOLINTNEXTLINE(google-explicit-constructor)")
+            }
+            writeAlignedCall(w, actualSelf + "(", reqFields, ")", f => marshal.fieldType(f.ty) + " " + idCpp.local(f.ident) + "_")
+            w.wl
+
+            // Determimne if we are writing a constructor omitting optionals or one requiring
+            // all parameters
+            if (r.fields.size != reqFields.size) {
+              writeAlignedCall(w, ": " + actualSelf + "(", r.fields, ")", f => {
+                var param = "std::move(" + idCpp.local(f.ident) + "_)"
+                if (isOptional(f.ty.resolved))
+                  if (isInterface(f.ty.resolved.args.head)) 
+                    param = s"nullptr" 
+                  else 
+                    param = s"${spec.cppNulloptValue}"
+
+                param
+              })
+              w.wl
+            } else {
+              // required constructor
+              val init = (f: Field) => idCpp.field(f.ident) + "(std::move(" + idCpp.local(f.ident) + "_))"
+              w.wl(": " + init(r.fields.head))
+              r.fields.tail.map(f => ", " + init(f)).foreach(w.wl)
+            }
+            w.wl("{}")
           }
-          writeAlignedCall(w, actualSelf + "(", r.fields, ")", f => marshal.fieldType(f.ty) + " " + idCpp.local(f.ident) + "_")
-          w.wl
-          val init = (f: Field) => idCpp.field(f.ident) + "(std::move(" + idCpp.local(f.ident) + "_))"
-          w.wl(": " + init(r.fields.head))
-          r.fields.tail.map(f => ", " + init(f)).foreach(w.wl)
-          w.wl("{}")
+        }
+
+        // First write constructor requiring optionals, required for marshaling
+        writeConstructor(r.fields)
+
+        // Next write optional omitting constructor if necessary
+        if (!spec.cppLegacyRecords && !r.derivingTypes.contains(DerivingType.Req) && r.reqFields.size != r.fields.size) {
+          writeConstructor(r.reqFields)
         }
 
         if (r.ext.cpp) {

src/source/JavaGenerator.scala

@@ -275,27 +275,46 @@ class JavaGenerator(spec: Spec) extends Generator(spec) {
         generateJavaConstants(w, r.consts)
         // Field definitions.
         for (f <- r.fields) {
+          var fieldFinal = if (spec.javaLegacyRecords) "final " else if (r.derivingTypes.contains(DerivingType.Req) || !isOptional(f.ty.resolved)) "" else "/*optional*/ "
           w.wl
-          w.wl(s"/*package*/ final ${marshal.fieldType(f.ty)} ${idJava.field(f.ident)};")
+          w.wl(s"/*package*/ ${fieldFinal}${marshal.fieldType(f.ty)} ${idJava.field(f.ident)};")
         }
 
-        // Constructor.
-        w.wl
-        w.wl(s"public $self(").nestedN(2) {
-          val skipFirst = SkipFirst()
-          for (f <- r.fields) {
-            skipFirst { w.wl(",") }
-            marshal.nullityAnnotation(f.ty).map(annotation => w.w(annotation + " "))
-            w.w(marshal.paramType(f.ty) + " " + idJava.local(f.ident))
+        def writeConstructor(reqFields: Seq[Field]) {
+          w.wl
+          if (reqFields.isEmpty)
+            w.wl(s"public $self() {")
+          else
+            w.wl(s"public $self(").nestedN(2) {
+              val skipFirst = SkipFirst()
+              for (f <- reqFields) {
+                skipFirst { w.wl(",") }
+                marshal.nullityAnnotation(f.ty).map(annotation => w.w(annotation + " "))
+                w.w(marshal.paramType(f.ty) + " " + idJava.local(f.ident))
+              }
+              w.wl(") {")
+            }
+          w.nested {
+            // Optional constructor or full constructor
+            if (reqFields.size != r.fields.size) {
+              writeAlignedCall(w, "this(", r.fields, ");", f=> if (isOptional(f.ty.resolved)) "null" else idJava.local(f.ident))
+              w.wl
+            } else {
+              for (f <- reqFields) {
+                w.wl(s"this.${idJava.field(f.ident)} = ${idJava.local(f.ident)};")
+              }
+            }
           }
-          w.wl(") {")
+          w.wl("}")
         }
-        w.nested {
-          for (f <- r.fields) {
-            w.wl(s"this.${idJava.field(f.ident)} = ${idJava.local(f.ident)};")
-          }
+
+        // Constructor, requiring all parameters, used for marshaling
+        writeConstructor(r.fields)
+
+        // Constructor, not requiring optionals, generated if necessary
+        if (!spec.javaLegacyRecords && !r.derivingTypes.contains(DerivingType.Req) && r.fields.size != r.reqFields.size) {
+          writeConstructor(r.reqFields)
         }
-        w.wl("}")
 
         // Accessors
         for (f <- r.fields) {
@@ -305,6 +324,16 @@ class JavaGenerator(spec: Spec) extends Generator(spec) {
           w.w("public " + marshal.returnType(Some(f.ty)) + " " + idJava.method("get_" + f.ident.name) + "()").braced {
             w.wl("return " + idJava.field(f.ident) + ";")
           }
+
+          if (!spec.javaLegacyRecords) {
+            w.wl
+            // Check for null for setters used on required parameters
+            var nullability = if (isOptional(f.ty.resolved)) "" else marshal.nullityAnnotation(f.ty).map(_ + "").getOrElse("")
+            nullability = if (nullability.isEmpty) "" else nullability + " "
+            w.w("public void " + idJava.method("set_" + f.ident.name) + "(" + nullability + marshal.paramType(f.ty) + " " + idJava.local(f.ident) + ")").braced {
+              w.wl(s"this.${idJava.field(f.ident)} = ${idJava.local(f.ident)};")
+            }
+          }
         }
 
         if (r.derivingTypes.contains(DerivingType.Eq)) {