Type aliases for generics

[joshcarp]

Whenever I'm writing a new connect service it always seems frustrating that it seems like I've got a lot of noise in the function parameter:

func (e *elizaServer) Say(ctx context.Context, req *connect.Request[elizav1.SayRequest]) (*connect.Response[elizav1.SayResponse], error) { // very long
	return connect.NewResponse(&elizav1.SayResponse{
		Sentence: "foobar",
	}), nil
}

Even with the simplest unary functions the verbosity forces for multi line function signatures which can be an eye sore:

func (e *elizaServer) Say(
	ctx context.Context,
	req *connect.Request[elizav1.SayRequest],
) (*connect.Response[elizav1.SayResponse], error) {
	reply, _ := eliza.Reply(req.Msg.Sentence) // ignore end-of-conversation detection
	return connect.NewResponse(&elizav1.SayResponse{
		Sentence: reply,
	}), nil
}

To solve this, a SayRequest could be generated within elizav1connect such that no generic syntax shows up in the function signature:

// eliza.connect.go

type SayRequest = connect_go.Request[v1.SayRequest]
type SayResponse = connect_go.Response[v1.SayResponse]

// ElizaServiceHandler is an implementation of the buf.connect.demo.eliza.v1.ElizaService service.
type ElizaServiceHandler interface {
	// Say is a unary request demo. This method should allow for a one sentence
	// response given a one sentence request.
	Say(context.Context, *SayRequest) (*SayResponse, error)
}

This would allow for a simpler function signature to implement:

func (e *elizaServer) Say(ctx context.Context, req *elizav1connect.SayRequest) (*elizav1connect.SayResponse, error) { // not as long
	reply, _ := eliza.Reply(req.Msg.Sentence)
	return &elizav1connect.SayResponse{
		Msg: &elizav1.SayResponse{ // This part here is inferred by the IDE
			Sentence: reply,
		},
	}, nil
}

benefits:

• This would be a non breaking change
• IDEs would have an easier time with autocomplete and type hints

Making library decisions around IDEs is probably not a great justification, but I do find myself often getting confused by the numerous *connect.Request, *connect.Response and other types.

A similar thing could be done with streaming endpoints too:

type IntroduceRequest = connect_go.Request[v1.IntroduceRequest]
type IntroduceStream = connect_go.ServerStreamForClient[v1.IntroduceResponse]

[torkelrogstad]

This sounds like an excellent idea, IMO! Another pain point in the similar vein is the need for NewRequest in tests. IDE-autocomplete is not flawless, and I find myself writing very tiring amounts of repetetive strings when writing tests with many different cases.

[bufdev]

In theory, I like it. However, there is an issue with name conflicts. What happens if you have:

// a/a.proto

package a;

message One {}

// b/b.proto

package b;

import "a/a.proto";

message One {}

service OneService {
  rpc AOne(a.One) returns (a.One);
  rpc BOne(One) returns (One);
}

You'll have to derive a scheme to deal with this. The most obvious is to prefix each aliased name with RPC name, but even this doesn't work, consider:

// b/b.proto

package b;

import "a/a.proto";

message One {}

service OneService {
  rpc One(One) returns (One);
}

service TwoService {
  rpc One(a.One) returns (a.One);
}

So then, you have to prefix with either the package name, or the service AND RPC name. The results are not pretty:

// packageName
type BufConnectDemoV1SayRequest = connect_go.Request[v1.SayRequest]
type BufConnectDemoV1SayResponse = connect_go.Response[v1.SayResponse]

// service and RPC name
type ElizaService_Say_SayRequest = connect_go.Request[v1.SayRequest]
type ElizaService_Say_SayResponse = connect.go.Response[v1.SayResponse]

And even this doesn't get you what you need. In our above example, One is both a request and response. This means you'll have to suffix your aliased types with Request and Response. In the One case, this actually yields what you want, but in the SayRequest/SayResponse case, this gets even uglier:

// service and RPC name
type ElizaService_Say_SayRequestRequest = connect_go.Request[v1.SayRequest]
type ElizaService_Say_SayRequestResponse = connect_go.Response[v1.SayRequest]
type ElizaService_Say_SayResponseRequest = connect.go.Request[v1.SayResponse]
type ElizaService_Say_SayResponseResponse = connect.go.Response[v1.SayResponse]

