Make implicit defaults consistent with explicit defaults

3 files changed, 88 insertions, 96 deletions

diff --git a/js/message_test.js b/js/message_test.js
index 11792423..c9eee17d 100644
--- a/js/message_test.js
+++ b/js/message_test.js
@@ -145,11 +145,7 @@ describe('Message test suite', function() {
         undefined, undefined, undefined, undefined]);
     var result = foo.toObject();
     assertObjectEquals({
-      aString: undefined,
-      anOutOfOrderBool: undefined,
-      aNestedMessage: {
-        anInt: undefined
-      },
+      aNestedMessage: {},
       // Note: JsPb converts undefined repeated fields to empty arrays.
       aRepeatedMessageList: [],
       aRepeatedStringList: []
@@ -184,14 +180,7 @@ describe('Message test suite', function() {
     var response = new proto.jspb.test.DefaultValues();
 
     // Test toObject
-    var expectedObject = {
-      stringField: defaultString,
-      boolField: true,
-      intField: 11,
-      enumField: 13,
-      emptyField: '',
-      bytesField: 'bW9v'
-    };
+    var expectedObject = {};
     assertObjectEquals(expectedObject, response.toObject());
 
 
@@ -276,9 +265,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 +272,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 +296,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 +314,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 +332,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]);
   });
 
@@ -762,12 +748,7 @@ describe('Message test suite', function() {
     assertObjectEquals({id: 'g1', someBoolList: [true, false]},
         groups[0].toObject());
     assertObjectEquals({
-      repeatedGroupList: [{id: 'g1', someBoolList: [true, false]}],
-      requiredGroup: {id: undefined},
-      optionalGroup: undefined,
-      requiredSimple: {aRepeatedStringList: [], aString: undefined},
-      optionalSimple: undefined,
-      id: undefined
+      repeatedGroupList: [{id: 'g1', someBoolList: [true, false]}]
     }, group.toObject());
     var group1 = new proto.jspb.test.TestGroup1();
     group1.setGroup(someGroup);
@@ -806,7 +787,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 +796,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 +805,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 +826,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 +835,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 +870,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 +878,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 +892,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 +901,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 +920,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 +929,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 +943,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 +979,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 +987,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 +1004,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 +1022,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());
diff --git a/js/proto3_test.js b/js/proto3_test.js
index 7f76006a..fab0fd44 100644
--- a/js/proto3_test.js
+++ b/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());
diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc
index a24686b6..7829ccbf 100755
--- a/src/google/protobuf/compiler/js/js_generator.cc
+++ b/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;
@@ -1259,9 +1258,10 @@ string GetPivot(const Descriptor* desc) {
 // value. See http://go/proto3#heading=h.kozewqqcqhuz for more information.
 bool HasFieldPresence(const FieldDescriptor* field) {
   return
-      (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) ||
+      !field->is_repeated() &&
+      ((field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) ||
       (field->containing_oneof() != NULL) ||
-      (field->file()->syntax() != FileDescriptor::SYNTAX_PROTO3);
+      (field->file()->syntax() != FileDescriptor::SYNTAX_PROTO3));
 }
 
 // For proto3 fields without presence, returns a string representing the default
@@ -1949,7 +1949,7 @@ void Generator::GenerateClassToObject(const GeneratorOptions& options,
       " * @return {!Object}\n"
       " */\n"
       "$classname$.toObject = function(includeInstance, msg) {\n"
-      "  var f, obj = {",
+      "  var f, obj = {};",
       "classname", GetPath(options, desc));
 
   bool first = true;
@@ -1960,20 +1960,16 @@ void Generator::GenerateClassToObject(const GeneratorOptions& options,
     }
 
     if (!first) {
-      printer->Print(",\n    ");
+      printer->Print("\n  ");
     } else {
-      printer->Print("\n    ");
+      printer->Print("\n\n  ");
       first = false;
     }
 
     GenerateClassFieldToObject(options, printer, field);
   }
 
-  if (!first) {
-    printer->Print("\n  };\n\n");
-  } else {
-    printer->Print("\n\n  };\n\n");
-  }
+  printer->Print("\n\n");
 
   if (IsExtendable(desc)) {
     printer->Print(
@@ -2000,7 +1996,12 @@ void Generator::GenerateClassToObject(const GeneratorOptions& options,
 void Generator::GenerateClassFieldToObject(const GeneratorOptions& options,
                                            io::Printer* printer,
                                            const FieldDescriptor* field) const {
-  printer->Print("$fieldname$: ",
+  if (HasFieldPresence(field)) {
+    printer->Print("if (msg.has$name$()) ",
+      "name", JSGetterName(options, field));
+  }
+
+  printer->Print("obj.$fieldname$ = ",
                  "fieldname", JSObjectFieldName(options, field));
 
   if (field->is_map()) {
@@ -2030,21 +2031,12 @@ void Generator::GenerateClassFieldToObject(const GeneratorOptions& options,
       printer->Print("msg.get$getter$()",
                      "getter", JSGetterName(options, field, BYTES_B64));
     } else {
-      if (field->has_default_value()) {
-        printer->Print("!msg.has$name$() ? $defaultValue$ : ",
-                       "name", JSGetterName(options, field),
-                       "defaultValue", JSFieldDefault(field));
-      }
       if (field->cpp_type() == FieldDescriptor::CPPTYPE_FLOAT ||
           field->cpp_type() == FieldDescriptor::CPPTYPE_DOUBLE) {
         if (field->is_repeated()) {
           printer->Print("jspb.Message.getRepeatedFloatingPointField("
                          "msg, $index$)",
                          "index", JSFieldIndex(field));
-        } else if (field->is_optional() && !field->has_default_value()) {
-          printer->Print("jspb.Message.getOptionalFloatingPointField("
-                         "msg, $index$)",
-                         "index", JSFieldIndex(field));
         } else {
           // Convert "NaN" to NaN.
           printer->Print("+jspb.Message.getField(msg, $index$)",
@@ -2330,6 +2322,20 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
         "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));
+
   } else {
     bool untyped =
         false;
@@ -2387,7 +2393,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 +2404,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 +2479,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,7 +2496,9 @@ void Generator::GenerateClassField(const GeneratorOptions& options,
           "\n",
           "clearedvalue", (field->is_repeated() ? "[]" : "undefined"),
           "returnvalue", JSReturnClause(field));
+    }
 
+    if (HasFieldPresence(field)) {
       printer->Print(
           "/**\n"
           " * Returns whether this field is set.\n"
@@ -2756,11 +2760,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