Merge pull request #1802 from haberman/jsmapbin

JavaScript: segregate references to binary functionality

js/commonjs/export.js

@@ -8,6 +8,7 @@
 goog.require('goog.object');
 goog.require('jspb.BinaryReader');
 goog.require('jspb.BinaryWriter');
+goog.require('jspb.ExtensionFieldBinaryInfo');
 goog.require('jspb.ExtensionFieldInfo');
 goog.require('jspb.Message');
 
@@ -15,6 +16,7 @@ exports.Message = jspb.Message;
 exports.BinaryReader = jspb.BinaryReader;
 exports.BinaryWriter = jspb.BinaryWriter;
 exports.ExtensionFieldInfo = jspb.ExtensionFieldInfo;
+exports.ExtensionFieldBinaryInfo = jspb.ExtensionFieldBinaryInfo;
 
 // These are used by generated code but should not be used directly by clients.
 exports.exportSymbol = goog.exportSymbol;

js/map.js

@@ -44,65 +44,23 @@ goog.forwardDeclare('jspb.BinaryWriter');
  * on ES6 itself.
  *
  * This constructor should only be called from generated message code. It is not
- * intended for general use by library consumers. The callback function
- * arguments are references to methods in `BinaryReader` and `BinaryWriter`, as
- * well as constructors and reader/writer methods in submessage types if
- * appropriate, that are used for binary serialization and parsing.
+ * intended for general use by library consumers.
  *
  * @template K, V
  *
  * @param {!Array<!Array<!Object>>} arr
  *
- * @param {function(this:jspb.BinaryWriter,number,K)=} opt_keyWriterFn
- *     The method on BinaryWriter that writes type K to the stream.
- *
- * @param {function(this:jspb.BinaryReader):K=} opt_keyReaderFn
- *     The method on BinaryReader that reads type K from the stream.
- *
- * @param {function(this:jspb.BinaryWriter,number,V)|
- *         function(this:jspb.BinaryReader,V,?)=} opt_valueWriterFn
- *     The method on BinaryWriter that writes type V to the stream.  May be
- *     writeMessage, in which case the second callback arg form is used.
- *
- * @param {function(this:jspb.BinaryReader):V|
- *         function(this:jspb.BinaryReader,V,
- *                  function(V,!jspb.BinaryReader))=} opt_valueReaderFn
- *    The method on BinaryReader that reads type V from the stream. May be
- *    readMessage, in which case the second callback arg form is used.
- *
  * @param {?function(new:V)|function(new:V,?)=} opt_valueCtor
  *    The constructor for type V, if type V is a message type.
  *
- * @param {?function(V,!jspb.BinaryWriter)=} opt_valueWriterCallback
- *    The BinaryWriter serialization callback for type V, if V is a message
- *    type.
- *
- * @param {?function(V,!jspb.BinaryReader)=} opt_valueReaderCallback
- *    The BinaryReader parsing callback for type V, if V is a message type.
- *
  * @constructor
  * @struct
  */
