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 | diff --git a/packages/protobuf-test/src/name.test.ts b/packages/protobuf-test/src/name.test.ts new file mode 100644 index 000000000..e388bd611 --- /dev/null +++ b/packages/protobuf-test/src/name.test.ts @@ -0,0 +1,62 @@ +// 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"; + +describe("safeIdentifier", () => { + const { safeIdentifier } = codegenInfo; + + test("sanitized reserved identifiers", () => { + 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$"); + }); + + test("does not modify other inputs which are not reserved identifiers", () => { + 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; + + test("sanitizes reserved object property names", () => { + expect(safeObjectProperty("constructor")).toBe("constructor$"); + expect(safeObjectProperty("toString")).toBe("toString$"); + expect(safeObjectProperty("toJSON")).toBe("toJSON$"); + expect(safeObjectProperty("valueOf")).toBe("valueOf$"); + }); + + test("does not modify other inputs which are not reserved object properties", () => { + 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/codegen-info.ts b/packages/protobuf/src/codegen-info.ts index 5ffa890bb..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 } 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 +29,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 +67,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..eee341c62 100644 --- a/packages/protobuf/src/private/names.ts +++ b/packages/protobuf/src/private/names.ts @@ -50,10 +50,12 @@ 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; + // 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": { const sharedPrefix = desc.parent.sharedPrefix; @@ -61,10 +63,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 +71,7 @@ export function localName( return name; } name = name[0].toLowerCase() + name.substring(1); - if (reservedObjectProperties[name]) { - return name + "$"; - } - return name; + return safeObjectProperty(name); } } } @@ -84,15 +80,12 @@ 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 (reservedObjectProperties[name] || reservedMessageProperties[name]) { - name = name + "$"; - } - return name; + return safeObjectProperty(safeMessageProperty(name)); } /** @@ -180,98 +173,138 @@ 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: { [k: string]: boolean } = { +/** + * 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, -}; + "globalThis", + "Uint8Array", + "Partial", +]); -// Names that cannot be used for object properties because they are reserved -// by built-in JavaScript properties. -const reservedObjectProperties: { [k: string]: boolean } = { +/** + * 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, -}; + "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", +]); + +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; };