Skip to content

V2: Remove DescField.optional #808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timostamm merged 2 commits into from
Apr 24, 2024
Merged

V2: Remove DescField.optional #808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bece0f
V2: Remove DescField.optional
timostamm Apr 23, 2024
675148f
format
timostamm Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/protobuf-bench/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ server would usually do.

| code generator | bundle size | minified | compressed |
|---------------------|------------------------:|-----------------------:|-------------------:|
| protobuf-es | 127,054 b | 65,501 b | 15,983 b |
| protobuf-es | 126,666 b | 65,334 b | 15,946 b |
| protobuf-javascript | 394,384 b | 288,654 b | 45,122 b |
70 changes: 0 additions & 70 deletions packages/protobuf-test/src/reflect/registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,73 +802,6 @@ describe("DescField", () => {
).toBe(false);
});
});
describe("optional", () => {
test("false for proto2 required scalar", async () => {
const field = await compileField(`
syntax="proto2";
message M { required int32 f1 = 1; }
`);
expect(
field.fieldKind == "scalar" ||
field.fieldKind == "message" ||
field.fieldKind == "enum"
? field.optional
: undefined,
).toBe(false);
});
test("true for proto2 optional scalar", async () => {
const field = await compileField(`
syntax="proto2";
message M { required int32 f1 = 1; }
`);
expect(
field.fieldKind == "scalar" ||
field.fieldKind == "message" ||
field.fieldKind == "enum"
? field.optional
: undefined,
).toBe(false);
});
test("true for proto3 optional scalar", async () => {
const field = await compileField(`
syntax="proto3";
message M { optional int32 f1 = 1; }
`);
expect(
field.fieldKind == "scalar" ||
field.fieldKind == "message" ||
field.fieldKind == "enum"
? field.optional
: undefined,
).toBe(true);
});
test("false for features.field_presence = EXPLICIT", async () => {
const field = await compileField(`
edition="2023";
message M { int32 f1 = 1 [features.field_presence = EXPLICIT]; }
`);
expect(
field.fieldKind == "scalar" ||
field.fieldKind == "message" ||
field.fieldKind == "enum"
? field.optional
: undefined,
).toBe(false);
});
test("false for features.field_presence = IMPLICIT", async () => {
const field = await compileField(`
edition="2023";
message M { int32 f1 = 1 [features.field_presence = IMPLICIT]; }
`);
expect(
field.fieldKind == "scalar" ||
field.fieldKind == "message" ||
field.fieldKind == "enum"
? field.optional
: undefined,
).toBe(false);
});
});
describe("longType", () => {
test("returns default LongType.BIGINT for option omitted", async () => {
const { fields } = await compileMessage(`
Expand Down Expand Up @@ -1171,7 +1104,6 @@ describe("DescField", () => {
field.packed;

// exclusive to singular
field.optional;
const def: undefined = field.getDefaultValue();

// exclusive to map
Expand All @@ -1194,7 +1126,6 @@ describe("DescField", () => {
field.packed;

// exclusive to singular
field.optional;
const def: string | number | bigint | boolean | Uint8Array | undefined =
field.getDefaultValue();

Expand Down Expand Up @@ -1229,7 +1160,6 @@ describe("DescField", () => {
field.packed;

// exclusive to singular
field.optional;
const def: number | undefined = field.getDefaultValue();

// exclusive to map
Expand Down
4 changes: 0 additions & 4 deletions packages/protobuf/src/desc-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,6 @@ type descFieldSingularCommon = {
* This does not include synthetic oneofs for proto3 optionals.
*/
readonly oneof: DescOneof | undefined;
/**
* Whether this field was declared with `optional` in the protobuf source.
*/
readonly optional: boolean;
/**
* Presence of the field.
* See https://protobuf.dev/programming-guides/field_presence/
Expand Down
18 changes: 0 additions & 18 deletions packages/protobuf/src/reflect/registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,6 @@ const TYPE_ENUM: FieldDescriptorProto_Type.ENUM = 14;

// bootstrap-inject google.protobuf.FieldDescriptorProto.Label.LABEL_REPEATED: const $name: FieldDescriptorProto_Label.$localName = $number;
const LABEL_REPEATED: FieldDescriptorProto_Label.REPEATED = 3;
// bootstrap-inject google.protobuf.FieldDescriptorProto.Label.LABEL_OPTIONAL: const $name: FieldDescriptorProto_Label.$localName = $number;
const LABEL_OPTIONAL: FieldDescriptorProto_Label.OPTIONAL = 1;
// bootstrap-inject google.protobuf.FieldDescriptorProto.Label.LABEL_REQUIRED: const $name: FieldDescriptorProto_Label.$localName = $number;
const LABEL_REQUIRED: FieldDescriptorProto_Label.REQUIRED = 2;

Expand Down Expand Up @@ -909,7 +907,6 @@ function newField(
}
}
field.presence = getFieldPresence(proto, oneof, parentOrFile);
field.optional = isOptionalField(field as DescField | DescExtension);
return field as DescField | DescExtension;
}

Expand Down Expand Up @@ -1080,21 +1077,6 @@ function getFieldPresence(
return resolveFeature("fieldPresence", { proto, parent });
}

/**
* Did the user use the `optional` keyword?
*/
function isOptionalField(field: DescField | DescExtension): boolean {
const edition = (field.kind == "extension" ? field.file : field.parent.file)
.edition;
if (edition == EDITION_PROTO2) {
return !field.oneof && field.proto.label == LABEL_OPTIONAL;
}
if (edition == EDITION_PROTO3) {
return field.proto.proto3Optional;
}
return false;
}

/**
* Pack this repeated field?
*/
Expand Down
21 changes: 8 additions & 13 deletions packages/protoc-gen-es/src/protoc-gen-es-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@ import type {
Printable,
Target,
} from "@bufbuild/protoplugin/ecmascript";
import {
arrayLiteral,
fieldUsesPrototype,
functionCall,
getFieldTypeInfo,
} from "./util";
import { arrayLiteral, functionCall, fieldTypeScriptType } from "./util";
import { version } from "../package.json";

export const protocGenEs = createEcmaScriptPlugin({
Expand Down Expand Up @@ -92,7 +87,7 @@ function generateTs(schema: Schema) {
const { GenDescExtension, extDesc } = f.runtime.codegen;
const name = f.importDesc(desc).name;
const E = f.importShape(desc.extendee);
const V = getFieldTypeInfo(desc).typing;
const V = fieldTypeScriptType(desc).typing;
const call = functionCall(extDesc, [f.importDesc(file), ...pathInFileDesc(desc)]);
f.print(f.jsDoc(desc));
f.print(f.exportDecl("const", name), ": ", GenDescExtension, "<", E, ", ", V, ">", " = ", pure);
Expand Down Expand Up @@ -219,7 +214,7 @@ function generateDts(schema: Schema) {
const { GenDescExtension } = f.runtime.codegen;
const name = f.importDesc(desc).name;
const E = f.importShape(desc.extendee);
const V = getFieldTypeInfo(desc).typing;
const V = fieldTypeScriptType(desc).typing;
f.print(f.jsDoc(desc));
f.print(f.exportDecl("declare const", name), ": ", GenDescExtension, "<", E, ", ", V, ">;");
f.print();
Expand Down Expand Up @@ -310,19 +305,19 @@ function generateMessageShape(f: GeneratedFile, message: DescMessage, target: Ex
f.print(` } | {`);
}
f.print(f.jsDoc(field, " "));
const { typing } = getFieldTypeInfo(field);
const { typing } = fieldTypeScriptType(field);
f.print(` value: `, typing, `;`);
f.print(` case: "`, localName(field), `";`);
}
f.print(` } | { case: undefined; value?: undefined };`);
break;
default: {
f.print(f.jsDoc(member, " "));
const {typing, optional} = getFieldTypeInfo(member);
if (fieldUsesPrototype(member) || !optional) {
f.print(" ", localName(member), ": ", typing, ";");
} else {
const { typing, optional } = fieldTypeScriptType(member);
if (optional) {
f.print(" ", localName(member), "?: ", typing, ";");
} else {
f.print(" ", localName(member), ": ", typing, ";");
}
break;
}
Expand Down
39 changes: 5 additions & 34 deletions packages/protoc-gen-es/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,44 +18,19 @@ import {
ScalarType,
scalarTypeScriptType,
} from "@bufbuild/protobuf/reflect";
import { Edition, isWrapperDesc } from "@bufbuild/protobuf/wkt";
import { isWrapperDesc } from "@bufbuild/protobuf/wkt";
import type { Printable } from "@bufbuild/protoplugin/ecmascript";

/**
* Tells whether a field uses the prototype chain for field presence.
* Behavior must match with the counterpart in @bufbuild/protobuf.
*/
export function fieldUsesPrototype(field: DescField): field is DescField & {
fieldKind: "scalar" | "enum";
oneof: undefined;
repeated: false;
} {
if (field.parent.file.edition != Edition.EDITION_PROTO2) {
return false;
}
if (field.fieldKind != "scalar" && field.fieldKind != "enum") {
return false;
}
if (field.oneof) {
return false;
}
// proto2 singular scalar and enum fields use an initial value on the prototype chain
return true;
}

export function getFieldTypeInfo(field: DescField | DescExtension): {
export function fieldTypeScriptType(field: DescField | DescExtension): {
typing: Printable;
optional: boolean;
typingInferrableFromZeroValue: boolean;
} {
const typing: Printable = [];
let typingInferrableFromZeroValue: boolean;
let optional = false;
switch (field.fieldKind) {
case "scalar":
typing.push(scalarTypeScriptType(field.scalar, field.longType));
optional = field.optional;
typingInferrableFromZeroValue = true;
optional = field.proto.proto3Optional;
break;
case "message": {
if (!field.oneof && isWrapperDesc(field.message)) {
Expand All @@ -68,20 +43,17 @@ export function getFieldTypeInfo(field: DescField | DescExtension): {
});
}
optional = true;
typingInferrableFromZeroValue = true;
break;
}
case "enum":
typing.push({
kind: "es_shape_ref",
desc: field.enum,
});
optional = field.optional;
typingInferrableFromZeroValue = true;
optional = field.proto.proto3Optional;
break;
case "list":
optional = false;
typingInferrableFromZeroValue = false;
switch (field.listKind) {
case "enum":
typing.push(
Expand Down Expand Up @@ -140,12 +112,11 @@ export function getFieldTypeInfo(field: DescField | DescExtension): {
break;
}
typing.push("{ [key: ", keyType, "]: ", valueType, " }");
typingInferrableFromZeroValue = false;
optional = false;
break;
}
}
return { typing, optional, typingInferrableFromZeroValue };
return { typing, optional };
}

export function functionCall(
Expand Down
37 changes: 35 additions & 2 deletions packages/protoplugin-test/src/source-code-info.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ describe("getComments()", () => {
});

describe("getDeclarationString()", () => {
test("for field built-in option", async () => {
test("for field with json_name option", async () => {
const field = await compileField(`
syntax="proto3";
message M {
Expand All @@ -138,7 +138,18 @@ describe("getDeclarationString()", () => {
'string scalar_field = 1 [json_name = "scalarFieldJsonName"]',
);
});
test("for field with labels", async () => {
test("for field with jstype option", async () => {
const field = await compileField(`
syntax="proto3";
message M {
string scalar_field = 1 [jstype = JS_NORMAL];
}
`);
expect(getDeclarationString(field)).toBe(
"string scalar_field = 1 [jstype = JS_NORMAL]",
);
});
test("for repeated field", async () => {
const field = await compileField(`
syntax="proto3";
message M {
Expand All @@ -149,6 +160,28 @@ describe("getDeclarationString()", () => {
"repeated double double_field = 1",
);
});
test("for proto2 optional field", async () => {
const field = await compileField(`
syntax="proto3";
message M {
optional double double_field = 1;
}
`);
expect(getDeclarationString(field)).toBe(
"optional double double_field = 1",
);
});
test("for proto2 required field", async () => {
const field = await compileField(`
syntax="proto2";
message M {
required double double_field = 1;
}
`);
expect(getDeclarationString(field)).toBe(
"required double double_field = 1",
);
});
test("for map field", async () => {
const field = await compileField(`
syntax="proto3";
Expand Down
Loading