diff --git a/protoc_plugin/CHANGELOG.md b/protoc_plugin/CHANGELOG.md index 70ac37dcf..340d3fb09 100644 --- a/protoc_plugin/CHANGELOG.md +++ b/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. diff --git a/protoc_plugin/lib/client_generator.dart b/protoc_plugin/lib/client_generator.dart index 6767d6027..336e1f7db 100644 --- a/protoc_plugin/lib/client_generator.dart +++ b/protoc_plugin/lib/client_generator.dart @@ -27,7 +27,7 @@ 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); } }); @@ -35,20 +35,18 @@ class ClientApiGenerator { } // 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);'); }); } } diff --git a/protoc_plugin/lib/grpc_generator.dart b/protoc_plugin/lib/grpc_generator.dart index a45c5fa58..256e6fb8e 100644 --- a/protoc_plugin/lib/grpc_generator.dart +++ b/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 = {...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 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; diff --git a/protoc_plugin/lib/names.dart b/protoc_plugin/lib/names.dart index 047d0b288..95b4075a4 100644 --- a/protoc_plugin/lib/names.dart +++ b/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 forbiddenTopLevelNames = [ +const forbiddenTopLevelNames = [ 'List', 'Function', 'Map', ..._dartReservedWords, ]; -final List reservedMemberNames = [ +const reservedMemberNames = [ ..._dartReservedWords, ...GeneratedMessage_reservedNames, ..._generatedMessageNames ]; -final List forbiddenExtensionNames = [ +const forbiddenExtensionNames = [ ..._dartReservedWords, ...GeneratedMessage_reservedNames, ..._generatedMessageNames ]; +const serviceReservedMemberNames = [ + ..._dartReservedWords, + ..._serviceNames, +]; + +const _serviceNames = [ + // 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 _dartReservedWords = [ +const _dartReservedWords = [ 'assert', 'bool', 'break', diff --git a/protoc_plugin/lib/service_generator.dart b/protoc_plugin/lib/service_generator.dart index c1e33b6f1..913b619ee 100644 --- a/protoc_plugin/lib/service_generator.dart +++ b/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 = {...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 get _methodDescriptors => _descriptor.method; + List<_ServiceMethod> methods = <_ServiceMethod>[]; - String _methodName(String name) => lowerCaseFirstLetter(name); + String _methodName(String name, Set 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, + ); +} diff --git a/protoc_plugin/pubspec.yaml b/protoc_plugin/pubspec.yaml index 608629850..0446e0793 100644 --- a/protoc_plugin/pubspec.yaml +++ b/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 diff --git a/protoc_plugin/test/file_generator_test.dart b/protoc_plugin/test/file_generator_test.dart index c05ba8a02..443769caa 100644 --- a/protoc_plugin/test/file_generator_test.dart +++ b/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' diff --git a/protoc_plugin/test/generated_message_test.dart b/protoc_plugin/test/generated_message_test.dart index ee51abf0e..4c4a25aac 100755 --- a/protoc_plugin/test/generated_message_test.dart +++ b/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 abc(ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: 'Abc called'); + + @override + Future abc_Pre_(ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: 'abc_Pre called'); + + @override + Future handleCall_( + ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: 'handleCall called'); + + @override + Future if_(ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: 'If called'); + + @override + Future new_(ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: 'New called'); + + @override + Future x32SillyName_( + ServerContext ctx, SimpleReq request) async => + SimpleResponse(test: '_32SillyName called'); +} + void main() { final throwsInvalidProtocolBufferException = throwsA(TypeMatcher()); @@ -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')); + }); } diff --git a/protoc_plugin/test/goldens/grpc_service.pbgrpc b/protoc_plugin/test/goldens/grpc_service.pbgrpc index aaac3691a..63d2503bf 100644 --- a/protoc_plugin/test/goldens/grpc_service.pbgrpc +++ b/protoc_plugin/test/goldens/grpc_service.pbgrpc @@ -30,6 +30,18 @@ class TestClient extends $grpc.Client { '/Test/Bidirectional', ($0.Input value) => value.writeToBuffer(), ($core.List<$core.int> value) => $0.Output.fromBuffer(value)); + static final _$new_ = $grpc.ClientMethod<$0.Input, $0.Output>( + '/Test/New', + ($0.Input value) => value.writeToBuffer(), + ($core.List<$core.int> value) => $0.Output.fromBuffer(value)); + static final _$$addMethod_ = $grpc.ClientMethod<$0.Input, $0.Output>( + '/Test/$addMethod', + ($0.Input value) => value.writeToBuffer(), + ($core.List<$core.int> value) => $0.Output.fromBuffer(value)); + static final _$x32SillyName_ = $grpc.ClientMethod<$0.Input, $0.Output>( + '/Test/_32SillyName', + ($0.Input value) => value.writeToBuffer(), + ($core.List<$core.int> value) => $0.Output.fromBuffer(value)); TestClient($grpc.ClientChannel channel, {$grpc.CallOptions? options, @@ -59,6 +71,21 @@ class TestClient extends $grpc.Client { {$grpc.CallOptions? options}) { return $createStreamingCall(_$bidirectional, request, options: options); } + + $grpc.ResponseFuture<$0.Output> new_($0.Input request, + {$grpc.CallOptions? options}) { + return $createUnaryCall(_$new_, request, options: options); + } + + $grpc.ResponseFuture<$0.Output> $addMethod_($0.Input request, + {$grpc.CallOptions? options}) { + return $createUnaryCall(_$$addMethod_, request, options: options); + } + + $grpc.ResponseFuture<$0.Output> x32SillyName_($0.Input request, + {$grpc.CallOptions? options}) { + return $createUnaryCall(_$x32SillyName_, request, options: options); + } } abstract class TestServiceBase extends $grpc.Service { @@ -93,6 +120,27 @@ abstract class TestServiceBase extends $grpc.Service { true, ($core.List<$core.int> value) => $0.Input.fromBuffer(value), ($0.Output value) => value.writeToBuffer())); + $addMethod($grpc.ServiceMethod<$0.Input, $0.Output>( + 'New', + new__Pre, + false, + false, + ($core.List<$core.int> value) => $0.Input.fromBuffer(value), + ($0.Output value) => value.writeToBuffer())); + $addMethod($grpc.ServiceMethod<$0.Input, $0.Output>( + '$addMethod', + $addMethod__Pre, + false, + false, + ($core.List<$core.int> value) => $0.Input.fromBuffer(value), + ($0.Output value) => value.writeToBuffer())); + $addMethod($grpc.ServiceMethod<$0.Input, $0.Output>( + '_32SillyName', + x32SillyName__Pre, + false, + false, + ($core.List<$core.int> value) => $0.Input.fromBuffer(value), + ($0.Output value) => value.writeToBuffer())); } $async.Future<$0.Output> unary_Pre( @@ -105,6 +153,21 @@ abstract class TestServiceBase extends $grpc.Service { yield* serverStreaming(call, await request); } + $async.Future<$0.Output> new__Pre( + $grpc.ServiceCall call, $async.Future<$0.Input> request) async { + return new_(call, await request); + } + + $async.Future<$0.Output> $addMethod__Pre( + $grpc.ServiceCall call, $async.Future<$0.Input> request) async { + return $addMethod_(call, await request); + } + + $async.Future<$0.Output> x32SillyName__Pre( + $grpc.ServiceCall call, $async.Future<$0.Input> request) async { + return x32SillyName_(call, await request); + } + $async.Future<$0.Output> unary($grpc.ServiceCall call, $0.Input request); $async.Future<$0.Output> clientStreaming( $grpc.ServiceCall call, $async.Stream<$0.Input> request); @@ -112,4 +175,9 @@ abstract class TestServiceBase extends $grpc.Service { $grpc.ServiceCall call, $0.Input request); $async.Stream<$0.Output> bidirectional( $grpc.ServiceCall call, $async.Stream<$0.Input> request); + $async.Future<$0.Output> new_($grpc.ServiceCall call, $0.Input request); + $async.Future<$0.Output> $addMethod_( + $grpc.ServiceCall call, $0.Input request); + $async.Future<$0.Output> x32SillyName_( + $grpc.ServiceCall call, $0.Input request); } diff --git a/protoc_plugin/test/protos/reserved_names.proto b/protoc_plugin/test/protos/reserved_names.proto index dff00aa51..235c45f0e 100644 --- a/protoc_plugin/test/protos/reserved_names.proto +++ b/protoc_plugin/test/protos/reserved_names.proto @@ -256,6 +256,24 @@ message MessageWithReservedEnum { optional ReservedEnum enum = 1; } +message SimpleReq { + optional string test = 1; +} + +message SimpleResponse { + optional string test = 1; +} + +service ReservedNamesService { + rpc New(SimpleReq) returns (SimpleResponse); + rpc If(SimpleReq) returns (SimpleResponse); + rpc Abc(SimpleReq) returns (SimpleResponse); + rpc abc_Pre(SimpleReq) returns (SimpleResponse); + rpc _32SillyName(SimpleReq) returns (SimpleResponse); + rpc handleCall(SimpleReq) returns (SimpleResponse); +} + + enum ReservedEnum { assert = 0; break = 1;