Generate docs of enums and rpc clients, some refactoring (#909)

This implements generating doc comments for enum types, enum values, rpc types, and rpc methods. Existing code for generating doc comments is refactored: the comment generator handles trailing whitespace so we don't have to handle it in the call sites. Comment generator is documented with the invariants: it never returns an empty string (returns `null` instead) and never returns lines with trailing whitespace. To be able to test comment generating, a new kind of golden file test is added. The difference from the other golden file tests is that this file is compiled from .proto source instead of from a hand-crafted descriptor. This is because attaching comments to proto field and types by hand is difficult and error prone. Fixes the comment part of #900.
google · Jan 8, 2024 · 9a408a7 · 9a408a7
1 parent c4fd596
commit 9a408a7
Show file tree

Hide file tree

Showing 11 changed files with 315 additions and 30 deletions.
diff --git a/protoc_plugin/CHANGELOG.md b/protoc_plugin/CHANGELOG.md
@@ -7,11 +7,15 @@
   This change requires protobuf-4.0.0.
 * Generated files now export `GeneratedMessageGenericExtensions` from the
   protobuf library. ([#503], [#907])
+* Generate doc comments for enum types and values, rpc services and methods.
+  ([#900], [#909])
 
 [#738]: https://github.com/google/protobuf.dart/issues/738
 [#903]: https://github.com/google/protobuf.dart/pull/903
 [#503]: https://github.com/google/protobuf.dart/issues/503
 [#907]: https://github.com/google/protobuf.dart/pull/907
+[#900]: https://github.com/google/protobuf.dart/issues/900
+[#909]: https://github.com/google/protobuf.dart/pull/909
 
 ## 21.1.2
 

diff --git a/protoc_plugin/Makefile b/protoc_plugin/Makefile
@@ -24,6 +24,7 @@ TEST_PROTO_LIST = \
 	google/protobuf/wrappers \
 	custom_option \
 	dart_name \
+	doc_comments \
         default_value_escape \
 	entity \
 	enum_extension \

diff --git a/protoc_plugin/lib/src/client_generator.dart b/protoc_plugin/lib/src/client_generator.dart
@@ -10,7 +10,29 @@ class ClientApiGenerator {
   final String className;
   final Set<String> usedMethodNames = {...reservedMemberNames};
 
-  ClientApiGenerator(this.service, Set<String> usedNames)
+  /// Tag of `FileDescriptorProto.service`.
+  static const _fileDescriptorServiceTag = 6;
+
+  /// Tag of `ServiceDescriptorProto.method`.
+  static const _serviceDescriptorMethodTag = 2;
+
+  /// Index of the service in `FileDescriptorProto.service` repeated field.
+  final int _repeatedFieldIndex;
+
+  List<int> get _serviceDescriptorPath => [
+        ...service.fileGen.fieldPath,
+        _fileDescriptorServiceTag,
+        _repeatedFieldIndex
+      ];
+
+  List<int> _methodDescriptorPath(int methodRepeatedFieldIndex) => [
+        ..._serviceDescriptorPath,
+        _serviceDescriptorMethodTag,
+        methodRepeatedFieldIndex
+      ];
+
+  ClientApiGenerator(
+      this.service, Set<String> usedNames, this._repeatedFieldIndex)
       : className = disambiguateName(
             avoidInitialUnderscore(service._descriptor.name),
             usedNames,
@@ -20,27 +42,37 @@ class ClientApiGenerator {
   String get _clientType => '$protobufImportPrefix.RpcClient';
 
   void generate(IndentingWriter out) {
+    final commentBlock = service.fileGen.commentBlock(_serviceDescriptorPath);
+    if (commentBlock != null) {
+      out.println(commentBlock);
+    }
     out.addBlock('class ${className}Api {', '}', () {
       out.println('$_clientType _client;');
       out.println('${className}Api(this._client);');
       out.println();
 
-      for (final m in service._descriptor.method) {
-        generateMethod(out, m);
+      for (var i = 0; i < service._descriptor.method.length; i++) {
+        generateMethod(out, service._descriptor.method[i], i);
       }
     });
     out.println();
   }
 
   // Subclasses can override this.
-  void generateMethod(IndentingWriter out, MethodDescriptorProto m) {
+  void generateMethod(IndentingWriter out, MethodDescriptorProto m,
+      int methodRepeatedFieldIndex) {
     final methodName = disambiguateName(
         avoidInitialUnderscore(service._methodName(m.name)),
         usedMethodNames,
         defaultSuffixes());
     final inputType = service._getDartClassName(m.inputType, forMainFile: true);
     final outputType =
         service._getDartClassName(m.outputType, forMainFile: true);
+    final commentBlock = service.fileGen
+        .commentBlock(_methodDescriptorPath(methodRepeatedFieldIndex));
+    if (commentBlock != null) {
+      out.println(commentBlock);
+    }
     out.addBlock(
         '$asyncImportPrefix.Future<$outputType> $methodName('
             '$protobufImportPrefix.ClientContext? ctx, $inputType request) =>',

diff --git a/protoc_plugin/lib/src/enum_generator.dart b/protoc_plugin/lib/src/enum_generator.dart
@@ -12,10 +12,10 @@ class EnumAlias {
 
 class EnumGenerator extends ProtobufContainer {
   @override
-  final ProtobufContainer? parent;
+  final ProtobufContainer parent;
 
   @override
-  final String? classname;
+  final String classname;
 
   @override
   final String fullName;
@@ -32,16 +32,15 @@ class EnumGenerator extends ProtobufContainer {
   List<int>? _fieldPath;
   final List<int> _fieldPathSegment;
 
-  /// See [[ProtobufContainer]
   @override
-  List<int>? get fieldPath =>
-      _fieldPath ??= List.from(parent!.fieldPath!)..addAll(_fieldPathSegment);
+  List<int> get fieldPath =>
+      _fieldPath ??= List.from(parent.fieldPath!)..addAll(_fieldPathSegment);
 
   EnumGenerator._(EnumDescriptorProto descriptor, this.parent,
       Set<String> usedClassNames, int repeatedFieldIndex, int fieldIdTag)
       : _fieldPathSegment = [fieldIdTag, repeatedFieldIndex],
         classname = messageOrEnumClassName(descriptor.name, usedClassNames,
-            parent: parent!.classname ?? ''),
+            parent: parent.classname ?? ''),
         fullName = parent.fullName == ''
             ? descriptor.name
             : '${parent.fullName}.${descriptor.name}',
@@ -80,10 +79,10 @@ class EnumGenerator extends ProtobufContainer {
             _nestedFieldTag);
 
   @override
-  String get package => parent!.package;
+  String get package => parent.package;
 
   @override
-  FileGenerator? get fileGen => parent!.fileGen;
+  FileGenerator? get fileGen => parent.fileGen;
 
   /// Make this enum available as a field type.
   void register(GenerationContext ctx) {
@@ -103,17 +102,15 @@ class EnumGenerator extends ProtobufContainer {
   static const int _enumValueTag = 2;
 
   void generate(IndentingWriter out) {
-    final comment = fileGen?.commentBlock(fieldPath!);
-    if (comment != null) {
-      out.println(comment);
+    final commentBlock = fileGen?.commentBlock(fieldPath);
+    if (commentBlock != null) {
+      out.println(commentBlock);
     }
     out.addAnnotatedBlock(
         'class $classname extends $protobufImportPrefix.ProtobufEnum {',
         '}\n', [
       NamedLocation(
-          name: classname!,
-          fieldPathSegment: fieldPath!,
-          start: 'class '.length)
+          name: classname, fieldPathSegment: fieldPath, start: 'class '.length)
     ], () {
       // -----------------------------------------------------------------
       // Define enum types.
@@ -124,14 +121,21 @@ class EnumGenerator extends ProtobufContainer {
         out.addSuffix(
             omitEnumNames.constFieldName, omitEnumNames.constDefinition);
         final conditionalValName = omitEnumNames.createTernary(val.name);
+        final fieldPathSegment = List<int>.from(fieldPath)
+          ..addAll([_enumValueTag, _originalCanonicalIndices[i]]);
+
+        final commentBlock = fileGen?.commentBlock(fieldPathSegment);
+        if (commentBlock != null) {
+          out.println(commentBlock);
+        }
+
         out.printlnAnnotated(
             'static const $classname $name = '
             '$classname._(${val.number}, $conditionalValName);',
             [
               NamedLocation(
                   name: name,
-                  fieldPathSegment: List.from(fieldPath!)
-                    ..addAll([_enumValueTag, _originalCanonicalIndices[i]]),
+                  fieldPathSegment: fieldPathSegment,
                   start: 'static const $classname '.length)
             ]);
       }
@@ -146,7 +150,7 @@ class EnumGenerator extends ProtobufContainer {
               [
                 NamedLocation(
                     name: name,
-                    fieldPathSegment: List.from(fieldPath!)
+                    fieldPathSegment: List.from(fieldPath)
                       ..addAll([_enumValueTag, _originalAliasIndices[i]]),
                     start: 'static const $classname '.length)
               ]);

diff --git a/protoc_plugin/lib/src/file_generator.dart b/protoc_plugin/lib/src/file_generator.dart
@@ -173,15 +173,16 @@ class FileGenerator extends ProtobufContainer {
       extensionGenerators.add(ExtensionGenerator.topLevel(
           descriptor.extension[i], this, usedExtensionNames, i));
     }
-    for (final service in descriptor.service) {
+    for (var i = 0; i < descriptor.service.length; i++) {
+      final service = descriptor.service[i];
       if (options.useGrpc) {
         grpcGenerators.add(GrpcServiceGenerator(service, this));
       } else {
         final serviceGen =
             ServiceGenerator(service, this, usedTopLevelServiceNames);
         serviceGenerators.add(serviceGen);
         clientApiGenerators
-            .add(ClientApiGenerator(serviceGen, usedTopLevelNames));
+            .add(ClientApiGenerator(serviceGen, usedTopLevelNames, i));
       }
     }
   }

diff --git a/protoc_plugin/lib/src/message_generator.dart b/protoc_plugin/lib/src/message_generator.dart
@@ -67,7 +67,6 @@ class MessageGenerator extends ProtobufContainer {
 
   final List<int> _fieldPathSegment;
 
-  /// See [[ProtobufContainer]
   @override
   late final List<int> fieldPath = List.from(parent!.fieldPath!)
     ..addAll(_fieldPathSegment);
@@ -118,8 +117,13 @@ class MessageGenerator extends ProtobufContainer {
     }
   }
 
+  /// Tag of `FileDescriptorProto.message_type`.
   static const _topLevelMessageTag = 4;
+
+  /// Tag of `DescriptorProto.nested_type`.
   static const _nestedMessageTag = 3;
+
+  /// Tag of `DescriptorProto.field`.
   static const _messageFieldTag = 2;
 
   MessageGenerator.topLevel(
@@ -319,13 +323,12 @@ class MessageGenerator extends ProtobufContainer {
       extendedClass = 'GeneratedMessage';
     }
 
-    var commentBlock = fileGen.commentBlock(fieldPath) ?? '';
-    if (commentBlock.isNotEmpty) {
-      commentBlock = '$commentBlock\n';
+    final commentBlock = fileGen.commentBlock(fieldPath);
+    if (commentBlock != null) {
+      out.println(commentBlock);
     }
-
     out.addAnnotatedBlock(
-        '${commentBlock}class $classname extends $protobufImportPrefix.$extendedClass$mixinClause {',
+        'class $classname extends $protobufImportPrefix.$extendedClass$mixinClause {',
         '}', [
       NamedLocation(
           name: classname, fieldPathSegment: fieldPath, start: 'class '.length)
@@ -539,7 +542,7 @@ class MessageGenerator extends ProtobufContainer {
 
     final commentBlock = fileGen.commentBlock(memberFieldPath);
     if (commentBlock != null) {
-      out.println(commentBlock.trim());
+      out.println(commentBlock);
     }
 
     _emitDeprecatedIf(field.isDeprecated, out);

diff --git a/protoc_plugin/lib/src/shared.dart b/protoc_plugin/lib/src/shared.dart
@@ -14,6 +14,14 @@ const grpcImportPrefix = r'$grpc';
 const mixinImportPrefix = r'$mixin';
 
 extension FileDescriptorProtoExt on FileGenerator {
+  /// Convert leading comments of a definition at [path] to Dart doc comment
+  /// syntax.
+  ///
+  /// This never returns an empty string: if the comment is empty it returns
+  /// `null`.
+  ///
+  /// The output can contain multiple lines. None of the lines will have
+  /// trailing whitespace.
   String? commentBlock(List<int> path) {
     final bits = descriptor.sourceCodeInfo.location
         .where((element) => element.path.toString() == path.toString())
@@ -33,6 +41,10 @@ extension FileDescriptorProtoExt on FileGenerator {
   }
 }
 
+/// Convert a comment to Dart doc comment syntax.
+///
+/// This is the internal method for [FileDescriptorProtoExt.commentBlock],
+/// public to be able to test.
 String? toDartComment(String value) {
   if (value.isEmpty) return null;
 

diff --git a/protoc_plugin/test/doc_comments_test.dart b/protoc_plugin/test/doc_comments_test.dart
@@ -0,0 +1,22 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:io';
+
+import 'package:test/test.dart';
+
+import 'golden_file.dart';
+
+void main() {
+  test('Doc comment generation for messages', () {
+    final actual = File('out/protos/doc_comments.pb.dart').readAsStringSync();
+    expectMatchesGoldenFile(actual, 'test/goldens/doc_comments');
+  });
+
+  test('Doc comment generation for enums', () {
+    final actual = File('out/protos/constructor_args/doc_comments.pbenum.dart')
+        .readAsStringSync();
+    expectMatchesGoldenFile(actual, 'test/goldens/doc_comments.pbenum');
+  });
+}