diff --git a/cli/README.md b/cli/README.md index e7f189007..912e8bcd7 100644 --- a/cli/README.md +++ b/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] - ``` diff --git a/cli/pbjs.js b/cli/pbjs.js index 150a2b6f0..c276ce464 100644 --- a/cli/pbjs.js +++ b/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] -", "" diff --git a/cli/targets/static.js b/cli/targets/static.js index c130d9026..3b4cf05b3 100644 --- a/cli/targets/static.js +++ b/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(", ") + "]),"); diff --git a/src/parse.js b/src/parse.js index f5f070e3f..6e188b162 100644 --- a/src/parse.js +++ b/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(";"); } diff --git a/tests/cli.js b/tests/cli.js index 4e27dfc80..3208edfc1 100644 --- a/tests/cli.js +++ b/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"); diff --git a/tests/data/cli/null-defaults-proto3.proto b/tests/data/cli/null-defaults-proto3.proto new file mode 100644 index 000000000..4a17e8bd4 --- /dev/null +++ b/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; +} diff --git a/tests/data/cli/null-defaults.proto b/tests/data/cli/null-defaults.proto index 6a140b1e2..d182a01c3 100644 --- a/tests/data/cli/null-defaults.proto +++ b/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; }