Skip to content

Commit a1dceed

Browse files
authored
protoc_plugin refactors (#1082)
- Move static method `FileGenerator._getDeclaredMixins` to the end of the file as a top-level function. This is a utility method but it came first in the class and it's quite large (~90 lines). Move it to the end so that it will allow top-down reading of the class and won't obstruct more important details like fields. - Refactor import prefix generation: currently if I'm importing `container` in `file`, I need to do `container.importPrefix(context: file)`. This is confusing and it took me a while to figure out what it's doing: It's a method on the container type but it modifies the file. Files can import, not containers. So files should have import prefixes, not containers. Move the method to `FileGenerator`. The code above now looks like `file.importPrefix(container)`. Also document the fact that just generating the import prefix for an imported thing does not make it imported automatically. This is quite error prone, if I call `file.importPrefix(container)` and use the prefix in e.g. `EnumGenerator`, I need to make sure to update `FileGenerator` to actually generate the import. It would be good to refactor this so that just using an prefix is enough to import it. But for now this part is not done. - Minor changes: use spread syntax instead of `[]..addAll(...)` and similar. Add one line space between some of the fields.
1 parent 46c3697 commit a1dceed

File tree

8 files changed

+133
-131
lines changed

8 files changed

+133
-131
lines changed

protoc_plugin/lib/names.dart

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -262,11 +262,11 @@ String messageOrEnumClassName(
262262

263263
/// Returns the set of names reserved by the ProtobufEnum class and its
264264
/// generated subclasses.
265-
Set<String> get reservedEnumNames =>
266-
<String>{}
267-
..addAll(ProtobufEnum_reservedNames)
268-
..addAll(_dartReservedWords)
269-
..addAll(_protobufEnumNames);
265+
final Set<String> reservedEnumNames = <String>{
266+
...ProtobufEnum_reservedNames,
267+
..._dartReservedWords,
268+
..._protobufEnumNames,
269+
};
270270

271271
Iterable<String> enumSuffixes() sync* {
272272
var s = '_';

protoc_plugin/lib/src/base_type.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class BaseType {
4646
String prefixed(FileGenerator fileGen) =>
4747
generator == null
4848
? unprefixed
49-
: '${generator!.importPrefix(context: fileGen)}.$unprefixed';
49+
: '${fileGen.importPrefix(generator!)}.$unprefixed';
5050

5151
/// Returns the name to use in generated code for this Dart type.
5252
///

protoc_plugin/lib/src/code_generator.dart

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,6 @@ import 'options.dart';
2525
import 'output_config.dart';
2626

2727
abstract class ProtobufContainer {
28-
// A map of proto file paths to import prefix aliases.
29-
final Map<String, String> _prefixes = {};
30-
3128
String get package;
3229
String? get classname;
3330
String get fullName;
@@ -48,14 +45,6 @@ abstract class ProtobufContainer {
4845
String get binaryDescriptorName =>
4946
'${lowerCaseFirstLetter(classname!)}Descriptor';
5047

51-
String importPrefix({required FileGenerator context}) {
52-
final protoFilePath = fileGen!.protoFileUri.toString();
53-
return context._calculateImportPrefix(protoFilePath);
54-
}
55-
56-
String _calculateImportPrefix(String protoImportPath) =>
57-
_prefixes.putIfAbsent(protoImportPath, () => '\$${_prefixes.length}');
58-
5948
/// The generator of the .pb.dart file defining this entity.
6049
///
6150
/// (Represents the .pb.dart file that we need to import in order to use it.)
@@ -69,9 +58,6 @@ abstract class ProtobufContainer {
6958
/// The top-level parent of this entity, or itself if it is a top-level
7059
/// entity.
7160
ProtobufContainer? get toplevelParent {
72-
if (parent == null) {
73-
return null;
74-
}
7561
if (parent is FileGenerator) {
7662
return this;
7763
}
@@ -87,9 +73,9 @@ class CodeGenerator {
8773

8874
/// Runs the code generator. The optional [optionParsers] can be used to
8975
/// change how command line options are parsed (see [parseGenerationOptions]
90-
/// for details), and [config] can be used to override where
91-
/// generated files are created and how imports between generated files are
92-
/// constructed (see [OutputConfiguration] for details).
76+
/// for details), and [config] can be used to override where generated files
77+
/// are created and how imports between generated files are constructed (see
78+
/// [OutputConfiguration] for details).
9379
Future<void> generate({
9480
Map<String, SingleOptionParser>? optionParsers,
9581
OutputConfiguration config = const DefaultOutputConfiguration(),

protoc_plugin/lib/src/enum_generator.dart

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ class EnumAlias {
1111
}
1212

1313
class EnumGenerator extends ProtobufContainer {
14+
/// For top-level enums: [FileGenerator]. For nested enums:
15+
/// [MessageGenerator].
1416
@override
1517
final ProtobufContainer parent;
1618

@@ -24,14 +26,19 @@ class EnumGenerator extends ProtobufContainer {
2426
final String fullName;
2527

2628
final EnumDescriptorProto _descriptor;
29+
2730
final List<EnumValueDescriptorProto> _canonicalValues =
2831
<EnumValueDescriptorProto>[];
32+
2933
final List<int> _originalCanonicalIndices = <int>[];
34+
3035
final List<EnumAlias> _aliases = <EnumAlias>[];
3136

3237
/// Maps the name of an enum value to the Dart name we will use for it.
3338
final Map<String, String> dartNames = <String, String>{};
39+
3440
final List<int> _originalAliasIndices = <int>[];
41+
3542
final List<int> _fieldPathSegment;
3643

3744
@override
@@ -124,8 +131,7 @@ class EnumGenerator extends ProtobufContainer {
124131
if (context.protoFileUri == fileGen!.protoFileUri) {
125132
return name;
126133
}
127-
128-
return '${importPrefix(context: context)}.$name';
134+
return '${context.importPrefix(this)}.$name';
129135
}
130136

131137
static const int _enumValueTag = 2;
@@ -160,8 +166,11 @@ class EnumGenerator extends ProtobufContainer {
160166
omitEnumNames.constDefinition,
161167
);
162168
final conditionalValName = omitEnumNames.createTernary(val.name);
163-
final fieldPathSegment = List<int>.from(fieldPath)
164-
..addAll([_enumValueTag, _originalCanonicalIndices[i]]);
169+
final fieldPathSegment = <int>[
170+
...fieldPath,
171+
_enumValueTag,
172+
_originalCanonicalIndices[i],
173+
];
165174

166175
final commentBlock = fileGen?.commentBlock(fieldPathSegment);
167176
if (commentBlock != null) {
@@ -263,7 +272,7 @@ class EnumGenerator extends ProtobufContainer {
263272
);
264273
}
265274

266-
/// Writes a Dart constant containing the JSON for the EnumProtoDescriptor.
275+
/// Writes a Dart constant containing the JSON for the [EnumDescriptorProto].
267276
void generateConstants(IndentingWriter out) {
268277
final name = getJsonConstant(fileGen!);
269278
final json = _descriptor.writeToJsonMap();

protoc_plugin/lib/src/file_generator.dart

Lines changed: 106 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -22,93 +22,6 @@ const String _typedDataImportUrl = 'dart:typed_data';
2222
///
2323
/// Outputs include .pb.dart, pbenum.dart, and .pbjson.dart.
2424
class FileGenerator extends ProtobufContainer {
25-
/// Reads and the declared mixins in the file, keyed by name.
26-
///
27-
/// Performs some basic validation on declared mixins, e.g. whether names
28-
/// are valid dart identifiers and whether there are cycles in the `parent`
29-
/// hierarchy.
30-
/// Does not check for existence of import files or classes.
31-
static Map<String, PbMixin> _getDeclaredMixins(FileDescriptorProto desc) {
32-
String mixinError(String error) =>
33-
'Option "mixins" in ${desc.name}: $error';
34-
35-
if (!desc.hasOptions() ||
36-
!desc.options.hasExtension(Dart_options.imports)) {
37-
return <String, PbMixin>{};
38-
}
39-
final dartMixins = <String, DartMixin>{};
40-
final importedMixins =
41-
desc.options.getExtension(Dart_options.imports) as Imports;
42-
for (final mixin in importedMixins.mixins) {
43-
if (dartMixins.containsKey(mixin.name)) {
44-
throw mixinError('Duplicate mixin name: "${mixin.name}"');
45-
}
46-
if (!mixin.name.startsWith(_dartIdentifier)) {
47-
throw mixinError(
48-
'"${mixin.name}" is not a valid dart class identifier',
49-
);
50-
}
51-
if (mixin.hasParent() && !mixin.parent.startsWith(_dartIdentifier)) {
52-
throw mixinError(
53-
'Mixin parent "${mixin.parent}" of "${mixin.name}" is '
54-
'not a valid dart class identifier',
55-
);
56-
}
57-
dartMixins[mixin.name] = mixin;
58-
}
59-
60-
// Detect cycles and unknown parents.
61-
for (final mixin in dartMixins.values) {
62-
if (!mixin.hasParent()) continue;
63-
var currentMixin = mixin;
64-
final parentChain = <String>[];
65-
while (currentMixin.hasParent()) {
66-
final parentName = currentMixin.parent;
67-
68-
final declaredMixin = dartMixins.containsKey(parentName);
69-
final internalMixin = !declaredMixin && findMixin(parentName) != null;
70-
71-
if (internalMixin) break; // No further validation of parent chain.
72-
73-
if (!declaredMixin) {
74-
throw mixinError(
75-
'Unknown mixin parent "${mixin.parent}" of '
76-
'"${currentMixin.name}"',
77-
);
78-
}
79-
80-
if (parentChain.contains(parentName)) {
81-
final cycle = '${parentChain.join('->')}->$parentName';
82-
throw mixinError('Cycle in parent chain: $cycle');
83-
}
84-
parentChain.add(parentName);
85-
currentMixin = dartMixins[parentName]!;
86-
}
87-
}
88-
89-
// Turn DartMixins into PbMixins.
90-
final pbMixins = <String, PbMixin>{};
91-
PbMixin? resolveMixin(String name) {
92-
if (pbMixins.containsKey(name)) return pbMixins[name];
93-
if (dartMixins.containsKey(name)) {
94-
final dartMixin = dartMixins[name]!;
95-
final pbMixin = PbMixin(
96-
dartMixin.name,
97-
importFrom: dartMixin.importFrom,
98-
parent: resolveMixin(dartMixin.parent),
99-
);
100-
pbMixins[name] = pbMixin;
101-
return pbMixin;
102-
}
103-
return findMixin(name);
104-
}
105-
106-
for (final mixin in dartMixins.values) {
107-
resolveMixin(mixin.name);
108-
}
109-
return pbMixins;
110-
}
111-
11225
final FileDescriptorProto descriptor;
11326
final GenerationOptions options;
11427

@@ -124,16 +37,15 @@ class FileGenerator extends ProtobufContainer {
12437

12538
/// Used to avoid collisions after names have been mangled to match the Dart
12639
/// style.
127-
final Set<String> usedTopLevelNames =
128-
<String>{}..addAll(forbiddenTopLevelNames);
40+
final Set<String> usedTopLevelNames = <String>{...forbiddenTopLevelNames};
12941

13042
/// Used to avoid collisions in the service file after names have been mangled
13143
/// to match the dart style.
132-
final Set<String> usedTopLevelServiceNames =
133-
<String>{}..addAll(forbiddenTopLevelNames);
44+
final Set<String> usedTopLevelServiceNames = <String>{
45+
...forbiddenTopLevelNames,
46+
};
13447

135-
final Set<String> usedExtensionNames =
136-
<String>{}..addAll(forbiddenExtensionNames);
48+
final Set<String> usedExtensionNames = <String>{...forbiddenExtensionNames};
13749

13850
/// Whether cross-references have been resolved.
13951
bool _linked = false;
@@ -143,6 +55,23 @@ class FileGenerator extends ProtobufContainer {
14355
@override
14456
final FeatureSet features;
14557

58+
/// Maps imports in the current file to their import prefixes.
59+
/// E.g. in `import 'x/y/z.pb.dart' as $1` this maps `x/y/z.pb.dart` to `$1`.
60+
final Map<String, String> _importPrefixes = {};
61+
62+
/// Get the import prefix of `container` in the current file generator.
63+
///
64+
/// Note that just calling this does not import the `container` in the current
65+
/// file. This just assigns an prefix to the container in the current file
66+
/// generator.
67+
String importPrefix(ProtobufContainer container) {
68+
final protoFilePath = container.fileGen!.protoFileUri.toString();
69+
return _importPrefixes.putIfAbsent(
70+
protoFilePath,
71+
() => '\$${_importPrefixes.length}',
72+
);
73+
}
74+
14675
FileGenerator(
14776
FeatureSetDefaults editionDefaults,
14877
this.descriptor,
@@ -781,10 +710,7 @@ class FileGenerator extends ProtobufContainer {
781710
// evaluate to true not just for the main .pb.dart file based off the proto
782711
// file, but also for the .pbserver.dart, .pbgrpc.dart files.
783712
if (ext == '.pb.dart' || protoFileUri != target.protoFileUri) {
784-
importWriter.addImport(
785-
import,
786-
prefix: target.importPrefix(context: fileGen),
787-
);
713+
importWriter.addImport(import, prefix: fileGen.importPrefix(target));
788714
} else {
789715
importWriter.addImport(import);
790716
}
@@ -875,6 +801,89 @@ FeatureSet _getEditionDefaults(
875801
return defaults;
876802
}
877803

804+
/// Reads and the declared mixins in the file, keyed by name.
805+
///
806+
/// Performs some basic validation on declared mixins, e.g. whether names
807+
/// are valid dart identifiers and whether there are cycles in the `parent`
808+
/// hierarchy.
809+
/// Does not check for existence of import files or classes.
810+
Map<String, PbMixin> _getDeclaredMixins(FileDescriptorProto desc) {
811+
String mixinError(String error) => 'Option "mixins" in ${desc.name}: $error';
812+
813+
if (!desc.hasOptions() || !desc.options.hasExtension(Dart_options.imports)) {
814+
return <String, PbMixin>{};
815+
}
816+
final dartMixins = <String, DartMixin>{};
817+
final importedMixins =
818+
desc.options.getExtension(Dart_options.imports) as Imports;
819+
for (final mixin in importedMixins.mixins) {
820+
if (dartMixins.containsKey(mixin.name)) {
821+
throw mixinError('Duplicate mixin name: "${mixin.name}"');
822+
}
823+
if (!mixin.name.startsWith(_dartIdentifier)) {
824+
throw mixinError('"${mixin.name}" is not a valid dart class identifier');
825+
}
826+
if (mixin.hasParent() && !mixin.parent.startsWith(_dartIdentifier)) {
827+
throw mixinError(
828+
'Mixin parent "${mixin.parent}" of "${mixin.name}" is '
829+
'not a valid dart class identifier',
830+
);
831+
}
832+
dartMixins[mixin.name] = mixin;
833+
}
834+
835+
// Detect cycles and unknown parents.
836+
for (final mixin in dartMixins.values) {
837+
if (!mixin.hasParent()) continue;
838+
var currentMixin = mixin;
839+
final parentChain = <String>[];
840+
while (currentMixin.hasParent()) {
841+
final parentName = currentMixin.parent;
842+
843+
final declaredMixin = dartMixins.containsKey(parentName);
844+
final internalMixin = !declaredMixin && findMixin(parentName) != null;
845+
846+
if (internalMixin) break; // No further validation of parent chain.
847+
848+
if (!declaredMixin) {
849+
throw mixinError(
850+
'Unknown mixin parent "${mixin.parent}" of '
851+
'"${currentMixin.name}"',
852+
);
853+
}
854+
855+
if (parentChain.contains(parentName)) {
856+
final cycle = '${parentChain.join('->')}->$parentName';
857+
throw mixinError('Cycle in parent chain: $cycle');
858+
}
859+
parentChain.add(parentName);
860+
currentMixin = dartMixins[parentName]!;
861+
}
862+
}
863+
864+
// Turn DartMixins into PbMixins.
865+
final pbMixins = <String, PbMixin>{};
866+
PbMixin? resolveMixin(String name) {
867+
if (pbMixins.containsKey(name)) return pbMixins[name];
868+
if (dartMixins.containsKey(name)) {
869+
final dartMixin = dartMixins[name]!;
870+
final pbMixin = PbMixin(
871+
dartMixin.name,
872+
importFrom: dartMixin.importFrom,
873+
parent: resolveMixin(dartMixin.parent),
874+
);
875+
pbMixins[name] = pbMixin;
876+
return pbMixin;
877+
}
878+
return findMixin(name);
879+
}
880+
881+
for (final mixin in dartMixins.values) {
882+
resolveMixin(mixin.name);
883+
}
884+
return pbMixins;
885+
}
886+
878887
const _fileIgnores = {
879888
'annotate_overrides',
880889
'camel_case_types',

protoc_plugin/lib/src/grpc_generator.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ class GrpcServiceGenerator {
105105
throw 'FAILURE: Unknown type reference ($fqName) for $location';
106106
}
107107

108-
return '${generator.importPrefix(context: fileGen)}.${generator.classname}';
108+
return '${fileGen.importPrefix(generator)}.${generator.classname}';
109109
}
110110

111111
void generate(IndentingWriter out) {

protoc_plugin/lib/src/message_generator.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ class MessageGenerator extends ProtobufContainer {
218218
if (context.protoFileUri == fileGen.protoFileUri) {
219219
return name;
220220
}
221-
return '${importPrefix(context: context)}.$name';
221+
return '${context.importPrefix(this)}.$name';
222222
}
223223

224224
/// Adds all mixins used in this message and any submessages.

0 commit comments

Comments
 (0)