The committers listed above are authorized under a signed CLA.

• ✅ login: aarongable / name: Aaron Gable (d551dad, f75c2d6, 06c9a00, ba0d4e3, 8a9ade3, f159599, 08f3f64, 8c66d69, 617090d, faab808, 2230a4e, 586bde9, 9f4fa15, 8f93711, 9ddc54d, cc25c38)

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 81.17%. Comparing base (adf976b) to head (8a9ade3).
Report is 31 commits behind head on master.

❗ Current head 8a9ade3 differs from pull request most recent head 8f93711. Consider uploading reports for the commit 8f93711 to get more accurate results

@@            Coverage Diff             @@
##           master    #7057      +/-   ##
==========================================
- Coverage   81.24%   81.17%   -0.07%     
==========================================
  Files         345      345              
  Lines       33941    33954      +13     
==========================================
- Hits        27574    27562      -12     
- Misses       5202     5228      +26     
+ Partials     1165     1164       -1     

Thanks for the PR and sorry I haven't gotten to it yet. I like the way it looks, but I need to spend more time thinking about it than I currently have available. Hopefully I will have more time by early-mid next week.

So even with the generics being generated, the route_guide server and client do not need any changes?

I thought we were saying things would not be backward compatible...can you remind me where the trouble comes in please?

Correct, as this change is written now, everything is backwards compatible. (This is why I asked about maybe putting the new type definitions directly into the grpc package, rather than into experimental, since technically this change could be made completely invisibly to everyone as long as they're on a version of go with generics.)

In my ideal world, the type aliases wouldn't exist. Rather than the generated code saying

type RouteGuideServer interface {
	RouteChat(RouteGuide_RouteChatServer) error
}
...
type RouteGuide_RouteChatServer = grpc.BidiStreamServer[RouteNote, RouteNote]

it would just say

type RouteGuideServer interface {
	RouteChat(grpc.BidiStreamServer[RouteNote, RouteNote]) error
}

But that change would be backwards incompatible, since everyone would need to update the function signatures of the types that they use to implement such servers.

One way to split the difference between how this PR currently looks and my imagined ideal would be to use the new types everywhere (including in the interface definitions, and in the client implementation function signatures) and still provide the type aliases purely for backwards compatibility:

type RouteGuideServer interface {
	RouteChat(grpc.BidiStreamServer[RouteNote, RouteNote]) error
}
...
type RouteGuide_RouteChatServer = grpc.BidiStreamServer[RouteNote, RouteNote]

This would be a slightly more visible change, as anyone who regenerates their protos will see the interface definitions change, but all of their existing code will still continue to work thanks to the type alias.

Both of the two versions (this and the "split the difference") seem to be the same in practice. Is there anything you're missing from this current version that could be done with the other version (besides maybe looking nicer / being more understandable)?

Nope, they're roughly equivalent to me. From a "will it compile" perspective, they're identical to me.

I do slightly prefer the "split the difference" version for one reason. The original motivation behind this change was allowing a single struct to implement two different gRPC Services that share the same streaming method, and both the current version and the "split the difference" version allow that. However, the current version makes it look a little awkward, because the function signature on the type you implement won't match at least one of the two interfaces you're implementing:

// Generated code
type GreeterServer interface {
	SayHello(*HelloRequest, Greeter_SayHelloServer) error
}

type Greeter_SayHelloServer = grpc.ServerStreamServer[HelloReply]

type GreeterReadOnlyServer interface {
	SayHello(*HelloRequest, GreeterReadOnly_SayHelloServer) error
}

type GreeterReadOnly_SayHelloServer = grpc.ServerStreamServer[HelloReply]

// Implementation
type GreeterImpl struct { ... }

// Any of the three lines below will compile, but none of them are identical to both interface lines above.
func (g *GreeterImpl) SayHello(req *emptypb.Empty, stream pb.Greeter_SayHelloServer) error { ... }
func (g *GreeterImpl) SayHello(req *emptypb.Empty, stream pb.GreeterReadOnly_SayHelloServer) error { ... }
func (g *GreeterImpl) SayHello(req *emptypb.Empty, stream grpc.ServerStreamServer[pb.HelloReply]) error { ... }

The "spllit the difference" version would cause the third of the three options above to exactly match both generated interfaces.

But as you said, that's just "looking nicer / being more understandable", and the current version completely works.

Conflicting files
cmd/protoc-gen-go-grpc/testdata/golden_grpc.pb.go

FYI this file was deleted, so you can just delete it in your branch as well.

I love it. Thank you for the contribution. We just need a second pass review (and also please make the q->s change).