fix generated method names

[Skoti]

This PR fixes incorrectly generated method names.

For example:
rpc GRPCUnary(google.protobuf.Empty) returns (google.protobuf.Empty) {}

The plugin correctly generates:

• makeGRPCUnaryInterceptors
• Methods.gRPCUnary
• func gRPCUnary

but incorrectly:

• makeGrpcunaryCall
  This seems to be the only place with a different logic.

And when using lowercase letters, the interceptor method name is incorrect:
rpc unary(google.protobuf.Empty) returns (FunctionName) {}

The plugin correctly generates:

• makeUnaryCall
• Methods.unary
• func unary
  but incorrectly:
• makeunaryInterceptors

The PR fixes these two issues by introducing a common logic for methodMakeFunctionCallName and methodInterceptorFactoryName. The logic is similar to the one in methodFunctionName, just that it uppercases the first character instead of lowercasing.

Thus, for the given examples the result is:
makeGRPCUnaryCall and makeUnaryInterceptors

[linux-foundation-easycla]

• ❌ - login: @Skoti / name: Skoti . The commit (f7acfc9, 2d9c60b) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[glbrntt]

Thanks for opening this PR, however, I don't think we can accept it as is.

The incorrect casing has already shipped so changing it now will be a breaking change for any users currently relying on it.

We can, however, generate both variants of the makeX method (when it's necessary) and deprecate the incorrect version saying that it's been renamed to the new name.

The makeXInterceptors method is a protocol requirement if I remember correctly so fixing that is more difficult because adding a requirement to the protocol is a breaking change. If we added it we'd also need to add a default implementation. The only sensible thing for the default implementation of the new method to do is call the old and now deprecated method. This also means that the user won't be aware that they need to actually implement the new method.

I think if we're to accept a change for these fixes we should leave makeXInterceptors as it is and only fix the makeX methods. The incorrect methods should be kept and be deprecated and just call through to the correctly cased methods.

[Skoti]

Unfortunately, both methods are in a protocol. The makeX is used in the AsyncClientProtocol, and makeXInterceptors in ServerInterceptorFactoryProtocol.

There are two solutions:

1. Add the "Breaking Changes" section in the release notes, similarly as in gRPC Swift 1.7.1-async-await.2.
   It is breaking, but at the same time, it's super easy to adjust the code.

OR

2. Add it as another requirement to the protocol, which by default calls the old implementation.

   This also means that the user won't be aware that they need to actually implement the new method.

   They will see the deprecation warning when they call the method, but not when conforming to the protocol. However, I believe this is a rare case to conform to AsyncClientProtocol, the ServerInterceptorFactoryProtocol for interceptors is probably more commonly adopted - so we could constrain changes to the former only.
   Anyway, when the deprecated method is removed, thanks to the warning, they should already have the correct call sites and only change the conformance, which is again very easy, as it's just a rename.

[glbrntt]

1. Add the "Breaking Changes" section in the release notes, similarly as in gRPC Swift 1.7.1-async-await.2.
   It is breaking, but at the same time, it's super easy to adjust the code.

This approach isn't tenable; it's still a breaking change.

1.7.1-async-await.2 was allowed to have breaking changes because the -async-await release series was a side release (if you depended on from: "1.7.0" the -async-await.x versions wouldn't be picked up.)

2. Add it as another requirement to the protocol, which by default calls the old implementation.

   This also means that the user won't be aware that they need to actually implement the new method.

   They will see the deprecation warning when they call the method, but not when conforming to the protocol. However, I believe this is a rare case to conform to AsyncClientProtocol, the ServerInterceptorFactoryProtocol for interceptors is probably more commonly adopted - so we could constrain changes to the former only.

Yeah, I agree with that, I don't think we need to fix ServerInterceptorFactoryProtocol.

Anyway, when the deprecated method is removed, thanks to the warning, they should already have the correct call sites and only change the conformance, which is again very easy, as it's just a rename.

Given that users don't typically implement AsyncClientProtocol, why don't we keep the protocol as it is but (if necessary) generate an extension providing the correct names which calls through to the wrong names. Users which are only using the protocol can get the correct names, users which implement the protocol don't have to change anything, and the code generation changes are simpler. WDYT?

[Skoti]

Given that users don't typically implement AsyncClientProtocol, why don't we keep the protocol as it is but (if necessary) generate an extension providing the correct names which calls through to the wrong names. Users which are only using the protocol can get the correct names, users which implement the protocol don't have to change anything, and the code generation changes are simpler. WDYT?

We can do this, I'd prefer to have anything with a proper name than nothing.
I've already changed the code, take a look.

The current implementation of methodMakeFunctionCallName is itself a bit breaking because it relies on NamingUtils.toUpperCamelCase(self.method.name).
Which means its behavior can change depending on the SwiftProtobuf version being used.
This would be fine if the things it refers to come from SwiftProtobuf, but it's a bit weird to make such a dependency for a method name of a gRPC service that has nothing to do with how SwiftProtobuf names things.

These lines are especially a point of interest:
https://github.com/apple/swift-protobuf/blob/7407bce0a2f7228fd0b40c314f3ff3314e9a26e2/Sources/SwiftProtobufPluginLibrary/NamingUtils.swift#L300-L301
where appreviations are defined as:
https://github.com/apple/swift-protobuf/blob/7407bce0a2f7228fd0b40c314f3ff3314e9a26e2/Sources/SwiftProtobufPluginLibrary/NamingUtils.swift#L249

so for example
rpc GetId(google.protobuf.Empty) returns (google.protobuf.Empty) {}
is not generated as makeGetIdCall as one would expect, but makeGetIDCall.

So the newly generated wrapping function that uses a proper name also helps to avoid this issue 💪 .

[glbrntt]

The current implementation of methodMakeFunctionCallName is itself a bit breaking because it relies on NamingUtils.toUpperCamelCase(self.method.name). Which means its behavior can change depending on the SwiftProtobuf version being used. This would be fine if the things it refers to come from SwiftProtobuf, but it's a bit weird to make such a dependency for a method name of a gRPC service that has nothing to do with how SwiftProtobuf names things.

There's a strong dependency between gRPC Swift and SwiftProtobuf; we can't the major version of SwiftProtobuf without changing the major version of gRPC Swift. SwiftProtobuf also can't change the behavior of that method without potentially breaking its users so it's actually not an issue for gRPC to be using this at all.

These lines are especially a point of interest: https://github.com/apple/swift-protobuf/blob/7407bce0a2f7228fd0b40c314f3ff3314e9a26e2/Sources/SwiftProtobufPluginLibrary/NamingUtils.swift#L300-L301 where appreviations are defined as: https://github.com/apple/swift-protobuf/blob/7407bce0a2f7228fd0b40c314f3ff3314e9a26e2/Sources/SwiftProtobufPluginLibrary/NamingUtils.swift#L249

so for example rpc GetId(google.protobuf.Empty) returns (google.protobuf.Empty) {} is not generated as makeGetIdCall as one would expect, but makeGetIDCall.

So the newly generated wrapping function that uses a proper name also helps to avoid this issue 💪 .

Actually makeGetIDCall is what I would expect to be generated per the Swift API design guidelines: https://www.swift.org/documentation/api-design-guidelines/#conventions

This is exactly what that code in SwiftProtobuf is for, and we should use it here as well.

[glbrntt]

We should be using the camel caser here to make it upper camel-case

[glbrntt]

nit: these should both be printRPC...

[glbrntt]

You'll also need to agree to and sign the CLA for us to consider this patch.