Merge pull request #1447 from seishun/defaults

JavaScript: Make implicit defaults consistent with explicit defaults

js/debug.js

@@ -94,8 +94,10 @@ jspb.debug.dump_ = function(thing) {
     var match = /^get([A-Z]\w*)/.exec(name);
     if (match && name != 'getExtension' &&
         name != 'getJsPbMessageId') {
-      var val = thing[name]();
-      if (val != null) {
+      var has = 'has' + match[1];
+      if (!thing[has] || thing[has]())
+      {
+        var val = thing[name]();
         object[jspb.debug.formatFieldName_(match[1])] = jspb.debug.dump_(val);
       }
     }

js/message_test.js

@@ -276,9 +276,6 @@ describe('Message test suite', function() {
   });
 
   it('testClearFields', function() {
-    // We don't set 'proper' defaults, rather, bools, strings,
-    // etc, are cleared to undefined or null and take on the Javascript
-    // meaning for that value. Repeated fields are set to [] when cleared.
     var data = ['str', true, [11], [[22], [33]], ['s1', 's2']];
     var foo = new proto.jspb.test.OptionalFields(data);
     foo.clearAString();
@@ -286,8 +283,8 @@ describe('Message test suite', function() {
     foo.clearANestedMessage();
     foo.clearARepeatedMessageList();
     foo.clearARepeatedStringList();
-    assertUndefined(foo.getAString());
-    assertUndefined(foo.getABool());
+    assertEquals('', foo.getAString());
+    assertEquals(false, foo.getABool());
     assertUndefined(foo.getANestedMessage());
     assertFalse(foo.hasAString());
     assertFalse(foo.hasABool());
@@ -310,8 +307,8 @@ describe('Message test suite', function() {
     foo.setANestedMessage(null);
     foo.setARepeatedMessageList(null);
     foo.setARepeatedStringList(null);
-    assertNull(foo.getAString());
-    assertNull(foo.getABool());
+    assertEquals('', foo.getAString());
+    assertEquals(false, foo.getABool());
     assertNull(foo.getANestedMessage());
     assertFalse(foo.hasAString());
     assertFalse(foo.hasABool());
@@ -328,8 +325,8 @@ describe('Message test suite', function() {
     foo.setANestedMessage(undefined);
     foo.setARepeatedMessageList(undefined);
     foo.setARepeatedStringList(undefined);
-    assertUndefined(foo.getAString());
-    assertUndefined(foo.getABool());
+    assertEquals('', foo.getAString());
+    assertEquals(false, foo.getABool());
     assertUndefined(foo.getANestedMessage());
     assertFalse(foo.hasAString());
     assertFalse(foo.hasABool());
@@ -346,9 +343,9 @@ describe('Message test suite', function() {
                                                {1000: 'unique'}]);
     var diff = /** @type {proto.jspb.test.HasExtensions} */
         (jspb.Message.difference(p1, p2));
-    assertUndefined(diff.getStr1());
+    assertEquals('', diff.getStr1());
     assertEquals('what', diff.getStr2());
-    assertUndefined(diff.getStr3());
+    assertEquals('', diff.getStr3());
     assertEquals('unique', diff.extensionObject_[1000]);
   });
 
@@ -806,7 +803,7 @@ describe('Message test suite', function() {
     var message = new proto.jspb.test.TestMessageWithOneof([,, 'x']);
 
     assertEquals('x', message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPthree());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.PartialOneofCase.PONE,
         message.getPartialOneofCase());
@@ -815,7 +812,7 @@ describe('Message test suite', function() {
   it('testKeepsLastWireValueSetInUnion_multipleValues', function() {
     var message = new proto.jspb.test.TestMessageWithOneof([,, 'x',, 'y']);
 
-    assertUndefined('x', message.getPone());
+    assertEquals('', message.getPone());
     assertEquals('y', message.getPthree());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.PartialOneofCase.PTHREE,
@@ -824,19 +821,19 @@ describe('Message test suite', function() {
 
   it('testSettingOneofFieldClearsOthers', function() {
     var message = new proto.jspb.test.TestMessageWithOneof;
-    assertUndefined(message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPone());
+    assertEquals('', message.getPthree());
     assertFalse(message.hasPone());
     assertFalse(message.hasPthree());
 
     message.setPone('hi');
     assertEquals('hi', message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPthree());
     assertTrue(message.hasPone());
     assertFalse(message.hasPthree());
 
     message.setPthree('bye');
-    assertUndefined(message.getPone());
+    assertEquals('', message.getPone());
     assertEquals('bye', message.getPthree());
     assertFalse(message.hasPone());
     assertTrue(message.hasPthree());
@@ -845,8 +842,8 @@ describe('Message test suite', function() {
   it('testSettingOneofFieldDoesNotClearFieldsFromOtherUnions', function() {
     var other = new proto.jspb.test.TestMessageWithOneof;
     var message = new proto.jspb.test.TestMessageWithOneof;
-    assertUndefined(message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPone());
+    assertEquals('', message.getPthree());
     assertUndefined(message.getRone());
     assertFalse(message.hasPone());
     assertFalse(message.hasPthree());
@@ -854,13 +851,13 @@ describe('Message test suite', function() {
     message.setPone('hi');
     message.setRone(other);
     assertEquals('hi', message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPthree());
     assertEquals(other, message.getRone());
     assertTrue(message.hasPone());
     assertFalse(message.hasPthree());
 
     message.setPthree('bye');
-    assertUndefined(message.getPone());
+    assertEquals('', message.getPone());
     assertEquals('bye', message.getPthree());
     assertEquals(other, message.getRone());
     assertFalse(message.hasPone());
@@ -889,7 +886,7 @@ describe('Message test suite', function() {
   it('testMessageWithDefaultOneofValues', function() {
     var message = new proto.jspb.test.TestMessageWithOneof;
     assertEquals(1234, message.getAone());
-    assertUndefined(message.getAtwo());
+    assertEquals(0, message.getAtwo());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.DefaultOneofACase
             .DEFAULT_ONEOF_A_NOT_SET,
@@ -897,7 +894,7 @@ describe('Message test suite', function() {
 
     message.setAone(567);
     assertEquals(567, message.getAone());
-    assertUndefined(message.getAtwo());
+    assertEquals(0, message.getAtwo());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.DefaultOneofACase.AONE,
         message.getDefaultOneofACase());
@@ -911,7 +908,7 @@ describe('Message test suite', function() {
 
     message.clearAtwo();
     assertEquals(1234, message.getAone());
-    assertUndefined(message.getAtwo());
+    assertEquals(0, message.getAtwo());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.DefaultOneofACase
             .DEFAULT_ONEOF_A_NOT_SET,
@@ -920,7 +917,7 @@ describe('Message test suite', function() {
 
   it('testMessageWithDefaultOneofValues_defaultNotOnFirstField', function() {
     var message = new proto.jspb.test.TestMessageWithOneof;
-    assertUndefined(message.getBone());
+    assertEquals(0, message.getBone());
     assertEquals(1234, message.getBtwo());
     assertFalse(message.hasBone());
     assertFalse(message.hasBtwo());
@@ -939,7 +936,7 @@ describe('Message test suite', function() {
         message.getDefaultOneofBCase());
 
     message.setBtwo(3);
-    assertUndefined(message.getBone());
+    assertEquals(0, message.getBone());
     assertFalse(message.hasBone());
     assertTrue(message.hasBtwo());
     assertEquals(3, message.getBtwo());
@@ -948,7 +945,7 @@ describe('Message test suite', function() {
         message.getDefaultOneofBCase());
 
     message.clearBtwo();
-    assertUndefined(message.getBone());
+    assertEquals(0, message.getBone());
     assertFalse(message.hasBone());
     assertFalse(message.hasBtwo());
     assertEquals(1234, message.getBtwo());
@@ -962,7 +959,7 @@ describe('Message test suite', function() {
     var message =
         new proto.jspb.test.TestMessageWithOneof(new Array(9).concat(567));
     assertEquals(567, message.getAone());
-    assertUndefined(message.getAtwo());
+    assertEquals(0, message.getAtwo());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.DefaultOneofACase.AONE,
         message.getDefaultOneofACase());
@@ -998,7 +995,7 @@ describe('Message test suite', function() {
 
         message =
             new proto.jspb.test.TestMessageWithOneof(new Array(12).concat(890));
-        assertUndefined(message.getBone());
+        assertEquals(0, message.getBone());
         assertEquals(890, message.getBtwo());
         assertEquals(
             proto.jspb.test.TestMessageWithOneof.DefaultOneofBCase.BTWO,
@@ -1006,7 +1003,7 @@ describe('Message test suite', function() {
 
         message = new proto.jspb.test.TestMessageWithOneof(
             new Array(11).concat(567, 890));
-        assertUndefined(message.getBone());
+        assertEquals(0, message.getBone());
         assertEquals(890, message.getBtwo());
         assertEquals(
             proto.jspb.test.TestMessageWithOneof.DefaultOneofBCase.BTWO,
@@ -1023,7 +1020,7 @@ describe('Message test suite', function() {
     var other = new proto.jspb.test.TestMessageWithOneof;
     message.setRone(other);
     assertEquals(other, message.getRone());
-    assertUndefined(message.getRtwo());
+    assertEquals('', message.getRtwo());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.RecursiveOneofCase.RONE,
         message.getRecursiveOneofCase());
@@ -1041,7 +1038,7 @@ describe('Message test suite', function() {
     var message = new proto.jspb.test.TestMessageWithOneof;
     message.setPone('x');
     assertEquals('x', message.getPone());
-    assertUndefined(message.getPthree());
+    assertEquals('', message.getPthree());
     assertEquals(
         proto.jspb.test.TestMessageWithOneof.PartialOneofCase.PONE,
         message.getPartialOneofCase());

js/proto3_test.js

@@ -221,10 +221,10 @@ describe('proto3Test', function() {
   it('testOneofs', function() {
     var msg = new proto.jspb.test.TestProto3();
 
-    assertEquals(msg.getOneofUint32(), undefined);
+    assertEquals(msg.getOneofUint32(), 0);
     assertEquals(msg.getOneofForeignMessage(), undefined);
-    assertEquals(msg.getOneofString(), undefined);
-    assertEquals(msg.getOneofBytes(), undefined);
+    assertEquals(msg.getOneofString(), '');
+    assertEquals(msg.getOneofBytes(), '');
     assertFalse(msg.hasOneofUint32());
     assertFalse(msg.hasOneofString());
     assertFalse(msg.hasOneofBytes());
@@ -232,8 +232,8 @@ describe('proto3Test', function() {
     msg.setOneofUint32(42);
     assertEquals(msg.getOneofUint32(), 42);
     assertEquals(msg.getOneofForeignMessage(), undefined);
-    assertEquals(msg.getOneofString(), undefined);
-    assertEquals(msg.getOneofBytes(), undefined);
+    assertEquals(msg.getOneofString(), '');
+    assertEquals(msg.getOneofBytes(), '');
     assertTrue(msg.hasOneofUint32());
     assertFalse(msg.hasOneofString());
     assertFalse(msg.hasOneofBytes());
@@ -241,27 +241,27 @@ describe('proto3Test', function() {
 
     var submsg = new proto.jspb.test.ForeignMessage();
     msg.setOneofForeignMessage(submsg);
-    assertEquals(msg.getOneofUint32(), undefined);
+    assertEquals(msg.getOneofUint32(), 0);
     assertEquals(msg.getOneofForeignMessage(), submsg);
-    assertEquals(msg.getOneofString(), undefined);
-    assertEquals(msg.getOneofBytes(), undefined);
+    assertEquals(msg.getOneofString(), '');
+    assertEquals(msg.getOneofBytes(), '');
     assertFalse(msg.hasOneofUint32());
     assertFalse(msg.hasOneofString());
     assertFalse(msg.hasOneofBytes());
 
     msg.setOneofString('hello');
-    assertEquals(msg.getOneofUint32(), undefined);
+    assertEquals(msg.getOneofUint32(), 0);
     assertEquals(msg.getOneofForeignMessage(), undefined);
     assertEquals(msg.getOneofString(), 'hello');
-    assertEquals(msg.getOneofBytes(), undefined);
+    assertEquals(msg.getOneofBytes(), '');
     assertFalse(msg.hasOneofUint32());
     assertTrue(msg.hasOneofString());
     assertFalse(msg.hasOneofBytes());
 
     msg.setOneofBytes(goog.crypt.base64.encodeString('\u00FF\u00FF'));
-    assertEquals(msg.getOneofUint32(), undefined);
+    assertEquals(msg.getOneofUint32(), 0);
     assertEquals(msg.getOneofForeignMessage(), undefined);
-    assertEquals(msg.getOneofString(), undefined);
+    assertEquals(msg.getOneofString(), '');
     assertEquals(msg.getOneofBytes_asB64(),
         goog.crypt.base64.encodeString('\u00FF\u00FF'));
     assertFalse(msg.hasOneofUint32());

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

@@ -768,7 +768,6 @@ string MaybeNumberString(const FieldDescriptor* field, const string& orig) {
 }
 
 string JSFieldDefault(const FieldDescriptor* field) {
-  assert(field->has_default_value());
   switch (field->cpp_type()) {
     case FieldDescriptor::CPPTYPE_INT32:
       return MaybeNumberString(
@@ -943,7 +942,7 @@ string JSFieldTypeAnnotation(const GeneratorOptions& options,
   }
 
   if (field->is_optional() && is_primitive &&
-      (!field->has_default_value() || force_optional) && !force_present) {
+      force_optional && !force_present) {
     jstype += "?";
   } else if (field->is_required() && !is_primitive && !force_optional) {
     jstype = "!" + jstype;
@@ -1258,6 +1257,10 @@ string GetPivot(const Descriptor* desc) {
 // Returns true for fields that represent "null" as distinct from the default
 // value. See http://go/proto3#heading=h.kozewqqcqhuz for more information.
 bool HasFieldPresence(const FieldDescriptor* field) {
+  if (field->is_repeated()) {
+    return false;
+  }
+
   return
       (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) ||
       (field->containing_oneof() != NULL) ||
@@ -2387,7 +2390,7 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
                      "index", JSFieldIndex(field),
                      "default", Proto3PrimitiveFieldDefault(field));
     } else {
-      if (field->has_default_value()) {
+      if (!field->is_repeated()) {
         printer->Print("!this.has$name$() ? $defaultValue$ : ",
                        "name", JSGetterName(options, field),
                        "defaultValue", JSFieldDefault(field));
@@ -2398,10 +2401,6 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
           printer->Print("jspb.Message.getRepeatedFloatingPointField("
                          "this, $index$)",
                          "index", JSFieldIndex(field));
-        } else if (field->is_optional() && !field->has_default_value()) {
-          printer->Print("jspb.Message.getOptionalFloatingPointField("
-                         "this, $index$)",
-                         "index", JSFieldIndex(field));
         } else {
           // Convert "NaN" to NaN.
           printer->Print("+jspb.Message.getField(this, $index$)",
@@ -2477,7 +2476,7 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
           "returndoc", JSReturnDoc(options, field));
     }
 
-    if (HasFieldPresence(field)) {
+    if (HasFieldPresence(field) || field->is_repeated()) {
       printer->Print(
           "$class$.prototype.clear$name$ = function() {\n"
           "  jspb.Message.set$oneoftag$Field(this, $index$$oneofgroup$, ",
@@ -2494,22 +2493,24 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
           "\n",
           "clearedvalue", (field->is_repeated() ? "[]" : "undefined"),
           "returnvalue", JSReturnClause(field));
-
-      printer->Print(
-          "/**\n"
-          " * Returns whether this field is set.\n"
-          " * @return{!boolean}\n"
-          " */\n"
-          "$class$.prototype.has$name$ = function() {\n"
-          "  return jspb.Message.getField(this, $index$) != null;\n"
-          "};\n"
-          "\n"
-          "\n",
-          "class", GetPath(options, field->containing_type()),
-          "name", JSGetterName(options, field),
-          "index", JSFieldIndex(field));
     }
   }
+
+  if (HasFieldPresence(field)) {
+    printer->Print(
+        "/**\n"
+        " * Returns whether this field is set.\n"
+        " * @return{!boolean}\n"
+        " */\n"
+        "$class$.prototype.has$name$ = function() {\n"
+        "  return jspb.Message.getField(this, $index$) != null;\n"
+        "};\n"
+        "\n"
+        "\n",
+        "class", GetPath(options, field->containing_type()),
+        "name", JSGetterName(options, field),
+        "index", JSFieldIndex(field));
+  }
 }
 
 void Generator::GenerateClassExtensionFieldInfo(const GeneratorOptions& options,
@@ -2756,11 +2757,18 @@ void Generator::GenerateClassSerializeBinaryField(
     const GeneratorOptions& options,
     io::Printer* printer,
     const FieldDescriptor* field) const {
-  printer->Print(
-      "  f = this.get$name$($nolazy$);\n",
-      "name", JSGetterName(options, field, BYTES_U8),
-      // No lazy creation for maps containers -- fastpath the empty case.
-      "nolazy", (field->is_map()) ? "true" : "");
+  if (HasFieldPresence(field) &&
+      field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) {
+    printer->Print(
+        "  f = jspb.Message.getField(this, $index$);\n",
+        "index", JSFieldIndex(field));
+  } else {
+    printer->Print(
+        "  f = this.get$name$($nolazy$);\n",
+        "name", JSGetterName(options, field, BYTES_U8),
+        // No lazy creation for maps containers -- fastpath the empty case.
+        "nolazy", (field->is_map()) ? "true" : "");
+  }
 
 
   // Print an `if (condition)` statement that evaluates to true if the field