From d5d0cea0c6d4d469ee967a95bfe97ce252ac9af8 Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Wed, 15 Feb 2023 11:38:05 -0500 Subject: [PATCH 1/9] adds `safeIdentifier` and `safeObjectProperty` helpers --- packages/protobuf-test/src/name.test.ts | 52 ++++++++++++++++++++ packages/protobuf/src/codegen-info.ts | 6 ++- packages/protobuf/src/private/names.ts | 65 ++++++++++++++++++------- 3 files changed, 105 insertions(+), 18 deletions(-) create mode 100644 packages/protobuf-test/src/name.test.ts diff --git a/packages/protobuf-test/src/name.test.ts b/packages/protobuf-test/src/name.test.ts new file mode 100644 index 000000000..47bae75f0 --- /dev/null +++ b/packages/protobuf-test/src/name.test.ts @@ -0,0 +1,52 @@ +import { codegenInfo } from "@bufbuild/protobuf"; +import { describe, expect, test } from "@jest/globals"; + +describe("safeIdentifier", () => { + const { safeIdentifier } = codegenInfo; + + // prettier-ignore + test("sanitized reserved identifiers", () => { + expect<"break$">(safeIdentifier("break")).toBe("break$"); + expect<"case$">(safeIdentifier("case")).toBe("case$"); + expect<"catch$">(safeIdentifier("catch")).toBe("catch$"); + expect<"class$">(safeIdentifier("class")).toBe("class$"); + expect<"const$">(safeIdentifier("const")).toBe("const$"); + expect<"continue$">(safeIdentifier("continue")).toBe("continue$"); + expect<"debugger$">(safeIdentifier("debugger")).toBe("debugger$"); + expect<"default$">(safeIdentifier("default")).toBe("default$"); + expect<"delete$">(safeIdentifier("delete")).toBe("delete$"); + }); + + // prettier-ignore + test("does not modify other inputs which are not reserved identifiers", () => { + expect<"constructor">(safeIdentifier("constructor")).toBe("constructor"); + expect<"toString">(safeIdentifier("toString")).toBe("toString"); + expect<"toJSON">(safeIdentifier("toJSON")).toBe("toJSON"); + expect<"valueOf">(safeIdentifier("valueOf")).toBe("valueOf"); + }); +}); + +describe("safeObjectProperty", () => { + const { safeObjectProperty } = codegenInfo; + + // prettier-ignore + test("sanitizes reserved object property names", () => { + expect<"constructor$">(safeObjectProperty("constructor")).toBe("constructor$"); + expect<"toString$">(safeObjectProperty("toString")).toBe("toString$"); + expect<"toJSON$">(safeObjectProperty("toJSON")).toBe("toJSON$"); + expect<"valueOf$">(safeObjectProperty("valueOf")).toBe("valueOf$"); + }); + + // prettier-ignore + test("does not modify other inputs which are not reserved object properties", () => { + expect<"break">(safeObjectProperty("break")).toBe("break"); + expect<"case">(safeObjectProperty("case")).toBe("case"); + expect<"catch">(safeObjectProperty("catch")).toBe("catch"); + expect<"class">(safeObjectProperty("class")).toBe("class"); + expect<"const">(safeObjectProperty("const")).toBe("const"); + expect<"continue">(safeObjectProperty("continue")).toBe("continue"); + expect<"debugger">(safeObjectProperty("debugger")).toBe("debugger"); + expect<"default">(safeObjectProperty("default")).toBe("default"); + expect<"delete">(safeObjectProperty("delete")).toBe("delete"); + }); +}); diff --git a/packages/protobuf/src/codegen-info.ts b/packages/protobuf/src/codegen-info.ts index 5ffa890bb..e57ec5294 100644 --- a/packages/protobuf/src/codegen-info.ts +++ b/packages/protobuf/src/codegen-info.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { localName } from "./private/names.js"; +import { localName, safeIdentifier, safeObjectProperty } from "./private/names.js"; import { getUnwrappedFieldType } from "./private/field-wrapper.js"; import { scalarDefaultValue } from "./private/scalars.js"; import { reifyWkt } from "./private/reify-wkt.js"; @@ -25,6 +25,8 @@ interface CodegenInfo { readonly wktSourceFiles: readonly string[]; readonly scalarDefaultValue: typeof scalarDefaultValue; readonly reifyWkt: typeof reifyWkt; + readonly safeIdentifier: typeof safeIdentifier; + readonly safeObjectProperty: typeof safeObjectProperty; } type RuntimeSymbolName = @@ -61,6 +63,8 @@ export const codegenInfo: CodegenInfo = { reifyWkt, getUnwrappedFieldType, scalarDefaultValue, + safeIdentifier, + safeObjectProperty, // prettier-ignore symbols: { proto2: {typeOnly: false, privateImportPath: "./proto2.js", publicImportPath: packageName}, diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index 591e7f7b6..904cfbbce 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -50,10 +50,7 @@ export function localName( const pkg = desc.file.proto.package; const offset = pkg === undefined ? 0 : pkg.length + 1; const name = desc.typeName.substring(offset).replace(/\./g, "_"); - if (reservedIdent[name]) { - return name + "$"; - } - return name; + return safeIdentifier(name); } case "enum_value": { const sharedPrefix = desc.parent.sharedPrefix; @@ -61,10 +58,7 @@ export function localName( return desc.name; } const name = desc.name.substring(sharedPrefix.length); - if (reservedObjectProperties[name]) { - return name + "$"; - } - return name; + return safeObjectProperty(name); } case "rpc": { let name = desc.name; @@ -72,10 +66,7 @@ export function localName( return name; } name = name[0].toLowerCase() + name.substring(1); - if (reservedObjectProperties[name]) { - return name + "$"; - } - return name; + return safeObjectProperty(name); } } } @@ -89,7 +80,7 @@ export function localFieldName(protoName: string, inOneof: boolean) { // oneof member names are not properties, but values of the `case` property. return name; } - if (reservedObjectProperties[name] || reservedMessageProperties[name]) { + if (isReservedObjectProperty(name) || reservedMessageProperties[name]) { name = name + "$"; } return name; @@ -182,7 +173,7 @@ function protoCamelCase(snakeCase: string): string { // Names that cannot be used for identifiers, such as class names, // but _can_ be used for object properties. -const reservedIdent: { [k: string]: boolean } = { +const reservedIdent = { // ECMAScript 2015 keywords break: true, case: true, @@ -246,17 +237,17 @@ const reservedIdent: { [k: string]: boolean } = { globalThis: true, Uint8Array: true, Partial: true, -}; +} satisfies { [k: string]: boolean }; // Names that cannot be used for object properties because they are reserved // by built-in JavaScript properties. -const reservedObjectProperties: { [k: string]: boolean } = { +const reservedObjectProperties = { // names reserved by JavaScript constructor: true, toString: true, toJSON: true, valueOf: true, -}; +} satisfies { [k: string]: boolean }; // Names that cannot be used for object properties because they are reserved // by the runtime. @@ -275,3 +266,43 @@ const reservedMessageProperties: { [k: string]: boolean } = { // names reserved by the runtime for the future toObject: true, }; + +type Fallback = `${T}$`; + +const fallback = (word: T): Fallback => `${word}$`; + +type ReservedObjectProperty = keyof typeof reservedObjectProperties; + +const isReservedObjectProperty = ( + word: T | ReservedObjectProperty +): word is ReservedObjectProperty => + reservedObjectProperties[word as ReservedObjectProperty]; + +export const safeObjectProperty = ( + word: T +): T extends ReservedObjectProperty ? Fallback : T => { + if (isReservedObjectProperty(word)) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing + return fallback(word) as any; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing + return word as any; +}; + +type ReservedIdentifier = keyof typeof reservedIdent; + +const isReservedIdentifier = ( + word: T | ReservedIdentifier +): word is ReservedIdentifier => + reservedIdent[word as ReservedIdentifier]; + +export const safeIdentifier = ( + word: T +): T extends ReservedIdentifier ? Fallback : T => { + if (isReservedIdentifier(word)) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing + return fallback(word) as any; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing + return word as any; +}; From e4f43261f355c39a539528e0fa16409b80e8c044 Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Wed, 15 Feb 2023 12:14:47 -0500 Subject: [PATCH 2/9] fix for prototype inheritance bug --- packages/protobuf-test/src/name.test.ts | 56 +++--- packages/protobuf/src/private/names.ts | 221 ++++++++++++------------ 2 files changed, 134 insertions(+), 143 deletions(-) diff --git a/packages/protobuf-test/src/name.test.ts b/packages/protobuf-test/src/name.test.ts index 47bae75f0..2bc75c429 100644 --- a/packages/protobuf-test/src/name.test.ts +++ b/packages/protobuf-test/src/name.test.ts @@ -4,49 +4,45 @@ import { describe, expect, test } from "@jest/globals"; describe("safeIdentifier", () => { const { safeIdentifier } = codegenInfo; - // prettier-ignore test("sanitized reserved identifiers", () => { - expect<"break$">(safeIdentifier("break")).toBe("break$"); - expect<"case$">(safeIdentifier("case")).toBe("case$"); - expect<"catch$">(safeIdentifier("catch")).toBe("catch$"); - expect<"class$">(safeIdentifier("class")).toBe("class$"); - expect<"const$">(safeIdentifier("const")).toBe("const$"); - expect<"continue$">(safeIdentifier("continue")).toBe("continue$"); - expect<"debugger$">(safeIdentifier("debugger")).toBe("debugger$"); - expect<"default$">(safeIdentifier("default")).toBe("default$"); - expect<"delete$">(safeIdentifier("delete")).toBe("delete$"); + expect(safeIdentifier("break")).toBe("break$"); + expect(safeIdentifier("case")).toBe("case$"); + expect(safeIdentifier("catch")).toBe("catch$"); + expect(safeIdentifier("class")).toBe("class$"); + expect(safeIdentifier("const")).toBe("const$"); + expect(safeIdentifier("continue")).toBe("continue$"); + expect(safeIdentifier("debugger")).toBe("debugger$"); + expect(safeIdentifier("default")).toBe("default$"); + expect(safeIdentifier("delete")).toBe("delete$"); }); - // prettier-ignore test("does not modify other inputs which are not reserved identifiers", () => { - expect<"constructor">(safeIdentifier("constructor")).toBe("constructor"); - expect<"toString">(safeIdentifier("toString")).toBe("toString"); - expect<"toJSON">(safeIdentifier("toJSON")).toBe("toJSON"); - expect<"valueOf">(safeIdentifier("valueOf")).toBe("valueOf"); + expect(safeIdentifier("constructor")).toBe("constructor"); + expect(safeIdentifier("toString")).toBe("toString"); + expect(safeIdentifier("toJSON")).toBe("toJSON"); + expect(safeIdentifier("valueOf")).toBe("valueOf"); }); }); describe("safeObjectProperty", () => { const { safeObjectProperty } = codegenInfo; - // prettier-ignore test("sanitizes reserved object property names", () => { - expect<"constructor$">(safeObjectProperty("constructor")).toBe("constructor$"); - expect<"toString$">(safeObjectProperty("toString")).toBe("toString$"); - expect<"toJSON$">(safeObjectProperty("toJSON")).toBe("toJSON$"); - expect<"valueOf$">(safeObjectProperty("valueOf")).toBe("valueOf$"); + expect(safeObjectProperty("constructor")).toBe("constructor$"); + expect(safeObjectProperty("toString")).toBe("toString$"); + expect(safeObjectProperty("toJSON")).toBe("toJSON$"); + expect(safeObjectProperty("valueOf")).toBe("valueOf$"); }); - // prettier-ignore test("does not modify other inputs which are not reserved object properties", () => { - expect<"break">(safeObjectProperty("break")).toBe("break"); - expect<"case">(safeObjectProperty("case")).toBe("case"); - expect<"catch">(safeObjectProperty("catch")).toBe("catch"); - expect<"class">(safeObjectProperty("class")).toBe("class"); - expect<"const">(safeObjectProperty("const")).toBe("const"); - expect<"continue">(safeObjectProperty("continue")).toBe("continue"); - expect<"debugger">(safeObjectProperty("debugger")).toBe("debugger"); - expect<"default">(safeObjectProperty("default")).toBe("default"); - expect<"delete">(safeObjectProperty("delete")).toBe("delete"); + expect(safeObjectProperty("break")).toBe("break"); + expect(safeObjectProperty("case")).toBe("case"); + expect(safeObjectProperty("catch")).toBe("catch"); + expect(safeObjectProperty("class")).toBe("class"); + expect(safeObjectProperty("const")).toBe("const"); + expect(safeObjectProperty("continue")).toBe("continue"); + expect(safeObjectProperty("debugger")).toBe("debugger"); + expect(safeObjectProperty("default")).toBe("default"); + expect(safeObjectProperty("delete")).toBe("delete"); }); }); diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index 904cfbbce..a64b79719 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -75,13 +75,16 @@ export function localName( * Returns the name of a field in generated code. */ export function localFieldName(protoName: string, inOneof: boolean) { - let name = protoCamelCase(protoName); + const name = protoCamelCase(protoName); if (inOneof) { // oneof member names are not properties, but values of the `case` property. return name; } - if (isReservedObjectProperty(name) || reservedMessageProperties[name]) { - name = name + "$"; + if ( + reservedObjectProperties.has(name) || + reservedMessageProperties.has(name) + ) { + fallback(name); } return name; } @@ -171,138 +174,130 @@ function protoCamelCase(snakeCase: string): string { return b.join(""); } -// Names that cannot be used for identifiers, such as class names, -// but _can_ be used for object properties. -const reservedIdent = { +/** + * Names that cannot be used for identifiers, such as class names, + * but _can_ be used for object properties. + */ +const reservedIdentifiers = new Set([ // ECMAScript 2015 keywords - break: true, - case: true, - catch: true, - class: true, - const: true, - continue: true, - debugger: true, - default: true, - delete: true, - do: true, - else: true, - export: true, - extends: true, - false: true, - finally: true, - for: true, - function: true, - if: true, - import: true, - in: true, - instanceof: true, - new: true, - null: true, - return: true, - super: true, - switch: true, - this: true, - throw: true, - true: true, - try: true, - typeof: true, - var: true, - void: true, - while: true, - with: true, - yield: true, + "break", + "case", + "catch", + "class", + "const", + "continue", + "debugger", + "default", + "delete", + "do", + "else", + "export", + "extends", + "false", + "finally", + "for", + "function", + "if", + "import", + "in", + "instanceof", + "new", + "null", + "return", + "super", + "switch", + "this", + "throw", + "true", + "try", + "typeof", + "var", + "void", + "while", + "with", + "yield", // ECMAScript 2015 future reserved keywords - enum: true, - implements: true, - interface: true, - let: true, - package: true, - private: true, - protected: true, - public: true, - static: true, + "enum", + "implements", + "interface", + "let", + "package", + "private", + "protected", + "public", + "static", // Class name cannot be 'Object' when targeting ES5 with module CommonJS - Object: true, + "Object", // TypeScript keywords that cannot be used for types (as opposed to variables) - bigint: true, - number: true, - boolean: true, - string: true, - object: true, + "bigint", + "number", + "boolean", + "string", + "object", // Identifiers reserved for the runtime, so we can generate legible code - globalThis: true, - Uint8Array: true, - Partial: true, -} satisfies { [k: string]: boolean }; + "globalThis", + "Uint8Array", + "Partial", +]); -// Names that cannot be used for object properties because they are reserved -// by built-in JavaScript properties. -const reservedObjectProperties = { +/** + * Names that cannot be used for object properties because they are reserved + * by built-in JavaScript properties. + */ +const reservedObjectProperties = new Set([ // names reserved by JavaScript - constructor: true, - toString: true, - toJSON: true, - valueOf: true, -} satisfies { [k: string]: boolean }; + "constructor", + "toString", + "toJSON", + "valueOf", +]); -// Names that cannot be used for object properties because they are reserved -// by the runtime. -const reservedMessageProperties: { [k: string]: boolean } = { +/** + * Names that cannot be used for object properties because they are reserved + * by the runtime. + */ +const reservedMessageProperties = new Set([ // names reserved by the runtime - getType: true, - clone: true, - equals: true, - fromBinary: true, - fromJson: true, - fromJsonString: true, - toBinary: true, - toJson: true, - toJsonString: true, + "getType", + "clone", + "equals", + "fromBinary", + "fromJson", + "fromJsonString", + "toBinary", + "toJson", + "toJsonString", // names reserved by the runtime for the future - toObject: true, -}; + "toObject", +]); type Fallback = `${T}$`; const fallback = (word: T): Fallback => `${word}$`; -type ReservedObjectProperty = keyof typeof reservedObjectProperties; - -const isReservedObjectProperty = ( - word: T | ReservedObjectProperty -): word is ReservedObjectProperty => - reservedObjectProperties[word as ReservedObjectProperty]; - -export const safeObjectProperty = ( - word: T -): T extends ReservedObjectProperty ? Fallback : T => { - if (isReservedObjectProperty(word)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing - return fallback(word) as any; +/** + * Names that cannot be used for object properties because they are reserved + * by built-in JavaScript properties. + */ +export const safeObjectProperty = (word: string): string => { + if (reservedObjectProperties.has(word)) { + return fallback(word); } - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing - return word as any; + return word; }; -type ReservedIdentifier = keyof typeof reservedIdent; - -const isReservedIdentifier = ( - word: T | ReservedIdentifier -): word is ReservedIdentifier => - reservedIdent[word as ReservedIdentifier]; - -export const safeIdentifier = ( - word: T -): T extends ReservedIdentifier ? Fallback : T => { - if (isReservedIdentifier(word)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing - return fallback(word) as any; +/** + * Names that cannot be used for identifiers, such as class names, + * but _can_ be used for object properties. + */ +export const safeIdentifier = (word: string): string => { + if (reservedIdentifiers.has(word)) { + return fallback(word); } - // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-explicit-any -- required for narrowing - return word as any; + return word; }; From 0b25ca689c4f55e4b1dc41fd2926e2485eb1eecd Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Wed, 15 Feb 2023 12:58:23 -0500 Subject: [PATCH 3/9] attempt at a fix to keep old behavior --- packages/protobuf/src/private/names.ts | 46 +++++++++++++++----------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index a64b79719..424479558 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -80,13 +80,7 @@ export function localFieldName(protoName: string, inOneof: boolean) { // oneof member names are not properties, but values of the `case` property. return name; } - if ( - reservedObjectProperties.has(name) || - reservedMessageProperties.has(name) - ) { - fallback(name); - } - return name; + return safeMessageProperty(name); } /** @@ -276,28 +270,42 @@ const reservedMessageProperties = new Set([ "toObject", ]); -type Fallback = `${T}$`; +const fallback = (name: T) => `${name}$` as const; -const fallback = (word: T): Fallback => `${word}$`; +/** + * Will wrap names that are Object prototype properties or names reserved + * for `Message`s. + */ +const safeMessageProperty = (name: string): string => { + if ( + reservedObjectProperties.has(name) || + reservedMessageProperties.has(name) + ) { + fallback(name); + } + return name; +} /** * Names that cannot be used for object properties because they are reserved * by built-in JavaScript properties. */ -export const safeObjectProperty = (word: string): string => { - if (reservedObjectProperties.has(word)) { - return fallback(word); +export const safeObjectProperty = (name: string): string => { + if (reservedObjectProperties.has(name)) { + return fallback(name); } - return word; + return name; }; /** - * Names that cannot be used for identifiers, such as class names, - * but _can_ be used for object properties. + * Names that can be used for identifiers or class properties */ -export const safeIdentifier = (word: string): string => { - if (reservedIdentifiers.has(word)) { - return fallback(word); +export const safeIdentifier = (name: string): string => { + if ( + reservedObjectProperties.has(name) || + reservedIdentifiers.has(name) + ) { + return fallback(name); } - return word; + return name; }; From 476fa15d9fab50f29e9418196f60326abfadd94a Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Thu, 16 Feb 2023 13:17:21 -0500 Subject: [PATCH 4/9] uses composition --- packages/protobuf/src/private/names.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index 424479558..dd1197fc8 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -50,7 +50,7 @@ export function localName( const pkg = desc.file.proto.package; const offset = pkg === undefined ? 0 : pkg.length + 1; const name = desc.typeName.substring(offset).replace(/\./g, "_"); - return safeIdentifier(name); + return safeObjectProperty(safeIdentifier(name)); } case "enum_value": { const sharedPrefix = desc.parent.sharedPrefix; @@ -80,7 +80,7 @@ export function localFieldName(protoName: string, inOneof: boolean) { // oneof member names are not properties, but values of the `case` property. return name; } - return safeMessageProperty(name); + return safeObjectProperty(safeMessageProperty(name)); } /** @@ -277,11 +277,8 @@ const fallback = (name: T) => `${name}$` as const; * for `Message`s. */ const safeMessageProperty = (name: string): string => { - if ( - reservedObjectProperties.has(name) || - reservedMessageProperties.has(name) - ) { - fallback(name); + if (reservedMessageProperties.has(name)) { + return fallback(name); } return name; } @@ -301,10 +298,7 @@ export const safeObjectProperty = (name: string): string => { * Names that can be used for identifiers or class properties */ export const safeIdentifier = (name: string): string => { - if ( - reservedObjectProperties.has(name) || - reservedIdentifiers.has(name) - ) { + if (reservedIdentifiers.has(name)) { return fallback(name); } return name; From 1aec057227151c94a5dee5510e6765fc33a59a70 Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Thu, 16 Feb 2023 14:06:31 -0500 Subject: [PATCH 5/9] `make & make test` --- packages/protobuf-bench/README.md | 2 +- packages/protobuf-test/src/name.test.ts | 14 ++++++++++++++ packages/protobuf/src/codegen-info.ts | 6 +++++- packages/protobuf/src/private/names.ts | 2 +- 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md index a351d5238..b2f51610d 100644 --- a/packages/protobuf-bench/README.md +++ b/packages/protobuf-bench/README.md @@ -10,5 +10,5 @@ server would usually do. | code generator | bundle size | minified | compressed | |---------------------|------------------------:|-----------------------:|-------------------:| -| protobuf-es | 86,785 b | 36,950 b | 9,642 b | +| protobuf-es | 87,019 b | 36,999 b | 9,692 b | | protobuf-javascript | 375,550 b | 271,672 b | 43,769 b | diff --git a/packages/protobuf-test/src/name.test.ts b/packages/protobuf-test/src/name.test.ts index 2bc75c429..e388bd611 100644 --- a/packages/protobuf-test/src/name.test.ts +++ b/packages/protobuf-test/src/name.test.ts @@ -1,3 +1,17 @@ +// Copyright 2021-2023 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + import { codegenInfo } from "@bufbuild/protobuf"; import { describe, expect, test } from "@jest/globals"; diff --git a/packages/protobuf/src/codegen-info.ts b/packages/protobuf/src/codegen-info.ts index e57ec5294..e13f7a832 100644 --- a/packages/protobuf/src/codegen-info.ts +++ b/packages/protobuf/src/codegen-info.ts @@ -12,7 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { localName, safeIdentifier, safeObjectProperty } from "./private/names.js"; +import { + localName, + safeIdentifier, + safeObjectProperty, +} from "./private/names.js"; import { getUnwrappedFieldType } from "./private/field-wrapper.js"; import { scalarDefaultValue } from "./private/scalars.js"; import { reifyWkt } from "./private/reify-wkt.js"; diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index dd1197fc8..c156d82c0 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -281,7 +281,7 @@ const safeMessageProperty = (name: string): string => { return fallback(name); } return name; -} +}; /** * Names that cannot be used for object properties because they are reserved From 0573d4b6c9023caf4fc8aa1762f7a85b2a0ea2da Mon Sep 17 00:00:00 2001 From: Timo Stamm Date: Wed, 15 Mar 2023 10:36:05 -0400 Subject: [PATCH 6/9] make bench --- packages/protobuf-bench/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md index 34e51056d..cca6d38bb 100644 --- a/packages/protobuf-bench/README.md +++ b/packages/protobuf-bench/README.md @@ -10,5 +10,5 @@ server would usually do. | code generator | bundle size | minified | compressed | |---------------------|------------------------:|-----------------------:|-------------------:| -| protobuf-es | 86,799 b | 36,976 b | 11,083 b | +| protobuf-es | 87,033 b | 37,025 b | 11,104 b | | protobuf-javascript | 394,384 b | 288,775 b | 54,825 b | From be0978aec27097e89fd60a3ecebe5be59d2648ee Mon Sep 17 00:00:00 2001 From: Timo Stamm Date: Wed, 15 Mar 2023 12:12:21 -0400 Subject: [PATCH 7/9] Add comment --- packages/protobuf/src/private/names.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index c156d82c0..eee341c62 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -50,6 +50,11 @@ export function localName( const pkg = desc.file.proto.package; const offset = pkg === undefined ? 0 : pkg.length + 1; const name = desc.typeName.substring(offset).replace(/\./g, "_"); + // For services, we only care about safe identifiers, not safe object properties, + // but we have shipped v1 with a bug that respected object properties, and we + // do not want to introduce a breaking change, so we continue to escape for + // safe object properties. + // See https://github.com/bufbuild/protobuf-es/pull/391 return safeObjectProperty(safeIdentifier(name)); } case "enum_value": { From 7c274cf8ec879ad73c3da34756f3a9c300d0eac0 Mon Sep 17 00:00:00 2001 From: Timo Stamm Date: Wed, 15 Mar 2023 12:18:26 -0400 Subject: [PATCH 8/9] Update to keep bundle size in check. --- packages/protobuf-bench/README.md | 2 +- packages/protobuf/src/private/names.ts | 214 ++++++++++++------------- 2 files changed, 100 insertions(+), 116 deletions(-) diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md index cca6d38bb..30301fd44 100644 --- a/packages/protobuf-bench/README.md +++ b/packages/protobuf-bench/README.md @@ -10,5 +10,5 @@ server would usually do. | code generator | bundle size | minified | compressed | |---------------------|------------------------:|-----------------------:|-------------------:| -| protobuf-es | 87,033 b | 37,025 b | 11,104 b | +| protobuf-es | 86,871 b | 37,036 b | 11,102 b | | protobuf-javascript | 394,384 b | 288,775 b | 54,825 b | diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index eee341c62..f9976d132 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -81,11 +81,8 @@ export function localName( */ export function localFieldName(protoName: string, inOneof: boolean) { const name = protoCamelCase(protoName); - if (inOneof) { - // oneof member names are not properties, but values of the `case` property. - return name; - } - return safeObjectProperty(safeMessageProperty(name)); + // oneof member names are not properties, but values of the `case` property. + return inOneof ? name : safeName(name, reservedMessageProperties); } /** @@ -125,6 +122,19 @@ export function findEnumSharedPrefix( return prefix; } +/** + * Names that cannot be used for object properties because they are reserved + * by built-in JavaScript properties. + */ +export const safeObjectProperty = (name: string): string => + safeName(name, reservedObjectProperties); + +/** + * Names that can be used for identifiers or class properties + */ +export const safeIdentifier = (name: string): string => + safeName(name, reservedIdentifiers); + /** * Converts lowerCamelCase or UpperCamelCase into lower_snake_case. * This is used to find shared prefixes in an enum. @@ -173,138 +183,112 @@ function protoCamelCase(snakeCase: string): string { return b.join(""); } +/** + * Escapes a given name if it is contained in the record. + */ +function safeName(name: string, properties: Record): string { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-boolean-literal-compare,@typescript-eslint/no-unnecessary-condition -- the properties object inherits from Object, so we use strict comparison to true + return properties[name] === true ? name + "$" : name; +} + /** * Names that cannot be used for identifiers, such as class names, * but _can_ be used for object properties. */ -const reservedIdentifiers = new Set([ +const reservedIdentifiers = { // ECMAScript 2015 keywords - "break", - "case", - "catch", - "class", - "const", - "continue", - "debugger", - "default", - "delete", - "do", - "else", - "export", - "extends", - "false", - "finally", - "for", - "function", - "if", - "import", - "in", - "instanceof", - "new", - "null", - "return", - "super", - "switch", - "this", - "throw", - "true", - "try", - "typeof", - "var", - "void", - "while", - "with", - "yield", + break: true, + case: true, + catch: true, + class: true, + const: true, + continue: true, + debugger: true, + default: true, + delete: true, + do: true, + else: true, + export: true, + extends: true, + false: true, + finally: true, + for: true, + function: true, + if: true, + import: true, + in: true, + instanceof: true, + new: true, + null: true, + return: true, + super: true, + switch: true, + this: true, + throw: true, + true: true, + try: true, + typeof: true, + var: true, + void: true, + while: true, + with: true, + yield: true, // ECMAScript 2015 future reserved keywords - "enum", - "implements", - "interface", - "let", - "package", - "private", - "protected", - "public", - "static", + enum: true, + implements: true, + interface: true, + let: true, + package: true, + private: true, + protected: true, + public: true, + static: true, // Class name cannot be 'Object' when targeting ES5 with module CommonJS - "Object", + Object: true, // TypeScript keywords that cannot be used for types (as opposed to variables) - "bigint", - "number", - "boolean", - "string", - "object", + bigint: true, + number: true, + boolean: true, + string: true, + object: true, // Identifiers reserved for the runtime, so we can generate legible code - "globalThis", - "Uint8Array", - "Partial", -]); + globalThis: true, + Uint8Array: true, + Partial: true, +} as const; /** * Names that cannot be used for object properties because they are reserved * by built-in JavaScript properties. */ -const reservedObjectProperties = new Set([ +const reservedObjectProperties = { // names reserved by JavaScript - "constructor", - "toString", - "toJSON", - "valueOf", -]); + constructor: true, + toString: true, + toJSON: true, + valueOf: true, +} as const; /** * Names that cannot be used for object properties because they are reserved - * by the runtime. + * by built-in JavaScript properties, or by our runtime. */ -const reservedMessageProperties = new Set([ +const reservedMessageProperties = { + ...reservedObjectProperties, // names reserved by the runtime - "getType", - "clone", - "equals", - "fromBinary", - "fromJson", - "fromJsonString", - "toBinary", - "toJson", - "toJsonString", - + getType: true, + clone: true, + equals: true, + fromBinary: true, + fromJson: true, + fromJsonString: true, + toBinary: true, + toJson: true, + toJsonString: true, // names reserved by the runtime for the future - "toObject", -]); - -const fallback = (name: T) => `${name}$` as const; - -/** - * Will wrap names that are Object prototype properties or names reserved - * for `Message`s. - */ -const safeMessageProperty = (name: string): string => { - if (reservedMessageProperties.has(name)) { - return fallback(name); - } - return name; -}; - -/** - * Names that cannot be used for object properties because they are reserved - * by built-in JavaScript properties. - */ -export const safeObjectProperty = (name: string): string => { - if (reservedObjectProperties.has(name)) { - return fallback(name); - } - return name; -}; - -/** - * Names that can be used for identifiers or class properties - */ -export const safeIdentifier = (name: string): string => { - if (reservedIdentifiers.has(name)) { - return fallback(name); - } - return name; -}; + toObject: true, +} as const; From b58ffd7535e6b0caeaad44fb46d4c49e7e729484 Mon Sep 17 00:00:00 2001 From: Timo Stamm Date: Wed, 15 Mar 2023 12:20:52 -0400 Subject: [PATCH 9/9] Revert "Update to keep bundle size in check." This reverts commit 7c274cf8ec879ad73c3da34756f3a9c300d0eac0. --- packages/protobuf-bench/README.md | 2 +- packages/protobuf/src/private/names.ts | 214 +++++++++++++------------ 2 files changed, 116 insertions(+), 100 deletions(-) diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md index 30301fd44..cca6d38bb 100644 --- a/packages/protobuf-bench/README.md +++ b/packages/protobuf-bench/README.md @@ -10,5 +10,5 @@ server would usually do. | code generator | bundle size | minified | compressed | |---------------------|------------------------:|-----------------------:|-------------------:| -| protobuf-es | 86,871 b | 37,036 b | 11,102 b | +| protobuf-es | 87,033 b | 37,025 b | 11,104 b | | protobuf-javascript | 394,384 b | 288,775 b | 54,825 b | diff --git a/packages/protobuf/src/private/names.ts b/packages/protobuf/src/private/names.ts index f9976d132..eee341c62 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -81,8 +81,11 @@ export function localName( */ export function localFieldName(protoName: string, inOneof: boolean) { const name = protoCamelCase(protoName); - // oneof member names are not properties, but values of the `case` property. - return inOneof ? name : safeName(name, reservedMessageProperties); + if (inOneof) { + // oneof member names are not properties, but values of the `case` property. + return name; + } + return safeObjectProperty(safeMessageProperty(name)); } /** @@ -122,19 +125,6 @@ export function findEnumSharedPrefix( return prefix; } -/** - * Names that cannot be used for object properties because they are reserved - * by built-in JavaScript properties. - */ -export const safeObjectProperty = (name: string): string => - safeName(name, reservedObjectProperties); - -/** - * Names that can be used for identifiers or class properties - */ -export const safeIdentifier = (name: string): string => - safeName(name, reservedIdentifiers); - /** * Converts lowerCamelCase or UpperCamelCase into lower_snake_case. * This is used to find shared prefixes in an enum. @@ -183,112 +173,138 @@ function protoCamelCase(snakeCase: string): string { return b.join(""); } -/** - * Escapes a given name if it is contained in the record. - */ -function safeName(name: string, properties: Record): string { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-boolean-literal-compare,@typescript-eslint/no-unnecessary-condition -- the properties object inherits from Object, so we use strict comparison to true - return properties[name] === true ? name + "$" : name; -} - /** * Names that cannot be used for identifiers, such as class names, * but _can_ be used for object properties. */ -const reservedIdentifiers = { +const reservedIdentifiers = new Set([ // ECMAScript 2015 keywords - break: true, - case: true, - catch: true, - class: true, - const: true, - continue: true, - debugger: true, - default: true, - delete: true, - do: true, - else: true, - export: true, - extends: true, - false: true, - finally: true, - for: true, - function: true, - if: true, - import: true, - in: true, - instanceof: true, - new: true, - null: true, - return: true, - super: true, - switch: true, - this: true, - throw: true, - true: true, - try: true, - typeof: true, - var: true, - void: true, - while: true, - with: true, - yield: true, + "break", + "case", + "catch", + "class", + "const", + "continue", + "debugger", + "default", + "delete", + "do", + "else", + "export", + "extends", + "false", + "finally", + "for", + "function", + "if", + "import", + "in", + "instanceof", + "new", + "null", + "return", + "super", + "switch", + "this", + "throw", + "true", + "try", + "typeof", + "var", + "void", + "while", + "with", + "yield", // ECMAScript 2015 future reserved keywords - enum: true, - implements: true, - interface: true, - let: true, - package: true, - private: true, - protected: true, - public: true, - static: true, + "enum", + "implements", + "interface", + "let", + "package", + "private", + "protected", + "public", + "static", // Class name cannot be 'Object' when targeting ES5 with module CommonJS - Object: true, + "Object", // TypeScript keywords that cannot be used for types (as opposed to variables) - bigint: true, - number: true, - boolean: true, - string: true, - object: true, + "bigint", + "number", + "boolean", + "string", + "object", // Identifiers reserved for the runtime, so we can generate legible code - globalThis: true, - Uint8Array: true, - Partial: true, -} as const; + "globalThis", + "Uint8Array", + "Partial", +]); /** * Names that cannot be used for object properties because they are reserved * by built-in JavaScript properties. */ -const reservedObjectProperties = { +const reservedObjectProperties = new Set([ // names reserved by JavaScript - constructor: true, - toString: true, - toJSON: true, - valueOf: true, -} as const; + "constructor", + "toString", + "toJSON", + "valueOf", +]); /** * Names that cannot be used for object properties because they are reserved - * by built-in JavaScript properties, or by our runtime. + * by the runtime. */ -const reservedMessageProperties = { - ...reservedObjectProperties, +const reservedMessageProperties = new Set([ // names reserved by the runtime - getType: true, - clone: true, - equals: true, - fromBinary: true, - fromJson: true, - fromJsonString: true, - toBinary: true, - toJson: true, - toJsonString: true, + "getType", + "clone", + "equals", + "fromBinary", + "fromJson", + "fromJsonString", + "toBinary", + "toJson", + "toJsonString", + // names reserved by the runtime for the future - toObject: true, -} as const; + "toObject", +]); + +const fallback = (name: T) => `${name}$` as const; + +/** + * Will wrap names that are Object prototype properties or names reserved + * for `Message`s. + */ +const safeMessageProperty = (name: string): string => { + if (reservedMessageProperties.has(name)) { + return fallback(name); + } + return name; +}; + +/** + * Names that cannot be used for object properties because they are reserved + * by built-in JavaScript properties. + */ +export const safeObjectProperty = (name: string): string => { + if (reservedObjectProperties.has(name)) { + return fallback(name); + } + return name; +}; + +/** + * Names that can be used for identifiers or class properties + */ +export const safeIdentifier = (name: string): string => { + if (reservedIdentifiers.has(name)) { + return fallback(name); + } + return name; +};