-jspb.Map = function(
-    arr, opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn, opt_valueReaderFn,
-    opt_valueCtor, opt_valueWriterCallback, opt_valueReaderCallback) {
-
+jspb.Map = function(arr, opt_valueCtor) {
   /** @const @private */
   this.arr_ = arr;
   /** @const @private */
-  this.keyWriterFn_ = opt_keyWriterFn;
-  /** @const @private */
-  this.keyReaderFn_ = opt_keyReaderFn;
-  /** @const @private */
-  this.valueWriterFn_ = opt_valueWriterFn;
-  /** @const @private */
-  this.valueReaderFn_ = opt_valueReaderFn;
-  /** @const @private */
   this.valueCtor_ = opt_valueCtor;
-  /** @const @private */
-  this.valueWriterCallback_ = opt_valueWriterCallback;
-  /** @const @private */
-  this.valueReaderCallback_ = opt_valueReaderCallback;
 
   /** @type {!Object<string, !jspb.Map.Entry_<K,V>>} @private */
   this.map_ = {};
@@ -385,19 +343,29 @@ jspb.Map.prototype.has = function(key) {
  * number.
  * @param {number} fieldNumber
  * @param {!jspb.BinaryWriter} writer
+ * @param {function(this:jspb.BinaryWriter,number,K)=} keyWriterFn
+ *     The method on BinaryWriter that writes type K to the stream.
+ * @param {function(this:jspb.BinaryWriter,number,V)|
+ *         function(this:jspb.BinaryReader,V,?)=} valueWriterFn
+ *     The method on BinaryWriter that writes type V to the stream.  May be
+ *     writeMessage, in which case the second callback arg form is used.
+ * @param {?function(V,!jspb.BinaryWriter)=} opt_valueWriterCallback
+ *    The BinaryWriter serialization callback for type V, if V is a message
+ *    type.
  */
-jspb.Map.prototype.serializeBinary = function(fieldNumber, writer) {
+jspb.Map.prototype.serializeBinary = function(
+    fieldNumber, writer, keyWriterFn, valueWriterFn, opt_valueWriterCallback) {
   var strKeys = this.stringKeys_();
   strKeys.sort();
   for (var i = 0; i < strKeys.length; i++) {
     var entry = this.map_[strKeys[i]];
     writer.beginSubMessage(fieldNumber);
-    this.keyWriterFn_.call(writer, 1, entry.key);
+    keyWriterFn.call(writer, 1, entry.key);
     if (this.valueCtor_) {
-      this.valueWriterFn_.call(writer, 2, this.wrapEntry_(entry),
-                               this.valueWriterCallback_);
+      valueWriterFn.call(writer, 2, this.wrapEntry_(entry),
+                         opt_valueWriterCallback);
     } else {
-      this.valueWriterFn_.call(writer, 2, entry.value);
+      valueWriterFn_.call(writer, 2, entry.value);
     }
     writer.endSubMessage();
   }
@@ -410,8 +378,21 @@ jspb.Map.prototype.serializeBinary = function(fieldNumber, writer) {
  * when a key/value pair submessage is encountered.
  * @param {!jspb.Map} map
  * @param {!jspb.BinaryReader} reader
+ * @param {function(this:jspb.BinaryReader):K=} keyReaderFn
+ *     The method on BinaryReader that reads type K from the stream.
+ *
+ * @param {function(this:jspb.BinaryReader):V|
+ *         function(this:jspb.BinaryReader,V,
+ *                  function(V,!jspb.BinaryReader))=} valueReaderFn
+ *    The method on BinaryReader that reads type V from the stream. May be
+ *    readMessage, in which case the second callback arg form is used.
+ *
+ * @param {?function(V,!jspb.BinaryReader)=} opt_valueReaderCallback
+ *    The BinaryReader parsing callback for type V, if V is a message type.
+ *
  */
-jspb.Map.deserializeBinary = function(map, reader) {
+jspb.Map.deserializeBinary = function(map, reader, keyReaderFn, valueReaderFn,
+                                      opt_valueReaderCallback) {
   var key = undefined;
   var value = undefined;
 
@@ -422,14 +403,14 @@ jspb.Map.deserializeBinary = function(map, reader) {
     var field = reader.getFieldNumber();
     if (field == 1) {
       // Key.
-      key = map.keyReaderFn_.call(reader);
+      key = keyReaderFn.call(reader);
     } else if (field == 2) {
       // Value.
       if (map.valueCtor_) {
         value = new map.valueCtor_();
-        map.valueReaderFn_.call(reader, value, map.valueReaderCallback_);
+        valueReaderFn.call(reader, value, opt_valueReaderCallback);
       } else {
-        value = map.valueReaderFn_.call(reader);
+        value = valueReaderFn.call(reader);
       }
     }
   }

js/message.js

@@ -34,6 +34,7 @@
  * @author mwr@google.com (Mark Rawling)
  */
 
+goog.provide('jspb.ExtensionFieldBinaryInfo');
 goog.provide('jspb.ExtensionFieldInfo');
 goog.provide('jspb.Message');
 
@@ -84,19 +85,12 @@ goog.forwardDeclare('xid.String');
  * @param {?function(new: jspb.Message, Array=)} ctor
  * @param {?function((boolean|undefined),!jspb.Message):!Object} toObjectFn
  * @param {number} isRepeated
- * @param {?function(number,?)=} opt_binaryReaderFn
- * @param {?function(number,?)|function(number,?,?,?,?,?)=} opt_binaryWriterFn
- * @param {?function(?,?)=} opt_binaryMessageSerializeFn
- * @param {?function(?,?)=} opt_binaryMessageDeserializeFn
- * @param {?boolean=} opt_isPacked
  * @constructor
  * @struct
  * @template T
  */
 jspb.ExtensionFieldInfo = function(fieldNumber, fieldName, ctor, toObjectFn,
-    isRepeated, opt_binaryReaderFn, opt_binaryWriterFn,
-    opt_binaryMessageSerializeFn, opt_binaryMessageDeserializeFn,
-    opt_isPacked) {
+    isRepeated) {
   /** @const */
   this.fieldIndex = fieldNumber;
   /** @const */
@@ -106,20 +100,37 @@ jspb.ExtensionFieldInfo = function(fieldNumber, fieldName, ctor, toObjectFn,
   /** @const */
   this.toObjectFn = toObjectFn;
   /** @const */
-  this.binaryReaderFn = opt_binaryReaderFn;
+  this.isRepeated = isRepeated;
+};
+
+/**
+ * Stores binary-related information for a single extension field.
+ * @param {!jspb.ExtensionFieldInfo<T>} fieldInfo
+ * @param {?function(number,?)=} binaryReaderFn
+ * @param {?function(number,?)|function(number,?,?,?,?,?)=} binaryWriterFn
+ * @param {?function(?,?)=} opt_binaryMessageSerializeFn
+ * @param {?function(?,?)=} opt_binaryMessageDeserializeFn
+ * @param {?boolean=} opt_isPacked
+ * @constructor
+ * @struct
+ * @template T
+ */
+jspb.ExtensionFieldBinaryInfo = function(fieldInfo, binaryReaderFn, binaryWriterFn,
+    binaryMessageSerializeFn, binaryMessageDeserializeFn, isPacked) {
+  /** @const */
+  this.fieldInfo = fieldInfo;
   /** @const */
-  this.binaryWriterFn = opt_binaryWriterFn;
+  this.binaryReaderFn = binaryReaderFn;
   /** @const */
-  this.binaryMessageSerializeFn = opt_binaryMessageSerializeFn;
+  this.binaryWriterFn = binaryWriterFn;
   /** @const */
-  this.binaryMessageDeserializeFn = opt_binaryMessageDeserializeFn;
+  this.binaryMessageSerializeFn = binaryMessageSerializeFn;
   /** @const */
-  this.isRepeated = isRepeated;
+  this.binaryMessageDeserializeFn = binaryMessageDeserializeFn;
   /** @const */
-  this.isPacked = opt_isPacked;
+  this.isPacked = isPacked;
 };
 
-
 /**
  * @return {boolean} Does this field represent a sub Message?
  */
@@ -491,11 +502,13 @@ jspb.Message.toObjectExtension = function(proto, obj, extensions,
 jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions,
     getExtensionFn) {
   for (var fieldNumber in extensions) {
-    var fieldInfo = extensions[fieldNumber];
+    var binaryFieldInfo = extensions[fieldNumber];
+    var fieldInfo = binaryFieldInfo.fieldInfo;
+
     // The old codegen doesn't add the extra fields to ExtensionFieldInfo, so we
     // need to gracefully error-out here rather than produce a null dereference
     // below.
-    if (!fieldInfo.binaryWriterFn) {
+    if (!binaryFieldInfo.binaryWriterFn) {
       throw new Error('Message extension present that was generated ' +
                       'without binary serialization support');
     }
@@ -508,16 +521,17 @@ jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions,
         // message may require binary support, so we can *only* catch this error
         // here, at runtime (and this decoupled codegen is the whole point of
         // extensions!).
-        if (fieldInfo.binaryMessageSerializeFn) {
-          fieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex,
-              value, fieldInfo.binaryMessageSerializeFn);
+        if (binaryFieldInfo.binaryMessageSerializeFn) {
+          binaryFieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex,
+              value, binaryFieldInfo.binaryMessageSerializeFn);
         } else {
           throw new Error('Message extension present holding submessage ' +
                           'without binary support enabled, and message is ' +
                           'being serialized to binary format');
         }
       } else {
-        fieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex, value);
+        binaryFieldInfo.binaryWriterFn.call(
+            writer, fieldInfo.fieldIndex, value);
       }
     }
   }
@@ -535,12 +549,13 @@ jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions,
  */
 jspb.Message.readBinaryExtension = function(msg, reader, extensions,
     getExtensionFn, setExtensionFn) {
-  var fieldInfo = extensions[reader.getFieldNumber()];
-  if (!fieldInfo) {
+  var binaryFieldInfo = extensions[reader.getFieldNumber()];
+  var fieldInfo = binaryFieldInfo.fieldInfo;
+  if (!binaryFieldInfo) {
     reader.skipField();
     return;
   }
-  if (!fieldInfo.binaryReaderFn) {
+  if (!binaryFieldInfo.binaryReaderFn) {
     throw new Error('Deserializing extension whose generated code does not ' +
                     'support binary format');
   }
@@ -548,14 +563,14 @@ jspb.Message.readBinaryExtension = function(msg, reader, extensions,
   var value;
   if (fieldInfo.isMessageType()) {
     value = new fieldInfo.ctor();
-    fieldInfo.binaryReaderFn.call(
-        reader, value, fieldInfo.binaryMessageDeserializeFn);
+    binaryFieldInfo.binaryReaderFn.call(
+        reader, value, binaryFieldInfo.binaryMessageDeserializeFn);
   } else {
     // All other types.
-    value = fieldInfo.binaryReaderFn.call(reader);
+    value = binaryFieldInfo.binaryReaderFn.call(reader);
   }
 
-  if (fieldInfo.isRepeated && !fieldInfo.isPacked) {
+  if (fieldInfo.isRepeated && !binaryFieldInfo.isPacked) {
     var currentList = getExtensionFn.call(msg, fieldInfo);
     if (!currentList) {
       setExtensionFn.call(msg, fieldInfo, [value]);
@@ -747,29 +762,16 @@ jspb.Message.getFieldProto3 = function(msg, fieldNumber, defaultValue) {
  * of serialization/parsing callbacks (which are required by the map at
  * construction time, and the map may be constructed here).
  *
- * The below callbacks are used to allow the map to serialize and parse its
- * binary wire format data. Their purposes are described in more detail in
- * `jspb.Map`'s constructor documentation.
- *
  * @template K, V
  * @param {!jspb.Message} msg
  * @param {number} fieldNumber
  * @param {boolean|undefined} noLazyCreate
  * @param {?=} opt_valueCtor
- * @param {function(number,K)=} opt_keyWriterFn
- * @param {function():K=} opt_keyReaderFn
- * @param {function(number,V)|function(number,V,?)|
- *         function(number,V,?,?,?,?)=} opt_valueWriterFn
- * @param {function():V|
- *         function(V,function(?,?))=} opt_valueReaderFn
- * @param {function(?,?)|function(?,?,?,?,?)=} opt_valueWriterCallback
- * @param {function(?,?)=} opt_valueReaderCallback
  * @return {!jspb.Map<K, V>|undefined}
  * @protected
  */
 jspb.Message.getMapField = function(msg, fieldNumber, noLazyCreate,
-    opt_valueCtor, opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn,
-    opt_valueReaderFn, opt_valueWriterCallback, opt_valueReaderCallback) {
+    opt_valueCtor) {
   if (!msg.wrappers_) {
     msg.wrappers_ = {};
   }
@@ -787,10 +789,7 @@ jspb.Message.getMapField = function(msg, fieldNumber, noLazyCreate,
     }
     return msg.wrappers_[fieldNumber] =
         new jspb.Map(
-            /** @type {!Array<!Array<!Object>>} */ (arr),
-            opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn,
-            opt_valueReaderFn, opt_valueCtor, opt_valueWriterCallback,
-            opt_valueReaderCallback);
+            /** @type {!Array<!Array<!Object>>} */ (arr), opt_valueCtor);
   }
 };
 

src/google/protobuf/compiler/js/js_generator.cc

@@ -2252,25 +2252,6 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
           "      null");
     }
 
-    if (options.binary) {
-      printer->Print(",\n"
-          "      $keyWriterFn$,\n"
-          "      $keyReaderFn$,\n"
-          "      $valueWriterFn$,\n"
-          "      $valueReaderFn$",
-          "keyWriterFn", JSBinaryWriterMethodName(options, key_field),
-          "keyReaderFn", JSBinaryReaderMethodName(options, key_field),
-          "valueWriterFn", JSBinaryWriterMethodName(options, value_field),
-          "valueReaderFn", JSBinaryReaderMethodName(options, value_field));
-
-      if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) {
-        printer->Print(",\n"
-            "      $messageType$.serializeBinaryToWriter,\n"
-            "      $messageType$.deserializeBinaryFromReader",
-            "messageType", GetPath(options, value_field->message_type()));
-      }
-    }
-
     printer->Print(
         "));\n");
 
@@ -2555,6 +2536,29 @@ void Generator::GenerateClassExtensionFieldInfo(const GeneratorOptions& options,
         "$class$.extensions = {};\n"
         "\n",
         "class", GetPath(options, desc));
+
+    if (options.binary) {
+      printer->Print(
+          "\n"
+          "/**\n"
+          " * The extensions registered with this message class. This is a "
+          "map of\n"
+          " * extension field number to fieldInfo object.\n"
+          " *\n"
+          " * For example:\n"
+          " *     { 123: {fieldIndex: 123, fieldName: {my_field_name: 0}, "
+          "ctor: proto.example.MyMessage} }\n"
+          " *\n"
+          " * fieldName contains the JsCompiler renamed field name property "
+          "so that it\n"
+          " * works in OPTIMIZED mode.\n"
+          " *\n"
+          " * @type {!Object.<number, jspb.ExtensionFieldInfo>}\n"
+          " */\n"
+          "$class$.extensionsBinary = {};\n"
+          "\n",
+          "class", GetPath(options, desc));
+    }
   }
 }
 
@@ -2602,7 +2606,7 @@ void Generator::GenerateClassDeserializeBinary(const GeneratorOptions& options,
       "    default:\n");
   if (IsExtendable(desc)) {
     printer->Print(
-        "      jspb.Message.readBinaryExtension(msg, reader, $extobj$,\n"
+        "      jspb.Message.readBinaryExtension(msg, reader, $extobj$Binary,\n"
         "        $class$.prototype.getExtension,\n"
         "        $class$.prototype.setExtension);\n"
         "      break;\n",
@@ -2632,10 +2636,25 @@ void Generator::GenerateClassDeserializeBinaryField(
                  "num", SimpleItoa(field->number()));
 
   if (field->is_map()) {
+    const FieldDescriptor* key_field = MapFieldKey(field);
+    const FieldDescriptor* value_field = MapFieldValue(field);
     printer->Print(
         "      var value = msg.get$name$();\n"
-        "      reader.readMessage(value, jspb.Map.deserializeBinary);\n",
+        "      reader.readMessage(value, function(message, reader) {\n",
         "name", JSGetterName(options, field));
+
+    printer->Print("        jspb.Map.deserializeBinary(message, reader, "
+                   "$keyReaderFn$, $valueReaderFn$",
+          "keyReaderFn", JSBinaryReaderMethodName(options, key_field),
+          "valueReaderFn", JSBinaryReaderMethodName(options, value_field));
+
+    if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) {
+      printer->Print(", $messageType$.deserializeBinaryFromReader",
+          "messageType", GetPath(options, value_field->message_type()));
+    }
+
+    printer->Print(");\n");
+    printer->Print("         });\n");
   } else {
     if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
       printer->Print(
@@ -2721,8 +2740,8 @@ void Generator::GenerateClassSerializeBinary(const GeneratorOptions& options,
 
   if (IsExtendable(desc)) {
     printer->Print(
-        "  jspb.Message.serializeBinaryExtensions(this, writer, $extobj$,\n"
-        "    $class$.prototype.getExtension);\n",
+        "  jspb.Message.serializeBinaryExtensions(this, writer,\n"
+        "    $extobj$Binary, $class$.prototype.getExtension);\n",
         "extobj", JSExtensionsObjectName(options, desc->file(), desc),
         "class", GetPath(options, desc));
   }
@@ -2794,9 +2813,21 @@ void Generator::GenerateClassSerializeBinaryField(
 
   // Write the field on the wire.
   if (field->is_map()) {
+    const FieldDescriptor* key_field = MapFieldKey(field);
+    const FieldDescriptor* value_field = MapFieldValue(field);
     printer->Print(
-        "    f.serializeBinary($index$, writer);\n",
-        "index", SimpleItoa(field->number()));
+        "    f.serializeBinary($index$, writer, "
+                              "$keyWriterFn$, $valueWriterFn$",
+        "index", SimpleItoa(field->number()),
+        "keyWriterFn", JSBinaryWriterMethodName(options, key_field),
+        "valueWriterFn", JSBinaryWriterMethodName(options, value_field));
+
+    if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) {
+      printer->Print(", $messageType$.serializeBinaryToWriter",
+          "messageType", GetPath(options, value_field->message_type()));
+    }
+
+    printer->Print(");\n");
   } else {
     printer->Print(
         "    writer.write$method$(\n"
@@ -2878,7 +2909,7 @@ void Generator::GenerateExtension(const GeneratorOptions& options,
       "     /** @type {?function((boolean|undefined),!jspb.Message=): "
       "!Object} */ (\n"
       "         $toObject$),\n"
-      "    $repeated$",
+      "    $repeated$);\n",
       "index", SimpleItoa(field->number()),
       "name", JSObjectFieldName(options, field),
       "ctor", (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE ?
@@ -2890,12 +2921,18 @@ void Generator::GenerateExtension(const GeneratorOptions& options,
 
   if (options.binary) {
     printer->Print(
-        ",\n"
+        "\n"
+        "$extendName$Binary[$index$] = new jspb.ExtensionFieldBinaryInfo(\n"
+        "    $class$.$name$,\n"
         "    $binaryReaderFn$,\n"
         "    $binaryWriterFn$,\n"
         "    $binaryMessageSerializeFn$,\n"
-        "    $binaryMessageDeserializeFn$,\n"
-        "    $isPacked$);\n",
+        "    $binaryMessageDeserializeFn$,\n",
+        "extendName", JSExtensionsObjectName(options, field->file(),
+                                             field->containing_type()),
+        "index", SimpleItoa(field->number()),
+        "class", extension_scope,
+        "name", JSObjectFieldName(options, field),
         "binaryReaderFn", JSBinaryReaderMethodName(options, field),
         "binaryWriterFn", JSBinaryWriterMethodName(options, field),
         "binaryMessageSerializeFn",
@@ -2905,10 +2942,11 @@ void Generator::GenerateExtension(const GeneratorOptions& options,
         "binaryMessageDeserializeFn",
         (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) ?
         (SubmessageTypeRef(options, field) +
-         ".deserializeBinaryFromReader") : "null",
+         ".deserializeBinaryFromReader") : "null");
+
+    printer->Print(
+        "    $isPacked$);\n",
         "isPacked", (field->is_packed() ? "true" : "false"));
-  } else {
-    printer->Print(");\n");
   }
 
   printer->Print(