Skip to content

Commit 1af8454

Browse files
mkruskal-googlemkruskal-google
committed
Hook up field presence feature
1 parent 785416f commit 1af8454

6 files changed

Lines changed: 129 additions & 85 deletions

File tree

‎cli/targets/static.js‎

Lines changed: 18 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -362,58 +362,16 @@ function toJsType(field, parentIsInterface = false) {
362362
return type;
363363
}
364364

365-
function syntaxForType(type) {
366-
367-
var syntax = null;
368-
var namespace = type;
369-
370-
while (syntax === null && namespace !== null) {
371-
if (namespace.options != null && "syntax" in namespace.options) {
372-
syntax = namespace.options["syntax"];
373-
}
374-
else {
375-
namespace = namespace.parent;
376-
}
377-
}
378-
379-
return syntax !== null ? syntax : "proto2";
380-
}
381-
382-
function isExplicitPresence(field, syntax) {
383-
384-
// In proto3, optional fields are explicit
385-
if (syntax === "proto3") {
386-
return field.options != null && field.options["proto3_optional"] === true;
387-
}
388-
389-
// In proto2, fields are explicitly optional if they are not part of a map, array or oneOf group
390-
if (syntax === "proto2") {
391-
return field.optional && !(field.partOf || field.repeated || field.map);
392-
}
393-
394-
throw new Error("Unknown proto syntax: [" + syntax + "]");
395-
}
396-
397-
function isImplicitPresence(field, syntax) {
398-
399-
// In proto3, everything not marked optional has implicit presence (including maps and repeated fields)
400-
if (syntax === "proto3") {
401-
return field.options == null || field.options["proto3_optional"] !== true;
402-
}
403-
404-
// In proto2, nothing has implicit presence
405-
if (syntax === "proto2") {
365+
function hasPresence(field) {
366+
if (field.repeated || field.map) {
406367
return false;
407368
}
408-
409-
throw new Error("Unknown proto syntax: [" + syntax + "]");
369+
return field.partOf || // oneofs
370+
field.declaringField || field.extensionField || // extensions
371+
field._features.field_presence === "EXPLICIT";
410372
}
411373

412-
function isOptionalOneOf(oneof, syntax) {
413-
414-
if (syntax === "proto2") {
415-
return false;
416-
}
374+
function isProto3Optional(oneof) {
417375

418376
if (oneof.fieldsArray == null || oneof.fieldsArray.length !== 1) {
419377
return false;
@@ -426,8 +384,6 @@ function isOptionalOneOf(oneof, syntax) {
426384

427385
function buildType(ref, type) {
428386

429-
var syntax = syntaxForType(type);
430-
431387
if (config.comments) {
432388
var typeDef = [
433389
"Properties of " + aOrAn(type.name) + ".",
@@ -443,13 +399,15 @@ function buildType(ref, type) {
443399
// With semantic nulls, only explicit optional fields and one-of members can be set to null
444400
// Implicit fields (proto3), maps and lists can be omitted, but if specified must be non-null
445401
// Implicit fields will take their default value when the message is constructed
446-
if (isExplicitPresence(field, syntax) || field.partOf) {
447-
jsType = jsType + "|null|undefined";
448-
nullable = true;
449-
}
450-
else if (isImplicitPresence(field, syntax) || field.repeated || field.map) {
451-
jsType = jsType + "|undefined";
452-
nullable = true;
402+
if (field.optional) {
403+
if (hasPresence(field)) {
404+
jsType = jsType + "|null|undefined";
405+
nullable = true;
406+
}
407+
else {
408+
jsType = jsType + "|undefined";
409+
nullable = true;
410+
}
453411
}
454412
}
455413
else {
@@ -490,7 +448,7 @@ function buildType(ref, type) {
490448
// With semantic nulls, fields are nullable if they are explicitly optional or part of a one-of
491449
// Maps, repeated values and fields with implicit defaults are never null after construction
492450
// Members are never undefined, at a minimum they are initialized to null
493-
if (isExplicitPresence(field, syntax) || field.partOf) {
451+
if (hasPresence(field) && field.optional) {
494452
jsType = jsType + "|null";
495453
}
496454
}
@@ -514,7 +472,7 @@ function buildType(ref, type) {
514472
// With semantic nulls, only explict optional fields and one-of members are null by default
515473
// Otherwise use field.optional, which doesn't consider proto3, maps, repeated fields etc.
516474
var nullDefault = config["null-semantics"]
517-
? isExplicitPresence(field, syntax)
475+
? hasPresence(field) && field.optional
518476
: field.optional && config["null-defaults"];
519477
if (field.repeated)
520478
push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor
@@ -546,7 +504,7 @@ function buildType(ref, type) {
546504
}
547505
oneof.resolve();
548506
push("");
549-
if (isOptionalOneOf(oneof, syntax)) {
507+
if (isProto3Optional(oneof)) {
550508
push("// Virtual OneOf for proto3 optional field");
551509
}
552510
else {

‎src/field.js‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -105,18 +105,6 @@ function Field(name, id, type, rule, extend, options, comment) {
105105
*/
106106
this.extend = extend || undefined; // toJSON
107107

108-
/**
109-
* Whether this field is required.
110-
* @type {boolean}
111-
*/
112-
this.required = rule === "required";
113-
114-
/**
115-
* Whether this field is optional.
116-
* @type {boolean}
117-
*/
118-
this.optional = !this.required;
119-
120108
/**
121109
* Whether this field is repeated.
122110
* @type {boolean}
@@ -190,6 +178,30 @@ function Field(name, id, type, rule, extend, options, comment) {
190178
this.comment = comment;
191179
}
192180

181+
/**
182+
* Determines whether this field is required.
183+
* @name Field#required
184+
* @type {boolean}
185+
* @readonly
186+
*/
187+
Object.defineProperty(Field.prototype, "required", {
188+
get: function() {
189+
return this._features.field_presence === "LEGACY_REQUIRED";
190+
}
191+
});
192+
193+
/**
194+
* Determines whether this field is not required.
195+
* @name Field#optional
196+
* @type {boolean}
197+
* @readonly
198+
*/
199+
Object.defineProperty(Field.prototype, "optional", {
200+
get: function() {
201+
return !this.required;
202+
}
203+
});
204+
193205
/**
194206
* Determines whether this field is packed. Only relevant when repeated.
195207
* @name Field#packed
@@ -320,6 +332,9 @@ Field.prototype._inferLegacyProtoFeatures = function _inferLegacyProtoFeatures(e
320332
if (edition) return {};
321333

322334
var features = {};
335+
if (this.rule === "required") {
336+
features.field_presence = "LEGACY_REQUIRED";
337+
}
323338
if (this.getOption("packed") === true) {
324339
features.repeated_field_encoding = "PACKED";
325340
} else if (this.getOption("packed") === false) {

‎tests/cli.js‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,45 @@ tape.test("with --null-semantics, optional fields are handled correctly in proto
236236
});
237237
});
238238

239+
tape.test("with --null-semantics, optional fields are handled correctly in editions", function(test) {
240+
cliTest(test, function() {
241+
var root = protobuf.loadSync("tests/data/cli/null-defaults-edition2023.proto");
242+
root.resolveAll();
243+
244+
var staticTarget = require("../cli/targets/static");
245+
246+
staticTarget(root, {
247+
create: true,
248+
decode: true,
249+
encode: true,
250+
convert: true,
251+
comments: true,
252+
"null-semantics": true,
253+
}, function(err, jsCode) {
254+
255+
test.error(err, 'static code generation worked');
256+
257+
test.ok(jsCode.includes("@property {OptionalFields.ISubMessage|null|undefined} [a] OptionalFields a"), "Property for a should use an interface")
258+
test.ok(jsCode.includes("@member {OptionalFields.SubMessage|null} a"), "Member for a should use a message type")
259+
test.ok(jsCode.includes("OptionalFields.prototype.a = null;"), "Initializer for a should be null")
260+
261+
test.ok(jsCode.includes("@property {string|null|undefined} [e] OptionalFields e"), "Property for e should be nullable")
262+
test.ok(jsCode.includes("@member {string|null} e"), "Member for e should be nullable")
263+
test.ok(jsCode.includes("OptionalFields.prototype.e = null;"), "Initializer for e should be null")
264+
265+
test.ok(jsCode.includes("@property {number} r OptionalFields r"), "Property for r should not be nullable")
266+
test.ok(jsCode.includes("@member {number} r"), "Member for r should not be nullable")
267+
test.ok(jsCode.includes("OptionalFields.prototype.r = 0;"), "Initializer for r should be zero")
268+
269+
test.ok(jsCode.includes("@property {number|undefined} [i] OptionalFields i"), "Property for i should be optional but not nullable")
270+
test.ok(jsCode.includes("@member {number} i"), "Member for i should not be nullable")
271+
test.ok(jsCode.includes("OptionalFields.prototype.i = 0;"), "Initializer for i should be zero")
272+
273+
test.end();
274+
});
275+
});
276+
});
277+
239278

240279
tape.test("pbjs generates static code with message filter", function (test) {
241280
cliTest(test, function () {

‎tests/data/cli/null-defaults-edition2023.proto‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
edition = "2023";
2+
3+
message OptionalFields {
4+
message SubMessage {
5+
string a = 1 [features.field_presence = IMPLICIT];
6+
}
7+
8+
SubMessage a = 1;
9+
string e = 2;
10+
uint32 r = 3 [features.field_presence = LEGACY_REQUIRED];
11+
uint32 i = 4 [features.field_presence = IMPLICIT];
12+
}

‎tests/feature_resolution_editions.js‎

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -445,12 +445,13 @@ tape.test("feature resolution inferred proto2 repeated encoding", function(test)
445445
repeated int32 unpacked = 3 [packed = false];
446446
}`).root.resolveAll();
447447

448-
test.notOk(root.lookup("Message").fields.default.packed)
449-
test.equal(root.lookup("Message").fields.default._features.repeated_field_encoding, "EXPANDED")
450-
test.ok(root.lookup("Message").fields.packed.packed)
451-
test.equal(root.lookup("Message").fields.packed._features.repeated_field_encoding, "PACKED")
452-
test.notOk(root.lookup("Message").fields.unpacked.packed)
453-
test.equal(root.lookup("Message").fields.unpacked._features.repeated_field_encoding, "EXPANDED")
448+
var msg = root.lookup("Message");
449+
test.notOk(msg.fields.default.packed)
450+
test.equal(msg.fields.default._features.repeated_field_encoding, "EXPANDED")
451+
test.ok(msg.fields.packed.packed)
452+
test.equal(msg.fields.packed._features.repeated_field_encoding, "PACKED")
453+
test.notOk(msg.fields.unpacked.packed)
454+
test.equal(msg.fields.unpacked._features.repeated_field_encoding, "EXPANDED")
454455

455456
test.end();
456457
});
@@ -463,13 +464,32 @@ tape.test("feature resolution inferred proto3 repeated encoding", function(test)
463464
repeated int32 unpacked = 3 [packed = false];
464465
}`).root.resolveAll();
465466

466-
test.ok(root.lookup("Message").fields.default.packed)
467-
test.equal(root.lookup("Message").fields.default._features.repeated_field_encoding, "PACKED")
468-
test.ok(root.lookup("Message").fields.packed.packed)
469-
test.equal(root.lookup("Message").fields.packed._features.repeated_field_encoding, "PACKED")
470-
test.notOk(root.lookup("Message").fields.unpacked.packed)
471-
test.equal(root.lookup("Message").fields.unpacked._features.repeated_field_encoding, "EXPANDED")
467+
var msg = root.lookup("Message");
468+
test.ok(msg.fields.default.packed)
469+
test.equal(msg.fields.default._features.repeated_field_encoding, "PACKED")
470+
test.ok(msg.fields.packed.packed)
471+
test.equal(msg.fields.packed._features.repeated_field_encoding, "PACKED")
472+
test.notOk(msg.fields.unpacked.packed)
473+
test.equal(msg.fields.unpacked._features.repeated_field_encoding, "EXPANDED")
472474

473475
test.end();
474476
});
475477

478+
479+
tape.test("feature resolution inferred proto2 presence", function(test) {
480+
var root = protobuf.parse(`syntax = "proto2";
481+
message Message {
482+
optional int32 default = 1;
483+
required int32 required = 2;
484+
}`).root.resolveAll();
485+
486+
var msg = root.lookup("Message");
487+
test.ok(msg.fields.default.optional)
488+
test.notOk(msg.fields.default.required)
489+
test.equal(msg.fields.default._features.field_presence, "EXPLICIT")
490+
test.notOk(msg.fields.required.optional)
491+
test.ok(msg.fields.required.required)
492+
test.equal(msg.fields.required._features.field_presence, "LEGACY_REQUIRED")
493+
494+
test.end();
495+
});

‎tests/other_protocolerror.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ tape.test("a protocol error", function(test) {
1515
).add(
1616
new protobuf.Field("bar", 2, "string", "required")
1717
)
18-
);
18+
).resolveAll();
1919

2020
var Test = root.lookup("Test");
2121
var buf = protobuf.util.newBuffer(2);

0 commit comments

Comments
 (0)