Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Aug 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fc7b5a8
Add a flag to generate constructor arguments
osa1 Jun 27, 2023
3b88cf4
Remove underscore in function local
osa1 Jun 27, 2023
a5d337b
Simplify factory generator
osa1 Jun 27, 2023
b45da9c
Merge branch 'master' into constructor_args
osa1 Jun 29, 2023
35401e8
Merge branch 'master' into constructor_args
osa1 Jul 3, 2023
3bec9ae
Merge branch 'master' into constructor_args
osa1 Jul 10, 2023
d655897
Merge remote-tracking branch 'origin/master' into constructor_args
osa1 Jul 21, 2023
491bda6
Start building test protos with constructor args, bring back tests
osa1 Jul 26, 2023
ce857b8
Test new message with old assertAllFieldsSet by converting it to the …
osa1 Jul 26, 2023
32249f0
Simplify dart test command, revert whitespace in Makefile
osa1 Jul 26, 2023
16b583f
Merge branch 'master' into constructor_args
osa1 Aug 3, 2023
b1c48b3
Rename flag to constructor_args
osa1 Aug 14, 2023
5dd3621
Formatting
osa1 Aug 14, 2023
a24f917
Generate constructor args by default
osa1 Aug 14, 2023
5621bcc
Reword changelog
osa1 Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions protoc_plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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` added to disable 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

Expand Down
18 changes: 15 additions & 3 deletions protoc_plugin/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
3 changes: 3 additions & 0 deletions protoc_plugin/lib/src/base_type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
39 changes: 38 additions & 1 deletion protoc_plugin/lib/src/message_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
]);
Expand Down Expand Up @@ -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();');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this local variable (result) can conflict with named params corresponding to a proto field.

For dartpad's protos code, we're seeing:

A value of type 'CompileResponse' can't be assigned to a variable of type 'String'.
Try changing the type of the variable, or casting the right-hand type to 'String'.

Perhaps name this _result? $result?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our proto definition:

message CompileResponse {
  string result = 1;
  string sourceMap = 2;

  // Make this response compatible with BadRequest
  ErrorMessage error = 99;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#869

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.
Expand Down
34 changes: 26 additions & 8 deletions protoc_plugin/lib/src/options.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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;
}
33 changes: 22 additions & 11 deletions protoc_plugin/test/file_generator_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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]);
Expand All @@ -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(
Expand All @@ -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]);
Expand All @@ -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]);
Expand All @@ -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]);
Expand All @@ -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]);
Expand All @@ -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]);
Expand All @@ -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]);
Expand Down Expand Up @@ -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)!;

Expand Down
Loading