From 970a4fda1785de0873378bca12e907e3a86e64d2 Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sun, 24 Apr 2016 14:38:16 +0300 Subject: Make implicit defaults consistent with explicit defaults --- js/message_test.js | 83 ++++++++++--------------- js/proto3_test.js | 24 +++---- src/google/protobuf/compiler/js/js_generator.cc | 77 +++++++++++++---------- 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 -- cgit v1.2.3