protoc-gen-yarpc-go doesn't handle gogo/protobuf well-known types import paths

[amckinney]

The protoc-gen-yarpc-go plugin fails to respect the import paths for the well-known types when they are included by gogo/protobuf. Given the following foo.proto definition:

syntax = "proto3";

package foo;

import "google/protobuf/empty.proto";

service Foo {
  rpc Bar(google.protobuf.Empty) returns (google.protobuf.Empty);
}

The generated rps.pb.yarpc.go file references the appropriate gogo/protobuf import path, but fails to resolve the correct package name. The generated output for the above file is:

// Code generated by protoc-gen-yarpc-go
// source: rpc/rps/foo.proto
// DO NOT EDIT!

package foo

import (
	"context"
	"io/ioutil"
	"reflect"

	"github.com/gogo/protobuf/proto"
	"github.com/gogo/protobuf/types"
	"go.uber.org/fx"
	"go.uber.org/yarpc"
	"go.uber.org/yarpc/api/transport"
	"go.uber.org/yarpc/encoding/protobuf"
)

var _ = ioutil.NopCloser

// FooYARPCClient is the YARPC client-side interface for the Foo service.
type FooYARPCClient interface {
	Bar(context.Context, *empty.Empty, ...yarpc.CallOption) (*empty.Empty, error)
}

...

Specifically, the reference to *empty.Empty is not correct - the well-known types are declared in package types when we use gogo/protobuf, i.e. github.com/gogo/protobuf/types.

The suspect code is likely here, since this is how we determine what package name to use. The empty.proto well-known type declares its go_package as option go_package = "github.com/golang/protobuf/ptypes/empty";, so the logic is choosing empty as the package name instead of types.

[peats-bond]

Just wanted to elaborate: users following the prototool lint rules will not see this issue.

There is a lint rule that requires Message types for the request/response for backwards compatibility. The well-known types would not be used for an RPC request/response which avoids this issue entirely.

That being said, this is certainly a bug in the plugin. However, requiring properly linted .proto files makes this a bit of an edge case.

[bufdev]

I only just saw this - I’m sure this has been handled in some form by now, but note this is by design/is working as intended to match the golang/protobuf and gogo/protobuf protoc plugins - back in the day, there was no generated WKT code in golang/protobuf or gogo/protobuf, and it got added on over time. Users might not want the plugins to auto-interpret where this code lives as they may have their own generated stubs, especially with gogo/protobuf and custom options. The way this is solved in the golang/protobuf and gogo/protobuf plug-ins is through using modifiers, which as @AlexAPB alludes to, is automatically done in Prototool as an example.

In lieu of special-casing protoc-gen-yarpc-go as relative to other golang protoc plugins, it handles import modifiers in the same manner. Of note, grpc-gateway also works this way (unless there’s been a recent update).

Edit: above still is a bit of a history lesson, but I see the issue now, should have read more closely. Was the above generated with modifiers? I’m surprised by this one - happy to help.