Generate constructor arguments again, add a flag to disable (#855)

See changelog for details. Fixes #850.
google · Aug 14, 2023 · 8505c58 · 8505c58
1 parent 217c030
commit 8505c58
Show file tree

Hide file tree

Showing 8 changed files with 229 additions and 25 deletions.
diff --git a/protoc_plugin/CHANGELOG.md b/protoc_plugin/CHANGELOG.md
@@ -1,8 +1,23 @@
 ## 21.1.0-dev
 
 * Generate code comments for annotated protobuf inputs. ([#161])
+* Generate message constructor arguments by default again. New flag
+  `disable_constructor_args` disables generating the arguments.
+
+  Constructor arguments were removed in 21.0.0 as they increase dart2js binary
+  sizes even when the arguments are not used.
+
+  Example usage to disable constructor arguments:
+
+  ```
+  protoc --dart_out='disable_constructor_args,<other options>:.' ...
+  ```
+
+  ([#850], [#855])
 
 [#161]: https://github.com/google/protobuf.dart/issues/161
+[#850]: https://github.com/google/protobuf.dart/issues/850
+[#855]: https://github.com/google/protobuf.dart/pull/855
 
 ## 21.0.2
 

diff --git a/protoc_plugin/Makefile b/protoc_plugin/Makefile
@@ -78,13 +78,25 @@ PREGENERATED_SRCS=protos/descriptor.proto protos/plugin.proto protos/dart_option
 
 $(TEST_PROTO_LIBS): $(PLUGIN_SRC) $(TEST_PROTO_SRCS)
 	mkdir -p $(TEST_PROTO_DIR)
+
+	protoc\
+		--experimental_allow_proto3_optional\
+		--dart_out="disable_constructor_args:$(TEST_PROTO_DIR)"\
+		-Iprotos\
+		-I$(TEST_PROTO_SRC_DIR)\
+		--plugin=protoc-gen-dart=$(realpath $(PLUGIN_PATH))\
+		$(TEST_PROTO_SRCS)
+
+	mkdir -p $(TEST_PROTO_DIR)/constructor_args
+
 	protoc\
 		--experimental_allow_proto3_optional\
-		--dart_out=$(TEST_PROTO_DIR)\
+		--dart_out="$(TEST_PROTO_DIR)/constructor_args"\
 		-Iprotos\
 		-I$(TEST_PROTO_SRC_DIR)\
 		--plugin=protoc-gen-dart=$(realpath $(PLUGIN_PATH))\
 		$(TEST_PROTO_SRCS)
+
 	dart format $(TEST_PROTO_DIR)
 
 update-pregenerated: $(PLUGIN_PATH) $(PREGENERATED_SRCS)
@@ -97,11 +109,11 @@ update-pregenerated: $(PLUGIN_PATH) $(PREGENERATED_SRCS)
 protos: $(PLUGIN_PATH) $(TEST_PROTO_LIBS)
 
 run-tests: protos
-	dart run test
+	dart test
 
 update-goldens: protos
 	rm -rf test/goldens
-	dart run test
+	dart test
 
 clean:
 	rm -rf $(OUTPUT_DIR)
diff --git a/protoc_plugin/lib/src/base_type.dart b/protoc_plugin/lib/src/base_type.dart
@@ -56,6 +56,9 @@ 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
@@ -330,7 +330,8 @@ class MessageGenerator extends ProtobufContainer {
       NamedLocation(
           name: classname, fieldPathSegment: fieldPath, start: 'class '.length)
     ], () {
-      out.println('factory $classname() => create();');
+      _generateFactory(out);
+
       out.printlnAnnotated('$classname._() : super();', [
         NamedLocation(name: classname, fieldPathSegment: fieldPath, start: 0)
       ]);
@@ -430,6 +431,42 @@ class MessageGenerator extends ProtobufContainer {
     out.println();
   }
 
+  void _generateFactory(IndentingWriter out) {
+    if (!fileGen.options.disableConstructorArgs && _fieldList.isNotEmpty) {
+      out.println('factory $classname({');
+      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.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('factory $classname() => create();');
+    }
+  }
+
   // Returns true if the message type has any required fields.  If it doesn't,
   // we can optimize out calls to its isInitialized()/_findInvalidFields()
   // methods.

diff --git a/protoc_plugin/lib/src/options.dart b/protoc_plugin/lib/src/options.dart
@@ -50,11 +50,12 @@ bool genericOptionsParser(CodeGeneratorRequest request,
 class GenerationOptions {
   final bool useGrpc;
   final bool generateMetadata;
+  final bool disableConstructorArgs;
 
-  GenerationOptions({
-    this.useGrpc = false,
-    this.generateMetadata = false,
-  });
+  GenerationOptions(
+      {this.useGrpc = false,
+      this.generateMetadata = false,
+      this.disableConstructorArgs = false});
 }
 
 /// A parser for a name-value pair option. Options parsed in
@@ -87,13 +88,26 @@ class GenerateMetadataParser implements SingleOptionParser {
   @override
   void parse(String name, String? value, OnError onError) {
     if (value != null) {
-      onError('Invalid metadata option. No value expected.');
+      onError('Invalid generate_kythe_info option. No value expected.');
       return;
     }
     generateKytheInfo = true;
   }
 }
 
+class DisableConstructorArgsParser implements SingleOptionParser {
+  bool value = false;
+
+  @override
+  void parse(String name, String? value, OnError onError) {
+    if (value != null) {
+      onError('Invalid disable_constructor_args option. No value expected.');
+      return;
+    }
+    this.value = true;
+  }
+}
+
 /// Parser used by the compiler, which supports the `rpc` option (see
 /// [GrpcOptionParser]) and any additional option added in [parsers]. If
 /// [parsers] has a key for `rpc`, it will be ignored.
@@ -105,14 +119,18 @@ GenerationOptions? parseGenerationOptions(
 
   final grpcOptionParser = GrpcOptionParser();
   newParsers['grpc'] = grpcOptionParser;
+
   final generateMetadataParser = GenerateMetadataParser();
   newParsers['generate_kythe_info'] = generateMetadataParser;
 
+  final disableConstructorArgsParser = DisableConstructorArgsParser();
+  newParsers['disable_constructor_args'] = disableConstructorArgsParser;
+
   if (genericOptionsParser(request, response, newParsers)) {
     return GenerationOptions(
-      useGrpc: grpcOptionParser.grpcEnabled,
-      generateMetadata: generateMetadataParser.generateKytheInfo,
-    );
+        useGrpc: grpcOptionParser.grpcEnabled,
+        generateMetadata: generateMetadataParser.generateKytheInfo,
+        disableConstructorArgs: disableConstructorArgsParser.value);
   }
   return null;
 }
diff --git a/protoc_plugin/test/file_generator_test.dart b/protoc_plugin/test/file_generator_test.dart
@@ -93,7 +93,8 @@ void main() {
       () {
     final fd = buildFileDescriptor();
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
     expectMatchesGoldenFile(
@@ -103,7 +104,8 @@ void main() {
   test('FileGenerator outputs a .pb.dart file for an Int64 message', () {
     final fd = createInt64Proto();
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
     expectMatchesGoldenFile(
@@ -115,7 +117,8 @@ void main() {
       () {
     final fd = buildFileDescriptor();
     final options = parseGenerationOptions(
-        CodeGeneratorRequest()..parameter = 'generate_kythe_info',
+        CodeGeneratorRequest()
+          ..parameter = 'generate_kythe_info,disable_constructor_args',
         CodeGeneratorResponse())!;
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -127,7 +130,8 @@ void main() {
       () {
     final fd = buildFileDescriptor();
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
     expectMatchesGoldenFile(
@@ -137,7 +141,8 @@ void main() {
   test('FileGenerator generates files for a top-level enum', () {
     final fd = buildFileDescriptor(phoneNumber: false, topLevelEnum: true);
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
 
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -150,7 +155,8 @@ void main() {
   test('FileGenerator generates metadata files for a top-level enum', () {
     final fd = buildFileDescriptor(phoneNumber: false, topLevelEnum: true);
     final options = parseGenerationOptions(
-        CodeGeneratorRequest()..parameter = 'generate_kythe_info',
+        CodeGeneratorRequest()
+          ..parameter = 'generate_kythe_info,disable_constructor_args',
         CodeGeneratorResponse())!;
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -164,7 +170,8 @@ void main() {
   test('FileGenerator generates a .pbjson.dart file for a top-level enum', () {
     final fd = buildFileDescriptor(phoneNumber: false, topLevelEnum: true);
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
 
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -176,7 +183,8 @@ void main() {
     final fd = buildFileDescriptor();
     fd.package = 'pb_library';
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
 
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -201,7 +209,8 @@ void main() {
         ]));
 
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
 
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -228,7 +237,8 @@ void main() {
       ..service.add(sd);
 
     final options = parseGenerationOptions(
-        CodeGeneratorRequest(), CodeGeneratorResponse())!;
+        CodeGeneratorRequest()..parameter = 'disable_constructor_args',
+        CodeGeneratorResponse())!;
 
     final fg = FileGenerator(fd, options);
     link(options, [fg]);
@@ -415,7 +425,8 @@ void main() {
       ..name = 'test.proto'
       ..messageType.add(md);
     fd.dependency.addAll(['package1.proto', 'package2.proto']);
-    final request = CodeGeneratorRequest();
+    final request = CodeGeneratorRequest()
+      ..parameter = 'disable_constructor_args';
     final response = CodeGeneratorResponse();
     final options = parseGenerationOptions(request, response)!;