I'm not sure if there is a solution that doesn't result in ugly naming, but perhaps you both can come up with one.

[joshcarp]

I think that either:

• The aliased types should be generated only for the base package, so if any request/response type isn't in the same proto package then the alias doesn't exist, or
• The type alias always exists in a <package>connect package, so even if a package doesn't have an rpc it will still have a connect package with the aliased types
• this last one probably wont work if the connect codegen hasnt been run for the other package which might be a problem

[akshayjshah]

The aliased types should be generated only for the base package, so if any request/response type isn't in the same proto package then the alias doesn't exist...

This seems reasonable to me. It handles the common case nicely, results in good names, and isn't overly complex.

My main reservation with this proposal is that it prioritizes short function signatures over obviousness. If we alias connect.Request[foov1.GetFooRequest] to foov1connect.GetFooRequest, we'd need to document that users must use connect.NewRequest(&foov1.GetFooRequest{...}) to construct these values - we wouldn't have an exported constructor for *foov1connect.GetFooRequests, and using a struct literal is incorrect.

[marekbuild]

What are people's thoughts on the following ...

While developing a demo application with connect-go, just to try out some possible solutions to this issue, I manually added the following type aliases to the request and response types in the demo:

type ConnectFilmsRequest = connect.Request[FilmsRequest] // Manually added

type FilmsRequest struct {
	...
	FilmIds []uint64 `protobuf:"varint,1,rep,packed,name=filmIds,proto3" json:"filmIds,omitempty"`
	Limit   uint32   `protobuf:"varint,2,opt,name=limit,proto3" json:"limit,omitempty"`
}

type ConnectFilmsResponse = connect.Response[FilmsResponse] // Manually added

type FilmsResponse struct {
	...
	Films []*Film `protobuf:"bytes,1,rep,name=films,proto3" json:"films,omitempty"`
}

That allowed for usage where the package name would still make it clear which type you were referring to even if multiple Go packages defined FilmsRequest or FilmsResponse:

func (s *FilmServer) GetFilms(
	ctx context.Context, 
	req *filmv1.ConnectFilmsRequest,
) (*filmv1.ConnectFilmsResponse, error) {
	return connect.NewResponse(&filmv1.FilmsResponse{
		Films: []*filmv1.Film{
			{
				Id:           1,
				Name:         "Return of the Jedi",
				CharacterIds: []uint64{},
			},
		},
	}), nil
}

Though not as short as some other solutions, it did at lease remove the stuttering, eg:
connect.Request[filmv1.FilmsRequest] becomes filmv1.ConnectFilmsRequest.

That seems nice since it is suggested that developers always add ...Request and ...Response to their input and output messages for RPCs.

[akshayjshah]

@marekbuild That's nice and avoids conflicts, but it generates code into the base types - which we don't want to do, because it's messy and incompatible with BSR remote packages.

[akshayjshah]

We've let this sit for quite a while. I think we can move forward with generating type aliases in the Connect packages.

Regarding Peter's concern with name collisions, I think the best course of action is to not generate aliases for poorly-behaved request/response messages. (We can generate comments explaining why.) The idea is to reduce boilerplate and wordy type names where possible, and very long identifiers don't help.

Regarding my earlier concern about people maybe forgetting to use constructors, I think we're okay - it's currently safe for people to skip the constructor and do &connect.Request{Msg: &someProtobufThing{}}. We should still document that they ought to use the NewRequest and NewResponse constructors, but this isn't as unsafe as I'd thought.

Whenever we tackle this, we should be sure to also update the README, all the examples, the tests, the Connect docs, and the ancillary repositories (health, reflection, etc) to use the aliases.

[emcfarlane]

Tried an idea for generating types based on service and method name prefix, similar to how procedure, client and server interfaces are namespaced.
For the ping service it helps a little:

Ping(context.Context, *connect_go.Request[v1.PingRequest]) (*connect_go.Response[v1.PingResponse], error)
Fail(context.Context, *connect_go.Request[v1.FailRequest]) (*connect_go.Response[v1.FailResponse], error)
Sum(context.Context, *connect_go.ClientStream[v1.SumRequest]) (*connect_go.Response[v1.SumResponse], error)
CountUp(context.Context, *connect_go.Request[v1.CountUpRequest], *connect_go.ServerStream[v1.CountUpResponse]) error
CumSum(context.Context, *connect_go.BidiStream[v1.CumSumRequest, v1.CumSumResponse]) error

