Disambiguate names of service methods in grpc output

protoc_plugin/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 20.0.1
+
+* Handle name disambiguation for service methods to avoid clashes with 
+  existing names.
+
+  Now you can have a method called `New`. In dart the method name will be `new_`.
+
 ## 20.0.0
 
 * Stable release generating null-safe code.

protoc_plugin/lib/client_generator.dart

@@ -27,28 +27,26 @@ class ClientApiGenerator {
       out.println('${className}Api(this._client);');
       out.println();
 
-      for (var m in service._descriptor.method) {
+      for (var m in service.methods) {
         generateMethod(out, m);
       }
     });
     out.println();
   }
 
   // Subclasses can override this.
-  void generateMethod(IndentingWriter out, MethodDescriptorProto m) {
-    var methodName = disambiguateName(
-        avoidInitialUnderscore(service._methodName(m.name)),
-        usedMethodNames,
-        defaultSuffixes());
-    var inputType = service._getDartClassName(m.inputType, forMainFile: true);
-    var outputType = service._getDartClassName(m.outputType, forMainFile: true);
+  void generateMethod(IndentingWriter out, _ServiceMethod m) {
+    var inputType =
+        service._getDartClassName(m.descriptor.inputType, forMainFile: true);
+    var outputType =
+        service._getDartClassName(m.descriptor.outputType, forMainFile: true);
     out.addBlock(
-        '$_asyncImportPrefix.Future<$outputType> $methodName('
+        '$_asyncImportPrefix.Future<$outputType> ${m.dartName}('
             '$_protobufImportPrefix.ClientContext? ctx, $inputType request) {',
         '}', () {
       out.println('var emptyResponse = $outputType();');
       out.println('return _client.invoke<$outputType>(ctx, \'${className}\', '
-          '\'${m.name}\', request, emptyResponse);');
+          '\'${m.descriptor.name}\', request, emptyResponse);');
     });
   }
 }

protoc_plugin/lib/grpc_generator.dart

