Revert contructor arguments

This PR reverts #442 (7ac17d0) This patch was ported to internal in cl/449707130, but benchmarking revealed that the new constructors add to release binary sizes even when no arguments are passed to the constructors. To keep the two code bases in sync I'm reverting this change. These new constructor arguments were never announced in the changelog, but I still added an entry to show how to migrate code that uses them.
google · Jul 8, 2022 · d973126 · d973126
1 parent 9b074b7
commit d973126
Show file tree

Hide file tree

Showing 14 changed files with 120 additions and 296 deletions.
diff --git a/protoc_plugin/CHANGELOG.md b/protoc_plugin/CHANGELOG.md
@@ -1,10 +1,29 @@
-## 21.0.0
+## 21.0.0-dev
 
 * Identifiers `fromBuffer`, `fromJson`, `$_defaultFor`, `initByValue` are no
   longer reserved. Proto fields with those Dart names will no longer have a
   suffix added. ([#679])
 
+* Message constructor arguments removed. Constructors with arguments cause
+  increase in release binary sizes even when no arguments are passed to the
+  constructors. ([#703])
+
+  **Migration:**
+
+  Set the fields after construction, using cascade syntax. For example, if you
+  have:
+  ```dart
+  MyMessage(a: 123, b: [1, 2, 3])
+  ```
+  You can do:
+  ```dart
+  MyMessage()
+    ..a = 123
+    ..b.addAll([1, 2, 3])
+  ```
+
 [#679]: https://github.com/google/protobuf.dart/pull/679
+[#703]: https://github.com/google/protobuf.dart/pull/703
 
 ## 20.0.1
 

diff --git a/protoc_plugin/lib/src/base_type.dart b/protoc_plugin/lib/src/base_type.dart
@@ -56,9 +56,6 @@ class BaseType {
   String getRepeatedDartType(FileGenerator fileGen) =>
       '$coreImportPrefix.List<${getDartType(fileGen)}>';
 
-  String getRepeatedDartTypeIterable(FileGenerator fileGen) =>
-      '$coreImportPrefix.Iterable<${getDartType(fileGen)}>';
-
   factory BaseType(FieldDescriptorProto field, GenerationContext ctx) {
     String constSuffix;
 

diff --git a/protoc_plugin/lib/src/message_generator.dart b/protoc_plugin/lib/src/message_generator.dart
@@ -362,44 +362,7 @@ class MessageGenerator extends ProtobufContainer {
       out.printlnAnnotated('$classname._() : super();', [
         NamedLocation(name: classname, fieldPathSegment: fieldPath, start: 0)
       ]);
-      out.print('factory $classname(');
-      if (_fieldList.isNotEmpty) {
-        out.println('{');
-        for (final field in _fieldList) {
-          _emitDeprecatedIf(field.isDeprecated, out);
-          if (field.isRepeated && !field.isMapField) {
-            out.println(
-                '  ${field.baseType.getRepeatedDartTypeIterable(fileGen)}? ${field.memberNames!.fieldName},');
-          } else {
-            out.println(
-                '  ${field.getDartType()}? ${field.memberNames!.fieldName},');
-          }
-        }
-        out.print('}');
-      }
-      if (_fieldList.isNotEmpty) {
-        out.println(') {');
-        out.println('  final _result = create();');
-        for (final field in _fieldList) {
-          out.println('  if (${field.memberNames!.fieldName} != null) {');
-          if (field.isDeprecated) {
-            out.println(
-                '    // ignore: deprecated_member_use_from_same_package');
-          }
-          if (field.isRepeated || field.isMapField) {
-            out.println(
-                '    _result.${field.memberNames!.fieldName}.addAll(${field.memberNames!.fieldName});');
-          } else {
-            out.println(
-                '    _result.${field.memberNames!.fieldName} = ${field.memberNames!.fieldName};');
-          }
-          out.println('  }');
-        }
-        out.println('  return _result;');
-        out.println('}');
-      } else {
-        out.println(') => create();');
-      }
+      out.println('factory $classname() => create();');
       out.println(
           'factory $classname.fromBuffer($coreImportPrefix.List<$coreImportPrefix.int> i,'
           ' [$protobufImportPrefix.ExtensionRegistry r = $protobufImportPrefix.ExtensionRegistry.EMPTY])'

diff --git a/protoc_plugin/test/freeze_test.dart b/protoc_plugin/test/freeze_test.dart
@@ -8,11 +8,10 @@ import '../out/protos/nested_message.pb.dart';
 
 void main() {
   test('testFreezingNestedFields', () {
-    var top = Top(
-      nestedMessageList: [Nested(a: 1)],
-      nestedMessageMap: {1: Nested(a: 2)},
-      nestedMessage: Nested(a: 3),
-    );
+    final top = Top()
+      ..nestedMessageList.add(Nested()..a = 1)
+      ..nestedMessageMap[1] = (Nested()..a = 2)
+      ..nestedMessage = (Nested()..a = 3);
 
     // Create aliases to lists, maps, nested messages
     var list = top.nestedMessageList;
@@ -28,24 +27,24 @@ void main() {
     // Check list field
     expect(top.nestedMessageList.length, 1);
     expect(top.nestedMessageList[0].isFrozen, true);
-    expect(() => top.nestedMessageList.add(Nested(a: 0)),
+    expect(() => top.nestedMessageList.add(Nested()..a = 0),
         throwsA(const TypeMatcher<UnsupportedError>()));
 
     // Check map field
     expect(top.nestedMessageMap.length, 1);
     expect(top.nestedMessageMap[1]!.isFrozen, true);
-    expect(() => top.nestedMessageMap[2] = Nested(a: 0),
+    expect(() => top.nestedMessageMap[2] = Nested()..a = 0,
         throwsA(const TypeMatcher<UnsupportedError>()));
-    expect(() => map[0] = Nested(a: 0),
+    expect(() => map[0] = Nested()..a = 0,
         throwsA(const TypeMatcher<UnsupportedError>()));
 
     // Check message field
     expect(top.nestedMessage.isFrozen, true);
 
     // Check aliases
-    expect(() => list.add(Nested(a: 0)),
+    expect(() => list.add(Nested()..a = 0),
         throwsA(const TypeMatcher<UnsupportedError>()));
-    expect(() => map[123] = Nested(a: 0),
+    expect(() => map[123] = Nested()..a = 0,
         throwsA(const TypeMatcher<UnsupportedError>()));
     expect(list[0].isFrozen, true);
     expect(map[1]!.isFrozen, true);

diff --git a/protoc_plugin/test/generated_message_test.dart b/protoc_plugin/test/generated_message_test.dart
@@ -876,88 +876,4 @@ void main() {
     final value2 = value1.deepCopy();
     assertAllExtensionsSet(value2);
   });
-
-  test('named arguments in constructor', () {
-    final value = TestAllTypes(
-      optionalInt32: 101,
-      optionalInt64: make64(102),
-      optionalUint32: 103,
-      optionalUint64: make64(104),
-      optionalSint32: 105,
-      optionalSint64: make64(106),
-      optionalFixed32: 107,
-      optionalFixed64: make64(108),
-      optionalSfixed32: 109,
-      optionalSfixed64: make64(110),
-      optionalFloat: 111.0,
-      optionalDouble: 112.0,
-      optionalBool: true,
-      optionalString: '115',
-      optionalBytes: '116'.codeUnits,
-      optionalGroup: TestAllTypes_OptionalGroup(a: 117),
-      optionalNestedMessage: TestAllTypes_NestedMessage(bb: 118),
-      optionalForeignMessage: ForeignMessage(c: 119),
-      optionalImportMessage: ImportMessage(d: 120),
-      optionalNestedEnum: TestAllTypes_NestedEnum.BAZ,
-      optionalForeignEnum: ForeignEnum.FOREIGN_BAZ,
-      optionalImportEnum: ImportEnum.IMPORT_BAZ,
-      optionalStringPiece: '124',
-      optionalCord: '125',
-      repeatedInt32: [201, 301],
-      repeatedInt64: [make64(202), make64(302)],
-      repeatedUint32: [203, 303],
-      repeatedUint64: [make64(204), make64(304)],
-      repeatedSint32: [205, 305],
-      repeatedSint64: [make64(206), make64(306)],
-      repeatedFixed32: [207, 307],
-      repeatedFixed64: [make64(208), make64(308)],
-      repeatedSfixed32: [209, 309],
-      repeatedSfixed64: [make64(210), make64(310)],
-      repeatedFloat: [211.0, 311.0],
-      repeatedDouble: [212.0, 312.0],
-      repeatedBool: [true, false],
-      repeatedString: ['215', '315'],
-      repeatedBytes: ['216'.codeUnits, '316'.codeUnits],
-      repeatedGroup: [
-        TestAllTypes_RepeatedGroup(a: 217),
-        TestAllTypes_RepeatedGroup(a: 317)
-      ],
-      repeatedNestedMessage: [
-        TestAllTypes_NestedMessage(bb: 218),
-        TestAllTypes_NestedMessage(bb: 318)
-      ],
-      repeatedForeignMessage: [ForeignMessage(c: 219), ForeignMessage(c: 319)],
-      repeatedImportMessage: [ImportMessage(d: 220), ImportMessage(d: 320)],
-      repeatedNestedEnum: [
-        TestAllTypes_NestedEnum.BAR,
-        TestAllTypes_NestedEnum.BAZ
-      ],
-      repeatedForeignEnum: [ForeignEnum.FOREIGN_BAR, ForeignEnum.FOREIGN_BAZ],
-      repeatedImportEnum: [ImportEnum.IMPORT_BAR, ImportEnum.IMPORT_BAZ],
-      repeatedStringPiece: ['224', '324'],
-      repeatedCord: ['225', '325'],
-      defaultInt32: 401,
-      defaultInt64: make64(402),
-      defaultUint32: 403,
-      defaultUint64: make64(404),
-      defaultSint32: 405,
-      defaultSint64: make64(406),
-      defaultFixed32: 407,
-      defaultFixed64: make64(408),
-      defaultSfixed32: 409,
-      defaultSfixed64: make64(410),
-      defaultFloat: 411.0,
-      defaultDouble: 412.0,
-      defaultBool: false,
-      defaultString: '415',
-      defaultBytes: '416'.codeUnits,
-      defaultNestedEnum: TestAllTypes_NestedEnum.FOO,
-      defaultForeignEnum: ForeignEnum.FOREIGN_FOO,
-      defaultImportEnum: ImportEnum.IMPORT_FOO,
-      defaultStringPiece: '424',
-      defaultCord: '425',
-    );
-
-    assertAllFieldsSet(value);
-  });
 }
diff --git a/protoc_plugin/test/goldens/imports.pb b/protoc_plugin/test/goldens/imports.pb
@@ -21,23 +21,7 @@ class M extends $pb.GeneratedMessage {
   ;
 
   M._() : super();
-  factory M({
-    M? m,
-    $1.M? m1,
-    $2.M? m2,
-  }) {
-    final _result = create();
-    if (m != null) {
-      _result.m = m;
-    }
-    if (m1 != null) {
-      _result.m1 = m1;
-    }
-    if (m2 != null) {
-      _result.m2 = m2;
-    }
-    return _result;
-  }
+  factory M() => create();
   factory M.fromBuffer($core.List<$core.int> i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromBuffer(i, r);
   factory M.fromJson($core.String i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromJson(i, r);
   @$core.Deprecated(

diff --git a/protoc_plugin/test/goldens/int64.pb b/protoc_plugin/test/goldens/int64.pb
@@ -17,15 +17,7 @@ class Int64 extends $pb.GeneratedMessage {
   ;
 
   Int64._() : super();
-  factory Int64({
-    $fixnum.Int64? value,
-  }) {
-    final _result = create();
-    if (value != null) {
-      _result.value = value;
-    }
-    return _result;
-  }
+  factory Int64() => create();
   factory Int64.fromBuffer($core.List<$core.int> i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromBuffer(i, r);
   factory Int64.fromJson($core.String i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromJson(i, r);
   @$core.Deprecated(

diff --git a/protoc_plugin/test/goldens/messageGenerator b/protoc_plugin/test/goldens/messageGenerator
@@ -7,29 +7,7 @@ class PhoneNumber extends $pb.GeneratedMessage {
   ;
 
   PhoneNumber._() : super();
-  factory PhoneNumber({
-    $core.String? number,
-    PhoneNumber_PhoneType? type,
-    $core.String? name,
-  @$core.Deprecated('This field is deprecated.')
-    $core.String? deprecatedField,
-  }) {
-    final _result = create();
-    if (number != null) {
-      _result.number = number;
-    }
-    if (type != null) {
-      _result.type = type;
-    }
-    if (name != null) {
-      _result.name = name;
-    }
-    if (deprecatedField != null) {
-      // ignore: deprecated_member_use_from_same_package
-      _result.deprecatedField = deprecatedField;
-    }
-    return _result;
-  }
+  factory PhoneNumber() => create();
   factory PhoneNumber.fromBuffer($core.List<$core.int> i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromBuffer(i, r);
   factory PhoneNumber.fromJson($core.String i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => create()..mergeFromJson(i, r);
   @$core.Deprecated(