is now:

Ping(context.Context, *PingServicePingRequest) (*PingServicePingResponse, error)
Fail(context.Context, *PingServiceFailRequest) (*PingServiceFailResponse, error)
Sum(context.Context, *PingServiceSumStream) (*PingServiceSumResponse, error)
CountUp(context.Context, *PingServiceCountUpRequest, *PingServiceCountUpStream) error
CumSum(context.Context, *PingServiceCumSumStream) error

But once you try implement it the type signature gets pretty unwieldy again.

Ping(context.Context, *pingv1connect.PingServicePingRequest) (*pingv1connect.PingServicePingResponse, error)
Fail(context.Context, *pingv1connect.PingServiceFailRequest) (*pingv1connect.PingServiceFailResponse, error)
Sum(context.Context, *pingv1connect.PingServiceSumStream) (*pingv1connect.PingServiceSumResponse, error)
CountUp(context.Context, *pingv1connect.PingServiceCountUpRequest, *pingv1connect.PingServiceCountUpStream) error
CumSum(context.Context, *pingv1connect.PingServiceCumSumStream) error

[akshayjshah]

Do we need the service name prefix? Just this should be fine:

Ping(context.Context, *PingRequest) (*PingResponse, error)
Fail(context.Context, *FailRequest) (*FailResponse, error)
Sum(context.Context, *SumStream) (*SumResponse, error)
CountUp(context.Context, *CountUpRequest, *CountUpStream) error
CumSum(context.Context, *CumSumStream) error

[emcfarlane]

It needs the service to separate the method names:

service FailService {
  rpc Ping(FailRequest) returns (FailResponse) {}
}

Would be overlapping with PingRequest.

Ping(context.Context, *FailServicePingRequest) (*FailServicePingResponse, error)

[meling]

It needs the service to separate the method names:

service FailService {
  rpc Ping(FailRequest) returns (FailResponse) {}
}

Would be overlapping with PingRequest.

Ping(context.Context, *FailServicePingRequest) (*FailServicePingResponse, error)

I assume that you meant PingRequest and PingResponse in the FailService.Ping() rpc (to match the "generated code" you provided next.)

But why does it matter that it will be overlapping, if they are in the same proto package, then PingRequest should be the same type whether or not it is used in the FailService or PingService.

[emcfarlane]

@meling it's name is based on the service and RPC method, not the message type. So for the two Ping methods above the type aliases would be:

type (
	// PingService.Ping
	PingServicePingRequest  = connect_go.Request[v1.PingRequest]
	PingServicePingResponse = connect_go.Response[v1.PingResponse]
	// FailService.Ping
	FailServicePingRequest  = connect_go.Request[v1.FailRequest]
	FailServicePingResponse = connect_go.Response[v1.FailResponse]
)

I think this will avoid issues with types imported from other package potentially conflicting with local types. If you have an RPC like:

service LibraryService {
  rpc DeleteBook(DeleteBookRequest) returns (google.protobuf.Empty)
}

The types would be:

type (
	LibraryServiceDeleteBookRequest  = connect_go.Request[library.DeleteBookRequest]
	LibraryServiceDeleteBookResponse = connect_go.Response[empty.Empty]
)

[meling]

I think this can be solved with shorter names by default and use longer names for conflict scenarios.

@emcfarlane Building on your example, I suggest instead to generate this by default:

type (
	// PingService.Ping
	PingRequest  = connect_go.Request[v1.PingRequest]
	// FailService.Ping
	FailRequest  = connect_go.Request[v1.FailRequest]
)

But of course, you can have another package x2 with an identically named message type PingRequest:

type (
	// PingService.Ping
	PingRequest  = connect_go.Request[v1.PingRequest]
	// FailService.Ping
	PingRequest  = connect_go.Request[x2.PingRequest]
)

However, I think such conflict scenarios are quite rare.

IIRC, I think there are some code generation strategies in protoc-gen-go that resolves similar conflicts by augmenting with the service name only if necessary. Thus, one possible solution can be to detect at generation time that using just the message type name would result in a conflict, and generate the following only for conflicting type(s):

