Conformance to GRPCProtobufPayload generates wrong code when using ProtoPathModuleMappings option

[jagobagascon]

New Issue Checklist

• Updated grpc-swift to the latest version
• I read the Contribution Guidelines
• I searched for existing GitHub issues(especially, there are many resolved issues related to SSL)

Issue Description

We are currently generating our swift code using ProtoPathModuleMappings option to split it into different modules. As each module needs to be in its own folder we were generating the code by calling protoc-gen-grpc-swift plugin once per module with a different out folder.

protoc ... --grpc-swift_out="/module-a" a.proto
protoc ... --grpc-swift_out="/module-b" b.proto

This causes the following error during swift build:

error: redundant conformance of 'Google_Protobuf_Empty' to protocol 'GRPCProtobufPayload'

This issue was fixed in this commit by avoiding the definition of multiple conformances to GRPCProtobufPayload for the same class. But for it to work we had to generate all our protos in the same plugin call and then move the generated code to its module folder:

protoc ... --grpc-swift_out=".tmp" a.proto b.proto
mv a.*.swift /module-a/
mv b.*.swift /module-b/

This is not ideal, but it "fixes" our previous problem. After this change, the GRPCProtobufPayload is added only to the first class that needs it. In our example this would be a.grpc.swift and when trying to compile the code in /module-b it fails with the following error:

b.grpc.swift:32:74: error: type 'Google_Protobuf_Empty' does not conform to protocol 'GRPCPayload'

I'm not sure what the fix should be. I'm kind of new to swift. I managed to compile the code by adding a protocol that extends GRPCProtobufPayload per module and using that.

protocol ModuleAPayload : GRPCProtobufPayload {}
extension SwiftProtobuf.Google_Protobuf_Empty: ModuleAPayload {}

I don't know if this is good practice or not, it feels kinda hacky.

You can use the code in this repo to replicate this problem (tested against master branch). It contains a Makefile with three rules:

make generate_single # generates code in a single call to protoc then moves it to its module
make generate # generates code calling protoc once per module
make build # builds generated code

• make generate_single build: will give you our first error
• make generate build: will give you our second error

[glbrntt]

Thanks for the super detailed issue and repro @jagobagascon!

At the moment the codgen will generate the conformance based on the input and output types of the rpc if they have not already been generated by the current invocation of the codegen rather than for each message seen in the proto file.

The rationale for this was that files containing just messages can be passed to protoc-gen-swift, and files with just services can be passed to a separate protoc invocation with protoc-gen-grpc-swift. Importantly in this situation, it allows protoc-gen-grpc-swift to generate conformance for messages for which it was not passed the source .proto file (i.e. Google_Protobuf_*).

Perhaps that was the wrong approach and we should generate conformance for the messages passed to us. Since Google_Protobuf_* messages are vended by SwiftProtobuf, GRPC could provide conformance for the same set of messages.

One concern I have is that SwiftProtobuf could vend more Google_Protobuf_* messages in a future version which would make the code generated by gRPC Swift incompatible (since the new conformance would be missing). However, it's not clear if this is actually a problem (i.e. will SwiftProtobuf actually vend more Google_Protobuf_* messages?), and if it becomes a problem the escape hatch would be for users to either wait for a new version of GRPC or pass the relevant source .proto to the invocation of protoc-gen-grpc-swift as well.

In summary, I think I'm leaning towards:

• Providing conformance for Google_Protobuf* in GRPC
• Generating conformance based on messages define the proto files passed to protoc

[glbrntt]

Any thoughts on this @Lukasa?

[Lukasa]

Wow, awkward. Let me restate my understanding of the problem here in less-Protobuf/more-Swift terms.

The GRPC module defines a protocol GRPCProtobufPayload. This protocol specifically defines a type that is both a SwiftProtobuf.Message and a GRPC.GRPCPayload. Its purpose is essentially to provide the conformance of a SwiftProtobuf.Message to GRPC.GRPCPayload.

The codegen will operate on a bunch of .proto files that spit out generated Swift code that is part of user Swift modules. These files include, amongst other things, conformances of SwiftProtobuf.Message objects to GRPC.GRPCProtobufPayload for protobufs as needed.

