From b38a668f495a665d6c42b9e1f27cc6565fc35f4c Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 27 Jul 2022 11:52:46 +0100 Subject: [PATCH 01/26] Fixes optional handling for class-level comments (JS) and interfaces (TS) --- cli/targets/static.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index c130d9026..1ec28bcae 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -354,6 +354,14 @@ function toJsType(field) { return type; } +function handleOptionalFields(jsType, field) { + + if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf) + return jsType + "|null|undefined"; + else + return jsType +} + function buildType(ref, type) { if (config.comments) { @@ -366,8 +374,7 @@ 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"; + jsType = handleOptionalFields(jsType, field); typeDef.push("@property {" + jsType + "} " + (field.optional ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name)); }); push(""); @@ -394,8 +401,7 @@ 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"; + jsType = handleOptionalFields(jsType, field); pushComment([ field.comment || type.name + " " + field.name + ".", "@member {" + jsType + "} " + field.name, From 358d3364398d3f25695a497d7dce70aab4a04702 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Tue, 18 Oct 2022 09:05:58 +0100 Subject: [PATCH 02/26] Hide the fix for PB3 optional type declarations behind the flag --pb3-optional --- cli/README.md | 1 + cli/pbjs.js | 4 +++- cli/targets/static.js | 12 +++++++++++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/cli/README.md b/cli/README.md index e7f189007..88981dd91 100644 --- a/cli/README.md +++ b/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. + --pb3-optional Make type declarations respect optional fields for PB3. usage: pbjs [options] file1.proto file2.json ... (or pipe) other | pbjs [options] - ``` diff --git a/cli/pbjs.js b/cli/pbjs.js index 150a2b6f0..e6cb0d162 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", "pb3-optional" ], 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, + "pb3-optional": 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.", + " --pb3-optional Make type declarations respect optional fields for PB3.", "", "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 1ec28bcae..f5824c1f0 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -374,7 +374,16 @@ 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); - jsType = handleOptionalFields(jsType, field); + + // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output + if (config.pb3Optional) { + jsType = handleOptionalFields(jsType, field); + } + else { + if (field.optional) + jsType = jsType + "|null"; + } + typeDef.push("@property {" + jsType + "} " + (field.optional ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name)); }); push(""); @@ -401,6 +410,7 @@ function buildType(ref, type) { if (config.comments) { push(""); var jsType = toJsType(field); + // No need to check config.tsOptional for member types, this logic has not changed jsType = handleOptionalFields(jsType, field); pushComment([ field.comment || type.name + " " + field.name + ".", From 94556f9f1b8555f414accd7cc9c3a8d8d39b6e6e Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Tue, 18 Oct 2022 09:34:10 +0100 Subject: [PATCH 03/26] Do not use null-defaults to decide type signature for pb3-optionals (this only sets the default of optional fields, it doesn't control whether a field is optional or not) --- cli/README.md | 3 ++- cli/targets/static.js | 14 +++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/cli/README.md b/cli/README.md index 88981dd91..749bb0352 100644 --- a/cli/README.md +++ b/cli/README.md @@ -68,7 +68,8 @@ 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. - --pb3-optional Make type declarations respect optional fields for PB3. + --null-defaults Default value for optional fields is null instead of zero value. + --pb3-optional Make type declarations respect optional fields for PB3. usage: pbjs [options] file1.proto file2.json ... (or pipe) other | pbjs [options] - ``` diff --git a/cli/targets/static.js b/cli/targets/static.js index f5824c1f0..385889250 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -356,7 +356,7 @@ function toJsType(field) { function handleOptionalFields(jsType, field) { - if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf) + if (field.optional && !field.map && !field.repeated && field.resolvedType instanceof Type || field.partOf) return jsType + "|null|undefined"; else return jsType @@ -410,8 +410,16 @@ function buildType(ref, type) { if (config.comments) { push(""); var jsType = toJsType(field); - // No need to check config.tsOptional for member types, this logic has not changed - jsType = handleOptionalFields(jsType, field); + + // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output + if (config.pb3Optional) { + jsType = handleOptionalFields(jsType, field); + } + 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, From 2bd0b43444e99b5c7cd76955b2378d5cb44b6347 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 24 Jul 2024 16:25:56 +0100 Subject: [PATCH 04/26] New implementation that explicitly respects the optional keyword --- cli/README.md | 3 +-- cli/pbjs.js | 8 ++++---- cli/targets/static.js | 33 ++++++++++++++++++--------------- src/field.js | 8 ++++++++ 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/cli/README.md b/cli/README.md index 749bb0352..1a24fa4df 100644 --- a/cli/README.md +++ b/cli/README.md @@ -68,8 +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. - --null-defaults Default value for optional fields is null instead of zero value. - --pb3-optional Make type declarations respect optional fields for PB3. + --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] - ``` diff --git a/cli/pbjs.js b/cli/pbjs.js index e6cb0d162..8167f2289 100644 --- a/cli/pbjs.js +++ b/cli/pbjs.js @@ -62,8 +62,8 @@ exports.main = function main(args, callback) { "force-number": false, "force-enum-string": false, "force-message": false, - "null-defaults": false, - "pb3-optional": false + "force-optional": false, + "null-defaults": false } }); @@ -147,9 +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.", - " --pb3-optional Make type declarations respect optional fields for PB3.", + " --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] -", "" diff --git a/cli/targets/static.js b/cli/targets/static.js index 385889250..6e8747541 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -354,14 +354,6 @@ function toJsType(field) { return type; } -function handleOptionalFields(jsType, field) { - - if (field.optional && !field.map && !field.repeated && field.resolvedType instanceof Type || field.partOf) - return jsType + "|null|undefined"; - else - return jsType -} - function buildType(ref, type) { if (config.comments) { @@ -375,10 +367,13 @@ function buildType(ref, type) { prop = prop.substring(1, prop.charAt(0) === "[" ? prop.length - 1 : prop.length); var jsType = toJsType(field); - // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output - if (config.pb3Optional) { - jsType = handleOptionalFields(jsType, field); + // 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 (field.explicitOptional || field.partOf) + jsType = jsType + "|null|undefined"; } + // Old behaviour - field.optional is true for all fields in proto3 else { if (field.optional) jsType = jsType + "|null"; @@ -411,10 +406,13 @@ function buildType(ref, type) { push(""); var jsType = toJsType(field); - // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output - if (config.pb3Optional) { - jsType = handleOptionalFields(jsType, field); + // 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 (field.explicitOptional || 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"; @@ -430,11 +428,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"] + ? field.explicitOptional + : 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(" diff --git a/src/field.js b/src/field.js index e0feb8b43..9c650d630 100644 --- a/src/field.js +++ b/src/field.js @@ -117,6 +117,14 @@ function Field(name, id, type, rule, extend, options, comment) { */ this.optional = !this.required; + /** + * Whether this field is explicitly marked as optional + * Proto3 fields must use the optional keyword + * Maps and repeated fields are not explicitly optional + * @type {boolean} + */ + this.explicitOptional = rule === "optional"; + /** * Whether this field is repeated. * @type {boolean} From 479375556ff1d6b5c7c3e58dd2414f8c6d51b0e1 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 24 Jul 2024 16:28:48 +0100 Subject: [PATCH 05/26] Fix options list in pbjs --- cli/pbjs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/pbjs.js b/cli/pbjs.js index 8167f2289..c8d9a8a1d 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", "pb3-optional" ], + 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, From f2a69a5c1dafdddcd0a6fccc41af5843308c6800 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 24 Jul 2024 18:30:11 +0100 Subject: [PATCH 06/26] Add tests for handling optional fields in both proto2 and proto3 syntax --- tests/cli.js | 66 +++++++++++++++++++++++ tests/data/cli/null-defaults-proto3.proto | 12 +++++ tests/data/cli/null-defaults.proto | 1 + 3 files changed, 79 insertions(+) create mode 100644 tests/data/cli/null-defaults-proto3.proto diff --git a/tests/cli.js b/tests/cli.js index 4e27dfc80..0460e6b43 100644 --- a/tests/cli.js +++ b/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"); 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; } From 996e445f59b8179703bc316c35d4c4852f2a20cb Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 24 Jul 2024 18:42:01 +0100 Subject: [PATCH 07/26] Explicit handling of proto3 syntax in the parser and static generator --- cli/targets/static.js | 36 +++++++++++++++++++++++++++++++----- src/field.js | 15 +++++++-------- src/parse.js | 4 ++++ 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 6e8747541..0d9c73ff2 100644 --- a/cli/targets/static.js +++ b/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,20 +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); + 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 (field.explicitOptional || field.partOf) + 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) + if (field.optional) { jsType = jsType + "|null"; + nullable = true; + } } - typeDef.push("@property {" + jsType + "} " + (field.optional ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name)); + typeDef.push("@property {" + jsType + "} " + (nullable ? "[" + prop + "]" : prop) + " " + (field.comment || type.name + " " + field.name)); }); push(""); pushComment(typeDef); @@ -409,7 +435,7 @@ function buildType(ref, type) { // 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 (field.explicitOptional || field.partOf) + if (isOptional(type, field) || field.partOf) jsType = jsType + "|null|undefined"; } // Old behaviour - field.optional is true for all fields in proto3 @@ -431,7 +457,7 @@ function buildType(ref, type) { // 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"] - ? field.explicitOptional + ? isOptional(type, field) : field.optional && config["null-defaults"]; if (field.repeated) push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor diff --git a/src/field.js b/src/field.js index 9c650d630..7b018f7ab 100644 --- a/src/field.js +++ b/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} @@ -117,14 +124,6 @@ function Field(name, id, type, rule, extend, options, comment) { */ this.optional = !this.required; - /** - * Whether this field is explicitly marked as optional - * Proto3 fields must use the optional keyword - * Maps and repeated fields are not explicitly optional - * @type {boolean} - */ - this.explicitOptional = rule === "optional"; - /** * Whether this field is repeated. * @type {boolean} diff --git a/src/parse.js b/src/parse.js index f5f070e3f..155cd81c5 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(";"); } From aa61793c1c5cb17ff7c88c62eeeb11975e1f72e6 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Wed, 24 Jul 2024 19:18:48 +0100 Subject: [PATCH 08/26] Only look up proto syntax once per type in the static generator --- cli/targets/static.js | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 0d9c73ff2..a504c5af6 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -354,21 +354,26 @@ 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 +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"] + syntax = namespace.options["syntax"]; else - namespace = namespace.parent + namespace = namespace.parent; } + if (syntax === "proto3") + return "proto3"; + else + return "proto2"; +} + +function isOptional(field, syntax) { + if (syntax === "proto3") return field.proto3Optional else @@ -377,6 +382,8 @@ function isOptional(type, field) { function buildType(ref, type) { + var syntax = syntaxForType(type); + if (config.comments) { var typeDef = [ "Properties of " + aOrAn(type.name) + ".", @@ -392,7 +399,7 @@ function buildType(ref, type) { // 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) { + if (isOptional(field, syntax) || field.partOf) { jsType = jsType + "|null|undefined"; nullable = true; } @@ -435,7 +442,7 @@ function buildType(ref, type) { // 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) + if (isOptional(field, syntax) || field.partOf) jsType = jsType + "|null|undefined"; } // Old behaviour - field.optional is true for all fields in proto3 @@ -457,7 +464,7 @@ function buildType(ref, type) { // 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) + ? isOptional(field, syntax) : field.optional && config["null-defaults"]; if (field.repeated) push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor From f2ed6e0021e1a6fddfff8f725440b24bd034b1bb Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Thu, 25 Jul 2024 11:47:27 +0100 Subject: [PATCH 09/26] Fix lint warnings --- cli/targets/static.js | 10 ++-------- src/parse.js | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index a504c5af6..4fbbd3b7f 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -366,18 +366,12 @@ function syntaxForType(type) { namespace = namespace.parent; } - if (syntax === "proto3") - return "proto3"; - else - return "proto2"; + return (syntax === "proto3") ? "proto3" : "proto2"; } function isOptional(field, syntax) { - if (syntax === "proto3") - return field.proto3Optional - else - return field.optional + return (syntax === "proto3") ? field.proto3Optional : field.optional; } function buildType(ref, type) { diff --git a/src/parse.js b/src/parse.js index 155cd81c5..6e188b162 100644 --- a/src/parse.js +++ b/src/parse.js @@ -265,7 +265,7 @@ function parse(source, root, options) { // 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) + root.setOption("syntax", syntax); skip(";"); } From 856af47881a0ca4c14f73a087f38a953f4a84637 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Thu, 25 Jul 2024 15:59:55 +0100 Subject: [PATCH 10/26] Use field options in proto3 instead of adding an extra property --- cli/targets/static.js | 7 ++++++- src/field.js | 7 ------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 4fbbd3b7f..b32b52d28 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -371,7 +371,12 @@ function syntaxForType(type) { function isOptional(field, syntax) { - return (syntax === "proto3") ? field.proto3Optional : field.optional; + // 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 + return field.optional && !(field.partOf || field.repeated || field.map); } function buildType(ref, type) { diff --git a/src/field.js b/src/field.js index 7b018f7ab..e0feb8b43 100644 --- a/src/field.js +++ b/src/field.js @@ -78,13 +78,6 @@ 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} From ccd640e62ba7974bea5755a2596238c2d89a6fe8 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Thu, 25 Jul 2024 16:18:32 +0100 Subject: [PATCH 11/26] Do not require repeated or map fields in the object properties (i.e. for interface types) --- cli/targets/static.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index b32b52d28..731e2fb27 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -395,11 +395,10 @@ function buildType(ref, type) { var jsType = toJsType(field); 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 + // New behaviour - respect explicit optional semantics in both proto2 and proto3 if (config["force-optional"]) { - if (isOptional(field, syntax) || field.partOf) { - jsType = jsType + "|null|undefined"; + if (isOptional(field, syntax) || field.partOf || field.repeated || field.map) { + jsType = jsType + "|null"; nullable = true; } } @@ -439,7 +438,7 @@ function buildType(ref, type) { var jsType = toJsType(field); // 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 + // Maps and repeated fields are not nullable, they default to empty instances if (config["force-optional"]) { if (isOptional(field, syntax) || field.partOf) jsType = jsType + "|null|undefined"; From 09f577a5abf8a53df39e438c655e8a739073a920 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Thu, 25 Jul 2024 16:19:44 +0100 Subject: [PATCH 12/26] Do not generate doc comments for virtual oneOfs --- cli/targets/static.js | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 731e2fb27..fbaae9f15 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -379,6 +379,19 @@ function isOptional(field, syntax) { return field.optional && !(field.partOf || field.repeated || field.map); } +function isOptionalOneOf(oneof, syntax) { + + if (syntax !== "proto3") + 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); @@ -494,12 +507,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(", ") + "]),"); From 6204b2dee90691e147cca6c456b13f2ff88c43a5 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:06:43 +0100 Subject: [PATCH 13/26] Address codestyle comment (braces for if/else) --- cli/targets/static.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index fbaae9f15..0e2775c87 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -360,10 +360,12 @@ function syntaxForType(type) { var namespace = type; while (syntax === null && namespace !== null) { - if (namespace.options != null && "syntax" in namespace.options) + if (namespace.options != null && "syntax" in namespace.options) { syntax = namespace.options["syntax"]; - else + } + else { namespace = namespace.parent; + } } return (syntax === "proto3") ? "proto3" : "proto2"; From 31f85e943c27612f9d1221e4751741622b6e3c00 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:07:33 +0100 Subject: [PATCH 14/26] Address comment for default syntax --- cli/targets/static.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 0e2775c87..0298eb32c 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -368,7 +368,7 @@ function syntaxForType(type) { } } - return (syntax === "proto3") ? "proto3" : "proto2"; + return (syntax !== null) ? syntax : "proto2"; } function isOptional(field, syntax) { From ed5980cadc40a4888e6743773e1d8d5f81f04b96 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:41:24 +0100 Subject: [PATCH 15/26] Change config flag to --null-semantics and update comments --- cli/targets/static.js | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 0298eb32c..17dfc4996 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -409,22 +409,23 @@ function buildType(ref, type) { prop = prop.substring(1, prop.charAt(0) === "[" ? prop.length - 1 : prop.length); var jsType = toJsType(field); var nullable = false; - - // New behaviour - respect explicit optional semantics in both proto2 and proto3 - if (config["force-optional"]) { + if (config["null-semantics"]) { + // With semantic nulls, decide which fields are required for the current protobuf version + // Fields with implicit defaults in proto3 are required for the purpose of constructing objects + // Optional fields can be undefined, i.e. they can be omitted for the source object altogether if (isOptional(field, syntax) || field.partOf || field.repeated || field.map) { - jsType = jsType + "|null"; + jsType = jsType + "|null|undefined"; nullable = true; } } - // Old behaviour - field.optional is true for all fields in proto3 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(""); @@ -451,19 +452,19 @@ function buildType(ref, type) { if (config.comments) { push(""); var jsType = toJsType(field); - - // New behaviour - fields explicitly marked as optional and members of a one-of are nullable - // Maps and repeated fields are not nullable, they default to empty instances - if (config["force-optional"]) { + 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 (isOptional(field, syntax) || field.partOf) - jsType = jsType + "|null|undefined"; + jsType = jsType + "|null"; } - // Old behaviour - field.optional is true for all fields in proto3 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, @@ -474,9 +475,9 @@ 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"] + // Semantic nulls respect the optional semantics for the current protobuf version + // Otherwise use field.optional, which doesn't consider proto3, maps, repeated fields etc. + var nullDefault = config["null-semantics"] ? isOptional(field, syntax) : field.optional && config["null-defaults"]; if (field.repeated) From bee997d5e86a85a395b76a6551570e8b756ce8d1 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:43:14 +0100 Subject: [PATCH 16/26] Update PBJS and README for new --null-semantics flag --- cli/README.md | 4 +++- cli/pbjs.js | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cli/README.md b/cli/README.md index 1a24fa4df..7fb6da453 100644 --- a/cli/README.md +++ b/cli/README.md @@ -68,7 +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. - --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3) + + --null-semantics Make nullable fields match protobuf semantics (including the optional keyword) + --null-defaults Default value for optional fields is null instead of zero value (no effect if --null-semantics is specified) usage: pbjs [options] file1.proto file2.json ... (or pipe) other | pbjs [options] - ``` diff --git a/cli/pbjs.js b/cli/pbjs.js index c8d9a8a1d..1d238a090 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", "force-optional", "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-semantics", "null-defaults"], default: { target: "json", create: true, @@ -62,7 +62,7 @@ exports.main = function main(args, callback) { "force-number": false, "force-enum-string": false, "force-message": false, - "force-optional": false, + "null-semantics": false, "null-defaults": false } }); @@ -147,9 +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 (deprecated, use --force-optional instead)", + " --null-semantics Make nullable fields match protobuf semantics (including the optional keyword)", + " --null-defaults Default value for optional fields is null instead of zero value (no effect if --null-semantics is specified)", "", "usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray(" (or pipe) ") + "other | " + chalk.bold.green("pbjs") + " [options] -", "" From dee30839b0719eda5806442b9945a92885e4b8cf Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:45:17 +0100 Subject: [PATCH 17/26] Update tests for --null-semantics flag --- tests/cli.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/cli.js b/tests/cli.js index 0460e6b43..bf3273b2b 100644 --- a/tests/cli.js +++ b/tests/cli.js @@ -165,7 +165,7 @@ 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) { +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(); @@ -178,13 +178,13 @@ tape.test("with force-optional, optional fields are handled correctly in proto2" encode: true, convert: true, comments: true, - "force-optional": true, + "null-semantics": 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("@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") @@ -197,7 +197,7 @@ tape.test("with force-optional, optional fields are handled correctly in proto2" }); -tape.test("with force-optional, optional fields are handled correctly in proto3", function(test) { +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(); @@ -210,15 +210,13 @@ tape.test("with force-optional, optional fields are handled correctly in proto3" encode: true, convert: true, comments: true, - "force-optional": true, + "null-semantics": 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("@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") From 50255d08621f9c1c2f66509ac4b296dd3b32cfb2 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 15:45:29 +0100 Subject: [PATCH 18/26] Fix one lint warning --- cli/targets/static.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 17dfc4996..d2d654d5d 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -368,7 +368,7 @@ function syntaxForType(type) { } } - return (syntax !== null) ? syntax : "proto2"; + return syntax !== null ? syntax : "proto2"; } function isOptional(field, syntax) { From 623477b6986f8d31fb5e2a77cbf33f4d0ac53146 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 16:00:15 +0100 Subject: [PATCH 19/26] Include tests for interface / message types under --null-semantics --- tests/cli.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/cli.js b/tests/cli.js index bf3273b2b..5f78a25c7 100644 --- a/tests/cli.js +++ b/tests/cli.js @@ -183,6 +183,10 @@ tape.test("with --null-semantics, optional fields are handled correctly in proto 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") @@ -215,6 +219,10 @@ tape.test("with --null-semantics, optional fields are handled correctly in proto 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") From d5700a5a6a62b82d35588b06667a5c0a6bec46ae Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 16:00:54 +0100 Subject: [PATCH 20/26] Implement propagation of interface / message types under --null-semantics flag --- cli/targets/static.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index d2d654d5d..c175b16dd 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) { 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": @@ -342,7 +348,7 @@ function toJsType(field) { break; default: if (field.resolve().resolvedType) - type = exportName(field.resolvedType, !(field.resolvedType instanceof protobuf.Enum || config.forceMessage)); + type = exportName(field.resolvedType, asInterface); else type = "*"; // should not happen break; @@ -407,7 +413,7 @@ 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); + var jsType = toJsType(field, /* parentIsInterface = */ true); var nullable = false; if (config["null-semantics"]) { // With semantic nulls, decide which fields are required for the current protobuf version @@ -451,7 +457,7 @@ function buildType(ref, type) { var prop = util.safeProp(field.name); if (config.comments) { push(""); - var jsType = toJsType(field); + 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 From 9bcc00610fad5029f244d5420d7e3f8f5c523a04 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 16:05:43 +0100 Subject: [PATCH 21/26] Update CLI options help and README --- cli/README.md | 4 ++-- cli/pbjs.js | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cli/README.md b/cli/README.md index 7fb6da453..912e8bcd7 100644 --- a/cli/README.md +++ b/cli/README.md @@ -69,8 +69,8 @@ Translates between file formats and generates static code. --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-semantics Make nullable fields match protobuf semantics (including the optional keyword) - --null-defaults Default value for optional fields is null instead of zero value (no effect if --null-semantics is specified) + --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 1d238a090..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-semantics", "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, @@ -62,8 +62,8 @@ exports.main = function main(args, callback) { "force-number": false, "force-enum-string": false, "force-message": false, - "null-semantics": false, - "null-defaults": false + "null-defaults": false, + "null-semantics": false } }); @@ -148,8 +148,8 @@ exports.main = function main(args, callback) { " --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-semantics Make nullable fields match protobuf semantics (including the optional keyword)", - " --null-defaults Default value for optional fields is null instead of zero value (no effect if --null-semantics is specified)", + " --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] -", "" From fda51db00c9e67212c17e20425259139653f085a Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 16:38:29 +0100 Subject: [PATCH 22/26] Allow for other syntax options than "proto2" or "proto3" --- cli/targets/static.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index c175b16dd..3924811cc 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -384,12 +384,15 @@ function isOptional(field, syntax) { 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 - return field.optional && !(field.partOf || field.repeated || field.map); + if (syntax === "proto2") + return field.optional && !(field.partOf || field.repeated || field.map); + + throw new Error("Unknown proto syntax: [" + syntax + "]"); } function isOptionalOneOf(oneof, syntax) { - if (syntax !== "proto3") + if (syntax === "proto2") return false; if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1) From 8e0027a193e9a3da1b3936c733c60dd5739b0026 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 20:26:52 +0100 Subject: [PATCH 23/26] Make parentIsInterface default to false in toJsType() Co-authored-by: Daniel Bankhead --- cli/targets/static.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 3924811cc..4471eecd7 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -311,7 +311,7 @@ function buildFunction(type, functionName, gen, scope) { push("};"); } -function toJsType(field, parentIsInterface) { +function toJsType(field, parentIsInterface = false) { var type; // With null semantics, interfaces are composed from interfaces and messages from messages From 94bfbbc18eeb462229ad6eb3f2c818ee728c980a Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 22:47:52 +0100 Subject: [PATCH 24/26] Allow undefined values (but not nulls) for implicit presence fields in properties / interfaces --- cli/targets/static.js | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 4471eecd7..077c15817 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -377,7 +377,7 @@ function syntaxForType(type) { return syntax !== null ? syntax : "proto2"; } -function isOptional(field, syntax) { +function isExplicitPresence(field, syntax) { // In proto3, optional fields are explicit if (syntax === "proto3") @@ -390,6 +390,19 @@ function isOptional(field, syntax) { 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") @@ -419,13 +432,17 @@ function buildType(ref, type) { var jsType = toJsType(field, /* parentIsInterface = */ true); var nullable = false; if (config["null-semantics"]) { - // With semantic nulls, decide which fields are required for the current protobuf version - // Fields with implicit defaults in proto3 are required for the purpose of constructing objects - // Optional fields can be undefined, i.e. they can be omitted for the source object altogether - if (isOptional(field, syntax) || field.partOf || field.repeated || field.map) { + // 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 @@ -465,7 +482,7 @@ function buildType(ref, type) { // 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 (isOptional(field, syntax) || field.partOf) + if (isExplicitPresence(field, syntax) || field.partOf) jsType = jsType + "|null"; } else { @@ -484,10 +501,10 @@ function buildType(ref, type) { push(""); firstField = false; } - // Semantic nulls respect the optional semantics for the current protobuf version + // 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"] - ? isOptional(field, syntax) + ? isExplicitPresence(field, syntax) : field.optional && config["null-defaults"]; if (field.repeated) push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor From 258679c2402d95105817b5c29ef9722cda2a0988 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Fri, 26 Jul 2024 23:11:42 +0100 Subject: [PATCH 25/26] Update tests for proto3 to let implicit members be optional but not nullable --- tests/cli.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cli.js b/tests/cli.js index 5f78a25c7..3208edfc1 100644 --- a/tests/cli.js +++ b/tests/cli.js @@ -227,7 +227,7 @@ tape.test("with --null-semantics, optional fields are handled correctly in proto 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("@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") From c478e1fe213f872fb580c22d4ef517710dae99b1 Mon Sep 17 00:00:00 2001 From: Martin Traverse Date: Mon, 29 Jul 2024 08:02:09 +0100 Subject: [PATCH 26/26] Add braces to if/else clauses for readability on all code touched by this change --- cli/targets/static.js | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/cli/targets/static.js b/cli/targets/static.js index 077c15817..3b4cf05b3 100644 --- a/cli/targets/static.js +++ b/cli/targets/static.js @@ -347,10 +347,12 @@ function toJsType(field, parentIsInterface = false) { type = "Uint8Array"; break; default: - if (field.resolve().resolvedType) + if (field.resolve().resolvedType) { type = exportName(field.resolvedType, asInterface); - else + } + else { type = "*"; // should not happen + } break; } if (field.map) @@ -380,12 +382,14 @@ function syntaxForType(type) { function isExplicitPresence(field, syntax) { // In proto3, optional fields are explicit - if (syntax === "proto3") + 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") + if (syntax === "proto2") { return field.optional && !(field.partOf || field.repeated || field.map); + } throw new Error("Unknown proto syntax: [" + syntax + "]"); } @@ -393,23 +397,27 @@ function isExplicitPresence(field, syntax) { function isImplicitPresence(field, syntax) { // In proto3, everything not marked optional has implicit presence (including maps and repeated fields) - if (syntax === "proto3") + if (syntax === "proto3") { return field.options == null || field.options["proto3_optional"] !== true; + } // In proto2, nothing has implicit presence - if (syntax === "proto2") + if (syntax === "proto2") { return false; + } throw new Error("Unknown proto syntax: [" + syntax + "]"); } function isOptionalOneOf(oneof, syntax) { - if (syntax === "proto2") + if (syntax === "proto2") { return false; + } - if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1) + if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1) { return false; + } var field = oneof.fieldsArray[0]; @@ -482,14 +490,16 @@ function buildType(ref, type) { // 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) + 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) + 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 + ".",