Fixed: Unified safe property escapes and added a test for #834

cli/targets/static.js

@@ -2,7 +2,6 @@
 module.exports = static_target;
 
 var protobuf   = require("../.."),
-    cliUtil    = require("../util"),
     UglifyJS   = require("uglify-js"),
     espree     = require("espree"),
     escodegen  = require("escodegen"),
@@ -42,7 +41,7 @@ function static_target(root, options, callback) {
             }
             push("// Exported root namespace");
         }
-        var rootProp = cliUtil.safeProp(config.root || "default");
+        var rootProp = util.safeProp(config.root || "default");
         push((config.es6 ? "const" : "var") + " $root = $protobuf.roots" + rootProp + " || ($protobuf.roots" + rootProp + " = {});");
         buildNamespace(null, root);
         return callback(null, out.join("\n"));
@@ -98,7 +97,7 @@ function exportName(object, asInterface) {
 function escapeName(name) {
     if (!name)
         return "$root";
-    return cliUtil.reserved(name) ? name + "_" : name;
+    return util.isReserved(name) ? name + "_" : name;
 }
 
 function aOrAn(name) {
@@ -363,7 +362,7 @@ function buildType(ref, type) {
             "@interface " + escapeName("I" + type.name)
         ];
         type.fieldsArray.forEach(function(field) {
-            var prop = util.safeProp(field.name);
+            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)
@@ -398,7 +397,7 @@ function buildType(ref, type) {
                 jsType = jsType + "|null|undefined";
             pushComment([
                 field.comment || type.name + " " + field.name + ".",
-                "@member {" + jsType + "} " + escapeName(field.name),
+                "@member {" + jsType + "} " + field.name,
                 "@memberof " + exportName(type),
                 "@instance"
             ]);

cli/util.js

@@ -181,21 +181,6 @@ exports.pad = function(str, len, l) {
     return str;
 };
 
-exports.reserved = function reserved(name) {
-    return /^(?:do|if|in|for|let|new|try|var|case|else|enum|eval|false|null|this|true|void|with|break|catch|class|const|super|throw|while|yield|delete|export|import|public|return|static|switch|typeof|default|extends|finally|package|private|continue|debugger|function|arguments|interface|protected|implements|instanceof)$/.test(name);
-};
-
-// generate dot-notation property accessors where possible. this saves a few chars (i.e. m.hello
-// instead of m["hello"]) but has no measurable performance impact (on V8). not present within the
-// library itself because the reserved words check requires a rather longish regex.
-exports.safeProp = protobuf.util.safeProp = (function(safeProp) {
-    return function safeProp_dn(name) {
-        return !/^[$\w]+$/.test(name) || exports.reserved(name)
-            ? safeProp(name)
-            : "." + name;
-    };
-})(protobuf.util.safeProp);
-
 exports.jsonSafeProp = function(json) {
     return json.replace(/^( +)"(\w+)":/mg, function($0, $1, $2) {
         return exports.safeProp($2).charAt(0) === "."

ext/descriptor/index.d.ts

@@ -4,15 +4,9 @@ export const FileDescriptorSet: $protobuf.Type;
 
 export const FileDescriptorProto: $protobuf.Type;
 
-export const DescriptorProto: $protobuf.Type & {
-    ExtensionRange: $protobuf.Type,
-    ReservedRange: $protobuf.Type
-};
+export const DescriptorProto: $protobuf.Type & {    ExtensionRange: $protobuf.Type,    ReservedRange: $protobuf.Type};
 
-export const FieldDescriptorProto: $protobuf.Type & {
-    Label: $protobuf.Enum,
-    Type: $protobuf.Enum
-};
+export const FieldDescriptorProto: $protobuf.Type & {    Label: $protobuf.Enum,    Type: $protobuf.Enum};
 
 export const OneofDescriptorProto: $protobuf.Type;
 
@@ -24,16 +18,11 @@ export const EnumValueDescriptorProto: $protobuf.Type;
 
 export const MethodDescriptorProto: $protobuf.Type;
 
-export const FileOptions: $protobuf.Type & {
-    OptimizeMode: $protobuf.Enum
-};
+export const FileOptions: $protobuf.Type & {    OptimizeMode: $protobuf.Enum};
 
 export const MessageOptions: $protobuf.Type;
 
-export const FieldOptions: $protobuf.Type & {
-    CType: $protobuf.Enum,
-    JSType: $protobuf.Enum
-};
+export const FieldOptions: $protobuf.Type & {    CType: $protobuf.Enum,    JSType: $protobuf.Enum};
 
 export const OneofOptions: $protobuf.Type;
 
@@ -45,17 +34,11 @@ export const ServiceOptions: $protobuf.Type;
 
 export const MethodOptions: $protobuf.Type;
 
-export const UninterpretedOption: $protobuf.Type & {
-    NamePart: $protobuf.Type
-};
+export const UninterpretedOption: $protobuf.Type & {    NamePart: $protobuf.Type};
 
-export const SourceCodeInfo: $protobuf.Type & {
-    Location: $protobuf.Type
-};
+export const SourceCodeInfo: $protobuf.Type & {    Location: $protobuf.Type};
 
-export const GeneratedCodeInfo: $protobuf.Type & {
-    Annotation: $protobuf.Type
-};
+export const GeneratedCodeInfo: $protobuf.Type & {    Annotation: $protobuf.Type};
 
 export interface IFileDescriptorSet {
     file: IFileDescriptorProto[];

index.d.ts

@@ -2027,7 +2027,14 @@ export namespace util {
     function toObject(array: any[]): { [k: string]: any };
 
     /**
-     * Returns a safe property accessor for the specified properly name.
+     * Tests whether the specified name is a reserved word in JS.
+     * @param name Name to test
+     * @returns `true` if reserved, otherwise `false`
+     */
+    function isReserved(name: string): boolean;
+
+    /**
+     * Returns a safe property accessor for the specified property name.
      * @param prop Property name
      * @returns Safe accessor
      */

package.json

@@ -36,7 +36,7 @@
     "postinstall": "node scripts/postinstall",
     "prof": "node bench/prof",
     "test": "tape -r ./lib/tape-adapter tests/*.js tests/node/*.js | tap-spec",
-    "test-types": "tsc tests/comp_typescript.ts --lib es2015 --strictNullChecks --experimentalDecorators --emitDecoratorMetadata && tsc tests/data/test.ts --lib es2015 --noEmit --strictNullChecks && tsc tests/data/rpc.ts --lib es2015 --noEmit --strictNullChecks",
+    "test-types": "tsc tests/comp_typescript.ts --lib es2015 --strictNullChecks --experimentalDecorators --emitDecoratorMetadata && tsc tests/data/test.js.ts --lib es2015 --noEmit --strictNullChecks && tsc tests/data/rpc.ts --lib es2015 --noEmit --strictNullChecks",
     "types": "node bin/pbts --main --global protobuf --out index.d.ts src/ lib/aspromise/index.js lib/base64/index.js lib/codegen/index.js lib/eventemitter/index.js lib/float/index.js lib/fetch/index.js lib/inquire/index.js lib/path/index.js lib/pool/index.js lib/utf8/index.js && npm run test-types",
     "make": "npm run test && npm run types && npm run build && npm run lint",
     "release": "npm run make && npm run changelog"

src/util.js

@@ -59,12 +59,23 @@ var safePropBackslashRe = /\\/g,
     safePropQuoteRe     = /"/g;
 
 /**
- * Returns a safe property accessor for the specified properly name.
+ * Tests whether the specified name is a reserved word in JS.
+ * @param {string} name Name to test
+ * @returns {boolean} `true` if reserved, otherwise `false`
+ */
+util.isReserved = function isReserved(name) {
+    return /^(?:do|if|in|for|let|new|try|var|case|else|enum|eval|false|null|this|true|void|with|break|catch|class|const|super|throw|while|yield|delete|export|import|public|return|static|switch|typeof|default|extends|finally|package|private|continue|debugger|function|arguments|interface|protected|implements|instanceof)$/.test(name);
+};
+
+/**
+ * Returns a safe property accessor for the specified property name.
  * @param {string} prop Property name
  * @returns {string} Safe accessor
  */
 util.safeProp = function safeProp(prop) {
-    return "[\"" + prop.replace(safePropBackslashRe, "\\\\").replace(safePropQuoteRe, "\\\"") + "\"]";
+    if (!/^[$\w_]+$/.test(prop) || util.isReserved(prop))
+        return "[\"" + prop.replace(safePropBackslashRe, "\\\\").replace(safePropQuoteRe, "\\\"") + "\"]";
+    return "." + prop;
 };
 
 /**

tests/comp_typescript.js

@@ -1,9 +1,5 @@
 "use strict";
-// uncomment for browser only / non long.js versions
-/*
-/// <reference path="../stub-long.d.ts" />
-/// <reference path="../stub-node.d.ts" />
-*/
+// test currently consists only of not throwing
 var __extends = (this && this.__extends) || (function () {
     var extendStatics = Object.setPrototypeOf ||
         ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
@@ -120,4 +116,3 @@ exports.AwesomeMessage = AwesomeMessage;
 var awesomeMessage = new AwesomeMessage({ awesomeField: "hi" });
 var awesomeBuffer = AwesomeMessage.encode(awesomeMessage).finish();
 var awesomeDecoded = AwesomeMessage.decode(awesomeBuffer);
-// test currently consists only of not throwing

tests/comp_typescript.ts

@@ -1,8 +1,4 @@
-// uncomment for browser only / non long.js versions
-/*
-/// <reference path="../stub-long.d.ts" />
-/// <reference path="../stub-node.d.ts" />
-*/
+// test currently consists only of not throwing
 
 import { Root, Message, Type, Field, MapField, OneOf } from "..";
 
@@ -88,5 +84,3 @@ export class AwesomeMessage extends Message<AwesomeMessage> {
 let awesomeMessage = new AwesomeMessage({ awesomeField: "hi" });
 let awesomeBuffer  = AwesomeMessage.encode(awesomeMessage).finish();
 let awesomeDecoded = AwesomeMessage.decode(awesomeBuffer);
-
-// test currently consists only of not throwing

tests/data/test.d.ts

@@ -95,9 +95,9 @@ export namespace jspb {
         class SpecialCases implements ISpecialCases {
             constructor(properties?: jspb.test.ISpecialCases);
             public normal: string;
-            public default_: string;
-            public function_: string;
-            public var_: string;
+            public default: string;
+            public function: string;
+            public var: string;
             public static create(properties?: jspb.test.ISpecialCases): jspb.test.SpecialCases;
             public static encode(message: jspb.test.ISpecialCases, writer?: $protobuf.Writer): $protobuf.Writer;
             public static encodeDelimited(message: jspb.test.ISpecialCases, writer?: $protobuf.Writer): $protobuf.Writer;
@@ -580,10 +580,10 @@ export namespace jspb {
             public atwo: number;
             public bone: number;
             public btwo: number;
-            public partialOneof?: string;
-            public recursiveOneof?: string;
-            public defaultOneofA?: string;
-            public defaultOneofB?: string;
+            public partialOneof?: ("pone"|"pthree");
+            public recursiveOneof?: ("rone"|"rtwo");
+            public defaultOneofA?: ("aone"|"atwo");
+            public defaultOneofB?: ("bone"|"btwo");
             public static create(properties?: jspb.test.ITestMessageWithOneof): jspb.test.TestMessageWithOneof;
             public static encode(message: jspb.test.ITestMessageWithOneof, writer?: $protobuf.Writer): $protobuf.Writer;
             public static encodeDelimited(message: jspb.test.ITestMessageWithOneof, writer?: $protobuf.Writer): $protobuf.Writer;
@@ -777,7 +777,7 @@ export namespace google {
         class FileDescriptorProto implements IFileDescriptorProto {
             constructor(properties?: google.protobuf.IFileDescriptorProto);
             public name: string;
-            public package_: string;
+            public package: string;
             public dependency: string[];
             public publicDependency: number[];
             public weakDependency: number[];

tests/data/test.js

@@ -913,23 +913,23 @@ $root.jspb = (function() {
 
             /**
              * SpecialCases default.
-             * @member {string} default_
+             * @member {string} default
              * @memberof jspb.test.SpecialCases
              * @instance
              */
             SpecialCases.prototype["default"] = "";
 
             /**
              * SpecialCases function.
-             * @member {string} function_
+             * @member {string} function
              * @memberof jspb.test.SpecialCases
              * @instance
              */
             SpecialCases.prototype["function"] = "";
 
             /**
              * SpecialCases var.
-             * @member {string} var_
+             * @member {string} var
              * @memberof jspb.test.SpecialCases
              * @instance
              */
@@ -6142,7 +6142,7 @@ $root.jspb = (function() {
 
             /**
              * TestMessageWithOneof partialOneof.
-             * @member {string|undefined} partialOneof
+             * @member {"pone"|"pthree"|undefined} partialOneof
              * @memberof jspb.test.TestMessageWithOneof
              * @instance
              */
@@ -6153,7 +6153,7 @@ $root.jspb = (function() {
 
             /**
              * TestMessageWithOneof recursiveOneof.
-             * @member {string|undefined} recursiveOneof
+             * @member {"rone"|"rtwo"|undefined} recursiveOneof
              * @memberof jspb.test.TestMessageWithOneof
              * @instance
              */
@@ -6164,7 +6164,7 @@ $root.jspb = (function() {
 
             /**
              * TestMessageWithOneof defaultOneofA.
-             * @member {string|undefined} defaultOneofA
+             * @member {"aone"|"atwo"|undefined} defaultOneofA
              * @memberof jspb.test.TestMessageWithOneof
              * @instance
              */
@@ -6175,7 +6175,7 @@ $root.jspb = (function() {
 
             /**
              * TestMessageWithOneof defaultOneofB.
-             * @member {string|undefined} defaultOneofB
+             * @member {"bone"|"btwo"|undefined} defaultOneofB
              * @memberof jspb.test.TestMessageWithOneof
              * @instance
              */
@@ -8435,7 +8435,7 @@ $root.google = (function() {
 
             /**
              * FileDescriptorProto package.
-             * @member {string} package_
+             * @member {string} package
              * @memberof google.protobuf.FileDescriptorProto
              * @instance
              */