@@ -58,8 +58,9 @@ class GrpcServiceGenerator {
   /// in [_undefinedDeps].
   /// Precondition: messages have been registered and resolved.
   void resolve(GenerationContext ctx) {
+    final usedNames = <String>{...serviceReservedMemberNames};
     for (var method in _descriptor.method) {
-      _methods.add(_GrpcMethod(this, ctx, method));
+      _methods.add(_GrpcMethod(this, ctx, method, usedNames));
     }
   }
 
@@ -181,9 +182,13 @@ class _GrpcMethod {
       this._serverReturnType);
 
   factory _GrpcMethod(GrpcServiceGenerator service, GenerationContext ctx,
-      MethodDescriptorProto method) {
+      MethodDescriptorProto method, Set<String> usedNames) {
     final grpcName = method.name;
-    final dartName = lowerCaseFirstLetter(grpcName);
+    final dartName = disambiguateName(
+        avoidInitialUnderscore(lowerCaseFirstLetter(grpcName)),
+        usedNames,
+        defaultSuffixes(),
+        generateVariants: (name) => [name, '${name}_Pre']);
 
     final clientStreaming = method.clientStreaming;
     final serverStreaming = method.serverStreaming;

protoc_plugin/lib/names.dart

@@ -487,28 +487,39 @@ bool _isDartFieldName(name) => name.startsWith(_dartFieldNameExpr);
 final _dartFieldNameExpr = RegExp(r'^[a-z]\w+$');
 
 /// Names that would collide as top-level identifiers.
-final List<String> forbiddenTopLevelNames = <String>[
+const forbiddenTopLevelNames = <String>[
   'List',
   'Function',
   'Map',
   ..._dartReservedWords,
 ];
 
-final List<String> reservedMemberNames = <String>[
+const reservedMemberNames = <String>[
   ..._dartReservedWords,
   ...GeneratedMessage_reservedNames,
   ..._generatedMessageNames
 ];
 
-final List<String> forbiddenExtensionNames = <String>[
+const forbiddenExtensionNames = <String>[
   ..._dartReservedWords,
   ...GeneratedMessage_reservedNames,
   ..._generatedMessageNames
 ];
 
+const serviceReservedMemberNames = <String>[
+  ..._dartReservedWords,
+  ..._serviceNames,
+];
+
+const _serviceNames = <String>[
+  // From GeneratedService
+  r'createRequest',
+  r'handleCall',
+];
+
 // List of Dart language reserved words in names which cannot be used in a
 // subclass of GeneratedMessage.
-const List<String> _dartReservedWords = [
+const _dartReservedWords = <String>[
   'assert',
   'bool',
   'break',

protoc_plugin/lib/service_generator.dart

@@ -51,7 +51,10 @@ class ServiceGenerator {
   /// If a type name can't be resolved, puts it in [_undefinedDeps].
   /// Precondition: messages have been registered and resolved.
   void resolve(GenerationContext ctx) {
-    for (var m in _methodDescriptors) {
+    final usedNames = <String>{...serviceReservedMemberNames};
+
+    for (var m in _descriptor.method) {
+      methods.add(_ServiceMethod(_methodName(m.name, usedNames), m));
       _addDependency(ctx, m.inputType, "input type of ${m.name}");
       _addDependency(ctx, m.outputType, "output type of ${m.name}");
     }
@@ -131,23 +134,26 @@ class ServiceGenerator {
     return mg.fileImportPrefix + "." + mg.classname;
   }
 
-  List<MethodDescriptorProto> get _methodDescriptors => _descriptor.method;
+  List<_ServiceMethod> methods = <_ServiceMethod>[];
 
-  String _methodName(String name) => lowerCaseFirstLetter(name);
+  String _methodName(String name, Set<String> usedNames) {
+    return disambiguateName(avoidInitialUnderscore(lowerCaseFirstLetter(name)),
+        usedNames, defaultSuffixes(),
+        generateVariants: (name) => [name, '${name}_Pre']);
+  }
 
   String get _parentClass => _generatedService;
 
-  void _generateStub(IndentingWriter out, MethodDescriptorProto m) {
-    var methodName = _methodName(m.name);
-    var inputClass = _getDartClassName(m.inputType);
-    var outputClass = _getDartClassName(m.outputType);
+  void _generateStub(IndentingWriter out, _ServiceMethod m) {
+    var inputClass = _getDartClassName(m.descriptor.inputType);
+    var outputClass = _getDartClassName(m.descriptor.outputType);
 
-    out.println('$_future<$outputClass> $methodName('
+    out.println('$_future<$outputClass> ${m.dartName}('
         '$_serverContext ctx, $inputClass request);');
   }
 
   void _generateStubs(IndentingWriter out) {
-    for (var m in _methodDescriptors) {
+    for (var m in methods) {
       _generateStub(out, m);
     }
     out.println();
@@ -158,9 +164,9 @@ class ServiceGenerator {
         '$_generatedMessage createRequest($_coreImportPrefix.String method) {',
         '}', () {
       out.addBlock("switch (method) {", "}", () {
-        for (var m in _methodDescriptors) {
-          var inputClass = _getDartClassName(m.inputType);
-          out.println("case '${m.name}': return $inputClass();");
+        for (var m in methods) {
+          var inputClass = _getDartClassName(m.descriptor.inputType);
+          out.println("case '${m.descriptor.name}': return $inputClass();");
         }
         out.println("default: "
             "throw $_coreImportPrefix.ArgumentError('Unknown method: \$method');");
@@ -175,10 +181,9 @@ class ServiceGenerator {
             '$_coreImportPrefix.String method, $_generatedMessage request) {',
         '}', () {
       out.addBlock("switch (method) {", "}", () {
-        for (var m in _methodDescriptors) {
-          var methodName = _methodName(m.name);
-          var inputClass = _getDartClassName(m.inputType);
-          out.println("case '${m.name}': return this.$methodName"
+        for (var m in methods) {
+          var inputClass = _getDartClassName(m.descriptor.inputType);
+          out.println("case '${m.descriptor.name}': return this.${m.dartName}"
               "(ctx, request as $inputClass);");
         }
         out.println("default: "
@@ -266,3 +271,13 @@ class ServiceGenerator {
   static final String _generatedService =
       '$_protobufImportPrefix.GeneratedService';
 }
+
+class _ServiceMethod {
+  final String dartName;
+  final MethodDescriptorProto descriptor;
+
+  _ServiceMethod(
+    this.dartName,
+    this.descriptor,
+  );
+}

protoc_plugin/pubspec.yaml

@@ -17,6 +17,7 @@ dev_dependencies:
   pedantic: ^1.10.0
   matcher: ^0.12.10
   collection: ^1.15.0
+  grpc: ^3.0.0
 
 executables:
   protoc-gen-dart: protoc_plugin

protoc_plugin/test/file_generator_test.dart

@@ -300,9 +300,23 @@ void main() {
       ..clientStreaming = true
       ..serverStreaming = true;
 
+    // Some methods with names that overlap reserved words or existing methods
+    // in the base classes Client/Service of package:grpc.
+    final names = [
+      MethodDescriptorProto()
+        ..name = 'New'
+        ..inputType = '.Input'
+        ..outputType = '.Output',
+      MethodDescriptorProto()
+        ..name = r'_32SillyName'
+        ..inputType = '.Input'
+        ..outputType = '.Output',
+    ];
+
     var sd = ServiceDescriptorProto()
       ..name = 'Test'
-      ..method.addAll([unary, clientStreaming, serverStreaming, bidirectional]);
+      ..method.addAll(
+          [unary, clientStreaming, serverStreaming, bidirectional, ...names]);
 
     var fd = FileDescriptorProto()
       ..name = 'test'

protoc_plugin/test/generated_message_test.dart

@@ -15,6 +15,7 @@ import '../out/protos/google/protobuf/unittest_import.pb.dart';
 import '../out/protos/google/protobuf/unittest_optimize_for.pb.dart';
 import '../out/protos/multiple_files_test.pb.dart';
 import '../out/protos/reserved_names.pb.dart';
+import '../out/protos/reserved_names.pbserver.dart';
 import '../out/protos/reserved_names_extension.pb.dart';
 import '../out/protos/reserved_names_message.pb.dart';
 import '../out/protos/duplicate_names_import.pb.dart';
@@ -26,6 +27,34 @@ import '../out/protos/toplevel.pb.dart';
 
 import 'test_util.dart';
 
+class ReservedNamesService extends ReservedNamesServiceBase {
+  @override
+  Future<SimpleResponse> abc(ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: 'Abc called');
+
+  @override
+  Future<SimpleResponse> abc_Pre_(ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: 'abc_Pre called');
+
+  @override
+  Future<SimpleResponse> handleCall_(
+          ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: 'handleCall called');
+
+  @override
+  Future<SimpleResponse> if_(ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: 'If called');
+
+  @override
+  Future<SimpleResponse> new_(ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: 'New called');
+
+  @override
+  Future<SimpleResponse> x32SillyName_(
+          ServerContext ctx, SimpleReq request) async =>
+      SimpleResponse(test: '_32SillyName called');
+}
+
 void main() {
   final throwsInvalidProtocolBufferException =
       throwsA(TypeMatcher<InvalidProtocolBufferException>());
@@ -970,4 +999,21 @@ void main() {
 
     assertAllFieldsSet(value);
   });
+
+  test('Service name disambiguation', () async {
+    final service = ReservedNamesService();
+    expect(await service.handleCall(ServerContext(), 'New', SimpleReq()),
+        SimpleResponse(test: 'New called'));
+    expect(await service.handleCall(ServerContext(), 'If', SimpleReq()),
+        SimpleResponse(test: 'If called'));
+    expect(await service.handleCall(ServerContext(), 'Abc', SimpleReq()),
+        SimpleResponse(test: 'Abc called'));
+    expect(await service.handleCall(ServerContext(), 'abc_Pre', SimpleReq()),
+        SimpleResponse(test: 'abc_Pre called'));
+    expect(
+        await service.handleCall(ServerContext(), '_32SillyName', SimpleReq()),
+        SimpleResponse(test: '_32SillyName called'));
+    expect(await service.handleCall(ServerContext(), 'handleCall', SimpleReq()),
+        SimpleResponse(test: 'handleCall called'));
+  });
 }