type (
	// PingService.Ping
	PingServicePingRequest  = connect_go.Request[v1.PingRequest]
	// FailService.Ping
	FailServicePingRequest  = connect_go.Request[x2.PingRequest]
)

Moreover, if v1.PingRequest is used in multiple RPC methods, it should only create a single type alias for all uses. (I'm aware that buf lint disapproves...)

[emcfarlane]

Would be great to have something shorter! I don't think we can use on conflicts, as introducing a message that causes a conflict will then break all the previous implementations. It has to avoid conflicts without knowing about all other types. We could prepend the package only on external packages, so:

type (
	PingRequest  = connect_go.Request[v1.PingRequest]
	BufConnectDemoV2PingRequest  = connect_go.Request[x2.PingRequest]
)

Which makes the simple case, simple. But still causes conflicts if you name your non external type message BufConnectDemoV2PingRequest message {}. Not sure how streaming types would look too. If we use the two names:

type CumSumRequestCumSumResponse = connect_go.BidiStream[v1.CumSumRequest, v1.CumSumResponse]

This could conflict with package names like bufdev showed above, so would need a separator between the types. Also would have to have a suffix on each type to avoid message PingServiceClient {} conflicting with service PingService{} when the client is generated.

I think using message types is too tricky to cover all edge cases. But the more succinct versions would be ideal, so be great to explore more.

[akshayjshah]

I've opened #560 - it's a little different from @emcfarlane's approach. In general, it's more conservative and only generates aliases for the well-behaved subset of messages where we can safely use short names.

I also skipped generating aliases for the stream types because they get very wordy. If there's appetite for them, we can add them later.

From a user's perspective, the Ping service goes from

Ping(context.Context, *connect.Request[pingv1.PingRequest]) (*connect.Response[pingv1.PingResponse], error)
Fail(context.Context, *connect.Request[pingv1.FailRequest]) (*connect.Response[pingv1.FailResponse], error)
Sum(context.Context, *connect.ClientStream[pingv1.SumRequest]) (*connect.Response[pingv1.SumResponse], error)
CountUp(context.Context, *connect.Request[pingv1.CountUpRequest], *connect.ServerStream[pingv1.CountUpResponse]) error
CumSum(context.Context, *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error

to

Ping(context.Context, *pingv1connect.PingRequest) (*pingv1connect.PingResponse, error)
Fail(context.Context, *pingv1connect.FailRequest) (*pingv1connect.FailResponse, error)
Sum(context.Context, *connect.ClientStream[pingv1.SumRequest]) (*pingv1connect.SumResponse, error)
CountUp(context.Context, *pingv1connect.CountUpRequest, *connect.ServerStream[pingv1.CountUpResponse]) error
CumSum(context.Context, *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error

[meling]

I'm in favor of #560. However, how bad is the streaming options considered; it wasn't clear to me. Could you write out the examples?

Wouldn't this work?

Sum(context.Context, *connect.ClientStream[pingv1.SumRequest]) (*pingv1connect.SumResponse, error)
CountUp(context.Context, *pingv1connect.CountUpRequest, *connect.ServerStream[pingv1.CountUpResponse]) error
CumSum(context.Context, *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error

to

Sum(context.Context, *pingv1connect.SumStream) (*pingv1connect.SumResponse, error)
CountUp(context.Context, *pingv1connect.CountUpRequest, *pingv1connect.CountUpResponseStream) error
CumSum(context.Context, *pingv1connect.CumSumBidiStream) error

or

Sum(context.Context, *pingv1connect.SumClientStream) (*pingv1connect.SumResponse, error)
CountUp(context.Context, *pingv1connect.CountUpRequest, *pingv1connect.CountUpServerStream) error
CumSum(context.Context, *pingv1connect.CumSumBidiStream) error

If client and server streams must be distinguishable.

[akshayjshah]

@meling Generating aliases for the stream types isn't infeasible, it's just more complex - there are more naming decisions and potential conflicts to worry about, so there's more to debate. This is especially true if we proceed with the approach in #560, where the aliases are derived from the message names rather than the service + method names.

I wasn't confident that we'd reach agreement even for the simpler unary aliases, so I wanted to push any discussion of streaming to a future PR :)

[akshayjshah]

#560 doesn't have majority approval either 🤣

In the discussion, @mattrobenolt made a good suggestion to consider using custom options: #560 (comment)

Pursuing that approach is probably the next step here.