twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.05k stars 328 forks source link

Compatibility with `gogo/protobuf` #359

Closed ditsuke closed 1 year ago

ditsuke commented 2 years ago

I want to use Twirp with structures generated by gogo's gofast compiler for idiomatic go structs. As of v8.1.2, twirp generated code seems to not be compatible with gogo's gofast compiler (twirp seemed to have a compatibility test as of v7.2.0). Are there any plans for gofast compatibility and are there any generator options I'm missing?

rhysh commented 2 years ago

What commands did you run, and what failing output did you see?

I tried downloading the latest version of https://github.com/gogo/protobuf (v1.3.2) and of Twirp, and using libprotoc 3.21.3 and gogo's gofast to regenerate the code in the ./example directory. After an ad-hoc go mod init, I ran go build ./... and got errors like this:

example/service.twirp.go:118:72: cannot use in (variable of type *Size) as type protoreflect.ProtoMessage in argument to doProtobufRequest:
    *Size does not implement protoreflect.ProtoMessage (missing ProtoReflect method)

Is that what you're reporting?


That function call uses arguments of type "google.golang.org/protobuf/proto".Message value, which is an alias to "google.golang.org/protobuf/reflect/protoreflect".ProtoMessage, which is:

type ProtoMessage interface{ ProtoReflect() Message }

That's a change from "github.com/golang/protobuf/proto".Message, which is https://pkg.go.dev/google.golang.org/protobuf@v1.28.0/runtime/protoiface#MessageV1:

type MessageV1 interface {
    Reset()
    String() string
    ProtoMessage()
}

The code that gofast generates appears to implement protoiface.MessageV1, so it may be usable with the deprecated github.com/golang/protobuf/proto package, but not with the newer google.golang.org/protobuf/proto package.

func (m *Size) Reset()         { *m = Size{} }
func (m *Size) String() string { return proto.CompactTextString(m) }
func (*Size) ProtoMessage()    {}

It looks like Twirp v8 included a move from the deprecated github.com/golang/protobuf module to its successor google.golang.org/protobuf in https://github.com/twitchtv/twirp/pull/304. It looks like github.com/gogo/protobuf can only interop with the deprecated module. So this doesn't look Twirp-specific to me. Maybe the gogo maintainers have ideas on how to interop with the current version of Google's protobuf+Go support? But I could be missing something.

Twirp v8 requires that its generated code can use a message type Msg like this:

package example

import "google.golang.org/protobuf/proto"

var _ proto.Message = (*Msg)(nil)
wmatveyenko commented 1 year ago

Hi @ditsuke, just checking in here. Since we view Google as the main vendor of protobuf, we want to stay up to date with that implementation. If gogo/protobuf is not keeping up with those releases, we would prioritize tracking the canonical implementation of protobuf from Google.