The problem is that some protobufs are defined by SwiftProtobuf. We can use for this example SwiftProtobuf.Google_Protobuf_Empty, but there are several others inside SwiftProtobuf. When used in service protos, these need to be conformed to GRPC.GPRCProtobufPayload.

The problem is that we are breaking the rule of Swift conformances: thou shalt only conform a type to a protocol when you own either the type or the protocol. In this case, as the conformances are emitted by codegen, "thou" is equivalent to "the module owned by the user into which we generate code".

That's why it's not a problem for most use-cases: if the user is using custom user-defined proto messages, the user owns the type. They meet the criteria for extension, and so may conform the type to whichever protocol they want.

However, for the proto messages owned by SwiftProtobuf, the user owns neither the type (it's owned by SwiftProtobuf) or the protocol (owned by GRPC). Thus they are in violation of the rule of Swift conformances. This is one of many manifestations of this kind of problem.

There are two solutions, I think. One is the one @glbrntt has outlined above: bring the protocol conformances into GRPC. This basically works fine so long as protobuf doesn't add new proto messages, and even if it does the problem can be nicely resolved by updating GRPC.

The second solution is to remove the protocol altogether. I have to admit that I'm a bit unsure why GRPC cannot simply take SwiftProtobuf.Message directly. It can then provide a transformation internally to use the same codepath as GRPCProtobufPayload.

That said, I think the first proposal probably gets my vote. I think the risk of breakage is low, and easily mitigated if it happens. It makes things substantively easier to work with without breaking or invalidating wide swathes of code. So I think I'm in favour of going that way.

[jagobagascon]

Another thing that concerns me is that with the current approach GRPCPayload conformance may appear in a class or another or even in both depending on how you generate the code. Compiling all protos at once VS compiling each of them independently will give you different results (just like the examples I used before).

Both of the changes proposed (apart from fixing this bug) would also make the code generation more consistent, because the generated code would be the same in both cases.

[glbrntt]

The second solution is to remove the protocol altogether. I have to admit that I'm a bit unsure why GRPC cannot simply take SwiftProtobuf.Message directly. It can then provide a transformation internally to use the same codepath as GRPCProtobufPayload.

To support different payloads (e.g. FlatBuffers) the public API for creating RPCs uses message types which conform to GRPCPayload. I think your suggestion would require having a special case for SwiftProtobuf.Message in the API unless I'm missing something?

That said, I think the first proposal probably gets my vote. I think the risk of breakage is low, and easily mitigated if it happens. It makes things substantively easier to work with without breaking or invalidating wide swathes of code. So I think I'm in favour of going that way.

To clarify: is your vote for only adding the conformance to the Google_Protobuf* messages to GRPC?

[Lukasa]

I think your suggestion would require having a special case for SwiftProtobuf.Message in the API unless I'm missing something?

Yes, indeed: we'd need to accept GRPCPayload and SwiftProtobuf.Message. Just throwing it out as an idea.

To clarify: is your vote for only adding the conformance to the Google_Protobuf* messages to GRPC?

I think that ought to be sufficient, no?

[glbrntt]

To clarify: is your vote for only adding the conformance to the Google_Protobuf* messages to GRPC?

I think that ought to be sufficient, no?

It's sufficient to fix the problem but I don't think it's optimal. The reason for generating the conformance the way we did was to support the messages whole files we don't have, specifically those provided by SwiftProtobuf. With that now gone I think generating the conformance when we're passed the file is probably for the "stability" reasons @jagobagascon pointed out. We can do these separately, of course.

[Lukasa]

Maybe it's just me, but I don't have a clear handle on what you're proposing here @glbrntt. Mind elaborating?

[glbrntt]

Eugh, yeah, that was pretty horrendously written!

I'm proposing two changes: the first is the add conformance for Google_Protobuf* directly to GRPC; this will resolve this issue.

The second change I'd like to make is to generate the GRPCProtobufPayload conformance based on message definitions in .proto files passed to the protoc-gen-grpc-swift plugin rather than looking at the input and output types for rpcs defined within services in the .proto files. The result of this is more stable output from the codegen; at the moment compiling all protos at once can give different output to compiling each of them independently.

[Lukasa]

Ok, fab, I am in favour of both changes then.

[glbrntt]

@jagobagascon this is resolved by #811, please let us know if you have any more problems!