fix: handle nullability for optional fields

cli/README.md

@@ -68,6 +68,9 @@ 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.
+
+  --null-defaults  Default value for optional fields is null instead of zero value.
+  --null-semantics Make nullable fields match protobuf semantics (overrides --null-defaults).
 
 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", "null-defaults", "null-semantics"],
         default: {
             target: "json",
             create: true,
@@ -63,6 +63,7 @@ exports.main = function main(args, callback) {
             "force-enum-string": false,
             "force-message": false,
             "null-defaults": false,
+            "null-semantics": false
         }
     });
 
@@ -148,6 +149,7 @@ exports.main = function main(args, callback) {
                 "  --force-message  Enforces the use of message instances instead of plain objects.",
                 "",
                 "  --null-defaults  Default value for optional fields is null instead of zero value.",
+                "  --null-semantics Make nullable fields match protobuf semantics (overrides --null-defaults).",
                 "",
                 "usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray("  (or pipe)  ") + "other | " + chalk.bold.green("pbjs") + " [options] -",
                 ""

cli/targets/static.js

@@ -311,9 +311,15 @@ function buildFunction(type, functionName, gen, scope) {
         push("};");
 }
 
-function toJsType(field) {
+function toJsType(field, parentIsInterface = false) {
     var type;
 
+    // With null semantics, interfaces are composed from interfaces and messages from messages
+    // Without null semantics, child types depend on the --force-message flag
+    var asInterface = config["null-semantics"]
+        ? parentIsInterface && !(field.resolvedType instanceof protobuf.Enum)
+        : !(field.resolvedType instanceof protobuf.Enum || config.forceMessage);
+
     switch (field.type) {
         case "double":
         case "float":
@@ -341,10 +347,12 @@ function toJsType(field) {
             type = "Uint8Array";
             break;
         default:
-            if (field.resolve().resolvedType)
-                type = exportName(field.resolvedType, !(field.resolvedType instanceof protobuf.Enum || config.forceMessage));
-            else
+            if (field.resolve().resolvedType) {
+                type = exportName(field.resolvedType, asInterface);
+            }
+            else {
                 type = "*"; // should not happen
+            }
             break;
     }
     if (field.map)
@@ -354,8 +362,72 @@ function toJsType(field) {
     return type;
 }
 
+function syntaxForType(type) {
+
+    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;
+        }
+    }
+
+    return syntax !== null ? syntax : "proto2";
+}
+
+function isExplicitPresence(field, syntax) {
+
+    // In proto3, optional fields are explicit
+    if (syntax === "proto3") {
+        return field.options != null && field.options["proto3_optional"] === true;
+    }
+
+    // In proto2, fields are explicitly optional if they are not part of a map, array or oneOf group
+    if (syntax === "proto2") {
+        return field.optional && !(field.partOf || field.repeated || field.map);
+    }
+
+    throw new Error("Unknown proto syntax: [" + syntax + "]");
+}
+
+function isImplicitPresence(field, syntax) {
+
+    // In proto3, everything not marked optional has implicit presence (including maps and repeated fields)
+    if (syntax === "proto3") {
+        return field.options == null || field.options["proto3_optional"] !== true;
+    }
+
+    // In proto2, nothing has implicit presence
+    if (syntax === "proto2") {
+        return false;
+    }
+
+    throw new Error("Unknown proto syntax: [" + syntax + "]");
+}
+
+function isOptionalOneOf(oneof, syntax) {
+
+    if (syntax === "proto2") {
+        return false;
+    }
+
+    if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1) {
+        return false;
+    }
+
+    var field = oneof.fieldsArray[0];
+
+    return field.options != null && field.options["proto3_optional"] === true;
+}
+
 function buildType(ref, type) {
 
+    var syntax = syntaxForType(type);
+
     if (config.comments) {
         var typeDef = [
             "Properties of " + aOrAn(type.name) + ".",
@@ -365,10 +437,30 @@ function buildType(ref, type) {
         type.fieldsArray.forEach(function(field) {
             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 jsType = toJsType(field, /* parentIsInterface = */ true);
+            var nullable = false;
+            if (config["null-semantics"]) {
+                // With semantic nulls, only explicit optional fields and one-of members can be set to null
+                // Implicit fields (proto3), maps and lists can be omitted, but if specified must be non-null
+                // Implicit fields will take their default value when the message is constructed
+                if (isExplicitPresence(field, syntax) || field.partOf) {
+                    jsType = jsType + "|null|undefined";
+                    nullable = true;
+                }
+                else if (isImplicitPresence(field, syntax) || field.repeated || field.map) {
+                    jsType = jsType + "|undefined";
+                    nullable = true;
+                }
+            }
+            else {
+                // Without semantic nulls, everything is optional in proto3
+                // Do not allow |undefined to keep backwards compatibility
+                if (field.optional) {
+                    jsType = jsType + "|null";
+                    nullable = true;
+                }
+            }
+            typeDef.push("@property {" + jsType + "} " + (nullable ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name));
         });
         push("");
         pushComment(typeDef);
@@ -393,9 +485,22 @@ function buildType(ref, type) {
         var prop = util.safeProp(field.name);
         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";
+            var jsType = toJsType(field, /* parentIsInterface = */ false);
+            if (config["null-semantics"]) {
+                // With semantic nulls, fields are nullable if they are explicitly optional or part of a one-of
+                // Maps, repeated values and fields with implicit defaults are never null after construction
+                // Members are never undefined, at a minimum they are initialized to null
+                if (isExplicitPresence(field, syntax) || field.partOf) {
+                    jsType = jsType + "|null";
+                }
+            }
+            else {
+                // Without semantic nulls, everything is optional in proto3
+                // Keep |undefined for backwards compatibility
+                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 +511,16 @@ function buildType(ref, type) {
             push("");
             firstField = false;
         }
+        // With semantic nulls, only explict optional fields and one-of members are null by default
+        // Otherwise use field.optional, which doesn't consider proto3, maps, repeated fields etc.
+        var nullDefault = config["null-semantics"]
+            ? isExplicitPresence(field, syntax)
+            : 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("
@@ -436,12 +546,17 @@ function buildType(ref, type) {
         }
         oneof.resolve();
         push("");
-        pushComment([
-            oneof.comment || type.name + " " + oneof.name + ".",
-            "@member {" + oneof.oneof.map(JSON.stringify).join("|") + "|undefined} " + escapeName(oneof.name),
-            "@memberof " + exportName(type),
-            "@instance"
-        ]);
+        if (isOptionalOneOf(oneof, syntax)) {
+            push("// Virtual OneOf for proto3 optional field");
+        }
+        else {
+            pushComment([
+                oneof.comment || type.name + " " + oneof.name + ".",
+                "@member {" + oneof.oneof.map(JSON.stringify).join("|") + "|undefined} " + escapeName(oneof.name),
+                "@memberof " + exportName(type),
+                "@instance"
+            ]);
+        }
         push("Object.defineProperty(" + escapeName(type.name) + ".prototype, " + JSON.stringify(oneof.name) +", {");
         ++indent;
             push("get: $util.oneOfGetter($oneOfFields = [" + oneof.oneof.map(JSON.stringify).join(", ") + "]),");

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,78 @@ tape.test("with null-defaults, absent optional fields have null values", functio
 });
 
 
+tape.test("with --null-semantics, 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,
+            "null-semantics": true,
+        }, function(err, jsCode) {
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {OptionalFields.ISubMessage|null|undefined} [a] OptionalFields a"), "Property for a should use an interface")
+            test.ok(jsCode.includes("@member {OptionalFields.SubMessage|null} a"), "Member for a should use a message type")
+            test.ok(jsCode.includes("OptionalFields.prototype.a = null;"), "Initializer for a should be null")
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null} 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 --null-semantics, 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,
+            "null-semantics": true,
+        }, function(err, jsCode) {
+
+            test.error(err, 'static code generation worked');
+
+            test.ok(jsCode.includes("@property {OptionalFields.ISubMessage|null|undefined} [a] OptionalFields a"), "Property for a should use an interface")
+            test.ok(jsCode.includes("@member {OptionalFields.SubMessage|null} a"), "Member for a should use a message type")
+            test.ok(jsCode.includes("OptionalFields.prototype.a = null;"), "Initializer for a should be null")
+
+            test.ok(jsCode.includes("@property {number|null|undefined} [c] OptionalFields c"), "Property for c should be nullable")
+            test.ok(jsCode.includes("@member {number|null} 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|undefined} [d] OptionalFields d"), "Property for d should be optional but not 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;
 }