Avoid generating multiple conformances to GRPCProtobufPayload #740
Conversation
Motivation: - Generated services must also add conformance to the request/response types that they require - If different services rely on the same message type then this conformance is generated multiple times; leading to a build error Modifications: - Pass the observed message types between each run of the generator Result: - Code generated for services defined in different files with the same message types does not cause a build error
|
cc @Davidde94 |
|
Fixes #738 |
| var observedMessages = Set<String>() | ||
|
|
||
| // process each .proto file separately | ||
| for fileDescriptor in descriptorSet.files { |
There was a problem hiding this comment.
Is this ordering stable? Otherwise the location of the declaration might shift around.
There was a problem hiding this comment.
Good question; I honestly have no idea. It's certainly stable when the input is stable so I assume it depends on the order you pass them to protoc. Do you think it matters?
I'm not super happy with this approach but I'm not sure of a better one. The alternative I considered was generating the extensions based on the message descriptions (instead of the input/output for each RPC), that would make the location stable but it means you can generate conformance for messages which aren't used for RPCs. We'd also need to provide conformance for the "built in" messages (e.g. Any, Timestamp, etc) within GRPC so we'd be forced to update if SwiftProtobuf adds new messages.
There was a problem hiding this comment.
The problem with an unstable ordering would be that every protoc run could create different files, resulting in a lot of churn. How about simply sorting the list ourselves in some stable fashion to be sure? Just an idea, though.
There was a problem hiding this comment.
Sorting by filename is straightforward to add, so might as well.
I ran this on a case I ran into the other day to double check. I think we probably need some integration style tests to generate code and build it to validate but we need all the infrastructure first. I'll have a think about it because it would also be useful to have things like allocation counting and perf tests as part of CI. |
SGTM; let's not worry about that in this PR. |
|
Hi @glbrntt, I updated to alpha-11 and still run into this issue in some cases where redundant GRPCProtobufPayload conformance was generated for structs. Unfortunately I can't share the original proto files and the generated swift files, but pls let me know how I could help debug this issue. |
3 participants
Motivation:
types that they require
conformance is generated multiple times; leading to a build error
Modifications:
Result:
message types does not cause a build error