Fix nullability of non-optional fields in TS interfaces and class-level JS comments

cli/README.md

@@ -68,6 +68,7 @@ Translates between file formats and generates static code.
   --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.
   --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.
   --force-message  Enforces the use of message instances instead of plain objects.
+  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)
 
 usage: pbjs [options] file1.proto file2.json ...  (or pipe)  other | pbjs [options] -
 ```

cli/pbjs.js

@@ -41,7 +41,7 @@ exports.main = function main(args, callback) {
             "force-message": "strict-message"
         },
         string: [ "target", "out", "path", "wrap", "dependency", "root", "lint", "filter" ],
-        boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "typeurl", "beautify", "comments", "service", "es6", "sparse", "keep-case", "alt-comment", "force-long", "force-number", "force-enum-string", "force-message", "null-defaults" ],
+        boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "typeurl", "beautify", "comments", "service", "es6", "sparse", "keep-case", "alt-comment", "force-long", "force-number", "force-enum-string", "force-message", "force-optional", "null-defaults"],
         default: {
             target: "json",
             create: true,
@@ -62,7 +62,8 @@ exports.main = function main(args, callback) {
             "force-number": false,
             "force-enum-string": false,
             "force-message": false,
-            "null-defaults": false,
+            "force-optional": false,
+            "null-defaults": false
         }
     });
 
@@ -146,8 +147,9 @@ exports.main = function main(args, callback) {
                 "  --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-message  Enforces the use of message instances instead of plain objects.",
+                "  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)",
                 "",
-                "  --null-defaults  Default value for optional fields is null instead of zero value.",
+                "  --null-defaults  Default value for optional fields is null instead of zero value (deprecated, use --force-optional instead)",
                 "",
                 "usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray("  (or pipe)  ") + "other | " + chalk.bold.green("pbjs") + " [options] -",
                 ""

cli/targets/static.js

@@ -354,6 +354,27 @@ function toJsType(field) {
     return type;
 }
 
+function isOptional(type, field) {
+
+    // Figure out if a field is explicitly marked optional, depending on the proto syntax (proto2 vs proto3)
+    // If the syntax has not been recorded in the AST, proto2 semantics will be the default
+
+    var syntax = null;
+    var namespace = type;
+
+    while (syntax === null && namespace !== null) {
+        if (namespace.options != null && "syntax" in namespace.options)
+            syntax = namespace.options["syntax"]
+        else
+            namespace = namespace.parent
+    }
+
+    if (syntax === "proto3")
+        return field.proto3Optional
+    else
+        return field.optional
+}
+
 function buildType(ref, type) {
 
     if (config.comments) {
@@ -366,9 +387,25 @@ function buildType(ref, type) {
             var prop = util.safeProp(field.name); // either .name or ["name"]
             prop = prop.substring(1, prop.charAt(0) === "[" ? prop.length - 1 : prop.length);
             var jsType = toJsType(field);
-            if (field.optional)
-                jsType = jsType + "|null";
-            typeDef.push("@property {" + jsType + "} " + (field.optional ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name));
+            var nullable = false;
+
+            // New behaviour - fields explicitly marked as optional and members of a one-of are nullable
+            // Maps and repeated fields are not explicitly optional, they default to empty instances
+            if (config["force-optional"]) {
+                if (isOptional(type, field) || field.partOf) {
+                    jsType = jsType + "|null|undefined";
+                    nullable = true;
+                }
+            }
+            // Old behaviour - field.optional is true for all fields in proto3
+            else {
+                if (field.optional) {
+                    jsType = jsType + "|null";
+                    nullable = true;
+                }
+            }
+
+            typeDef.push("@property {" + jsType + "} " + (nullable ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name));
         });
         push("");
         pushComment(typeDef);
@@ -394,8 +431,19 @@ function buildType(ref, type) {
         if (config.comments) {
             push("");
             var jsType = toJsType(field);
-            if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf)
-                jsType = jsType + "|null|undefined";
+
+            // New behaviour - fields explicitly marked as optional and members of a one-of are nullable
+            // Maps and repeated fields are not explicitly optional, they default to empty instances
+            if (config["force-optional"]) {
+                if (isOptional(type, field) || field.partOf)
+                    jsType = jsType + "|null|undefined";
+            }
+            // Old behaviour - field.optional is true for all fields in proto3
+            else {
+                if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf)
+                    jsType = jsType + "|null|undefined";
+            }
+
             pushComment([
                 field.comment || type.name + " " + field.name + ".",
                 "@member {" + jsType + "} " + field.name,
@@ -406,11 +454,16 @@ function buildType(ref, type) {
             push("");
             firstField = false;
         }
+        // New behaviour sets a null default when the optional keyword is used explicitly
+        // Old behaviour considers all proto3 fields optional and uses the null-defaults config flag
+        var nullDefault = config["force-optional"]
+            ? isOptional(type, field)
+            : field.optional && config["null-defaults"];
         if (field.repeated)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor
         else if (field.map)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyObject;"); // overwritten in constructor
-        else if (field.partOf || field.optional && config["null-defaults"])
+        else if (field.partOf || nullDefault)
             push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for oneof members
         else if (field.long)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.Long ? $util.Long.fromBits("

src/field.js

@@ -78,6 +78,13 @@ function Field(name, id, type, rule, extend, options, comment) {
     if (extend !== undefined && !util.isString(extend))
         throw TypeError("extend must be a string");
 
+    /**
+     * Explicit record of the proto3 optional rule
+     * Needed to stop the proto3 optional semantics from getting lost
+     * @type {boolean}
+     */
+    this.proto3Optional = rule === "proto3_optional";
+
     /**
      * Field rule, if any.
      * @type {string|undefined}

src/parse.js

@@ -263,6 +263,10 @@ function parse(source, root, options) {
         if (!isProto3 && syntax !== "proto2")
             throw illegal(syntax, "syntax");
 
+        // Syntax is needed to understand the meaning of the optional field rule
+        // Otherwise the meaning is ambiguous between proto2 and proto3
+        root.setOption("syntax", syntax)
+
         skip(";");
     }
 

tests/cli.js

@@ -165,6 +165,72 @@ tape.test("with null-defaults, absent optional fields have null values", functio
 });
 
 
+tape.test("with force-optional, optional fields are handled correctly in proto2", function(test) {
+    cliTest(test, function() {
+        var root = protobuf.loadSync("tests/data/cli/null-defaults.proto");
+        root.resolveAll();
+
+        var staticTarget = require("../cli/targets/static");
+
+        staticTarget(root, {
+            create: true,
+            decode: true,
+            encode: true,
+            convert: true,
+            comments: true,
+            "force-optional": true,
+        }, function(err, jsCode) {
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null|undefined} c"), "Member for c should be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.c = null;"), "Initializer for c should be null")
+
+            test.ok(jsCode.includes("@property {number} d OptionalFields d"), "Property for d should not be nullable")
+            test.ok(jsCode.includes("@member {number} d"), "Member for d should not be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.d = 0;"), "Initializer for d should be zero")
+
+            test.end();
+        });
+    });
+});
+
+
+tape.test("with force-optional, optional fields are handled correctly in proto3", function(test) {
+    cliTest(test, function() {
+        var root = protobuf.loadSync("tests/data/cli/null-defaults-proto3.proto");
+        root.resolveAll();
+
+        var staticTarget = require("../cli/targets/static");
+
+        staticTarget(root, {
+            create: true,
+            decode: true,
+            encode: true,
+            convert: true,
+            comments: true,
+            "force-optional": true,
+        }, function(err, jsCode) {
+
+            console.log(jsCode);
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null|undefined} c"), "Member for c should be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.c = null;"), "Initializer for c should be null")
+
+            test.ok(jsCode.includes("@property {number} d OptionalFields d"), "Property for d should not be nullable")
+            test.ok(jsCode.includes("@member {number} d"), "Member for d should not be nullable")
+            test.ok(jsCode.includes("OptionalFields.prototype.d = 0;"), "Initializer for d should be zero")
+
+            test.end();
+        });
+    });
+});
+
+
 tape.test("pbjs generates static code with message filter", function (test) {
     cliTest(test, function () {
         var root = protobuf.loadSync("tests/data/cli/test-filter.proto");

tests/data/cli/null-defaults-proto3.proto

@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+message OptionalFields {
+  message SubMessage {
+    string a = 1;
+  }
+
+  optional SubMessage a = 1;
+  optional string b = 2;
+  optional uint32 c = 3;
+  uint32 d = 4;
+}

tests/data/cli/null-defaults.proto

@@ -8,4 +8,5 @@ message OptionalFields {
   optional SubMessage a = 1;
   optional string b = 2;
   optional uint32 c = 3;
+  required uint32 d = 4;
 }