Since dev merge, code no longer compiles, lots of strange changes

[dotwaffle]

Over the weekend, it would appear that much of the generated code no longer compiles, almost all of which is related to the packages golang.org/x/net/context (I'm using Go 1.10.3) or google.golang.org/grpc not being used:

./cluster.pb.go:18:8: imported and not used: "golang.org/x/net/context" as golang_org_x_net_context
./cluster.pb.go:19:8: imported and not used: "google.golang.org/grpc" as google_golang_org_grpc

If I run goimports -w on the generated files it does fix these errors, but there appear to be a large number of strange changes. for instance:

• All the fileDescriptor names moved from (e.g.) fileDescriptorCluster to fileDescriptor_cluster_c98421a8c26a96a1 (making diffs a lot less readable)
• There are a number of strange package import aliases, such as import strings "strings" and import reflect "reflect" etc -- do these really need to be aliased?
• Some of the import aliases have been renamed to strange unwieldly strings, e.g. import sortkeys "github.com/gogo/protobuf/sortkeys" is now import github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" -- is that deliberate?
• Using gogofaster or gogoslick there are now a lot of XXX_ prefixed exported variables in structs, such as XXX_sizecache and XXX_NoUnkeyedLiteral, and the re-appearance of methods such as XXX_Unmarshal, XXX_Marshal, XXX_Merge, XXX_Size, XXX_DiscardUnknown etc.

I can't pinpoint where these issues began within the dev merge, but something is definitely substantially different compared to Friday. Should I be following some particular branch for a stable version?

[awalterschulze]

Yes this was a HUGE change.
These are all changes started by golang/protobuf and we merged with LOADS of effort, to keep the two repositories as compatible as possible.

See https://github.com/golang/protobuf/releases/tag/v1.1.0
Gogoprotobuf has made similar release to stay in sync with golang/protobuf
https://github.com/gogo/protobuf/releases/tag/v1.1.0
and a release before that, that you can pin to until you are ready for these changes https://github.com/gogo/protobuf/releases/tag/v1.0.0

Could you please provide a small example that reproduces your grpc problems?
Thanks for reporting and providing a work around.

Do the fileDescriptor renames bother you in the following diffs as well?

The import aliases could probably be fixed, but it is always hard, for some reason.

The XXX fields are unfortunate, but something that golang/protobuf deemed necessary. I don't think we ever had XXX_Marshal methods, so I don't know how they are reappearing?

Again thanks for reporting.

[dotwaffle]

Thanks Walter. It looks like my tests still work, so the functionality difference is minimal if not zero. The renaming of things such as fileDescriptor can be lived with, it's just that if the proto files change, every single Descriptor and EnumDescriptor method will change due to the new name!

The main issue is the addition of the additional XXX_ methods which clutter up the godoc output:

@@ -338,14 +571,38 @@ func (m *GetAllRouteClustersRequest) GetAssociationsToExpand() string {
 }

 type GetAllRouteClustersResponse struct {
-       RouteCluster []*RouteCluster `protobuf:"bytes,1,rep,name=route_cluster,json=routeCluster" json:"route_cluster,omitempty"`
+       RouteCluster         []*RouteCluster `protobuf:"bytes,1,rep,name=route_cluster,json=routeCluster" json:"route_cluster,omitempty"`
+       XXX_NoUnkeyedLiteral struct{}        `json:"-"`
+       XXX_sizecache        int32           `json:"-"`
 }

 func (m *GetAllRouteClustersResponse) Reset()      { *m = GetAllRouteClustersResponse{} }
 func (*GetAllRouteClustersResponse) ProtoMessage() {}
 func (*GetAllRouteClustersResponse) Descriptor() ([]byte, []int) {
-       return fileDescriptorCluster, []int{9}
+       return fileDescriptor_cluster_c98421a8c26a96a1, []int{9}
 }
+func (m *GetAllRouteClustersResponse) XXX_Unmarshal(b []byte) error {
+       return m.Unmarshal(b)
+}
+func (m *GetAllRouteClustersResponse) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
+       b = b[:cap(b)]
+       n, err := m.MarshalTo(b)
+       if err != nil {
+               return nil, err
+       }
+       return b[:n], nil
+}
+func (dst *GetAllRouteClustersResponse) XXX_Merge(src proto.Message) {
+       xxx_messageInfo_GetAllRouteClustersResponse.Merge(dst, src)
+}
+func (m *GetAllRouteClustersResponse) XXX_Size() int {
+       return m.Size()
+}
+func (m *GetAllRouteClustersResponse) XXX_DiscardUnknown() {
+       xxx_messageInfo_GetAllRouteClustersResponse.DiscardUnknown(m)
+}
+
+var xxx_messageInfo_GetAllRouteClustersResponse proto.InternalMessageInfo

I can live with these as the interface seems to be consistent, it's just noise in the documentation. I've added a goimports -w ./ line into my generation script for the time being to mitigate the compile errors.

Thanks again for looking into this!

[awalterschulze]

I would love to get rid of your goimports -w ./ as part of your build script if you could give us a small example that reproduces your problem?

I am also not a fan of the XXX methods, these come from golang/protobuf. I would advise to complain to them, because it is a bit outside of my power to get rid of these. Or maybe I made the wrong decision to stay close to the golang/protobuf implementation in the first place :(

[dotwaffle]

proto.zip
Use the above with the following protoc invocation:

% protoc \
        -I=. \
        -I="$GOPATH/src/github.com/gogo/protobuf/protobuf" \
        -I="$GOPATH/src" \
        --gogoslick_out=\
Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,\
Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/empty.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/field_mask.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/struct.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/wrappers.proto=github.com/gogo/protobuf/types,\
import_path=test_stuff,\
plugins=grpc:\
. \
proto/audit.proto

That will generate the audit.pb.go file that has the bad imports -- I've not included our entire set of proto files, but this is a subset (obfuscated slightly) that shows the problem.

[white-bear]

I have same issue. My generated files now have broken imports

import golang_org_x_net_context "golang.org/x/net/context"
import google_golang_org_grpc "google.golang.org/grpc"

They are not used in file - code contains short aliases like grpc and context:

var _ context.Context
var _ grpc.ClientConn

[bt]

I am also having the same issue; took me a while to realise this was related to the new version that was pushed since 2 days ago.

[mattkanwisher]

Running into same issue with the GPC and context imports

[bt]

Does anyone know how to pull the previous version with go get?

[taras-zak]

@bt maybe this helps:

git clone https://github.com/gogo/protobuf $GOPATH/src/github.com/gogo/protobuf
cd $GOPATH/src/github.com/gogo/protobuf && git checkout v1.0.0 && go install ./...

[bt]

@taras-zak Ah didn't know I could do it that way, thank you!

[awalterschulze]

@jmarais is working on a fix #429

[awalterschulze]

Could everyone check whether master fixes the problem?

[bt]

Works for me!

[mattkanwisher]

yup looks good thanks!

[awalterschulze]

Release pushed https://github.com/gogo/protobuf/releases/tag/v1.1.1

[awalterschulze]

Can I close this issue?

Maybe the rest of the issues should each have their own separate issue?

[dotwaffle]

@awalterschulze I can confirm everything compiles without error now. The following new fields / methods are still exposed:

type Thing struct {
        [...]
        XXX_NoUnkeyedLiteral struct{}
        XXX_sizecache        int32
}
func (m *Thing) XXX_Unmarshal(b []byte) error {
func (m *Thing) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
func (dst *Thing) XXX_Merge(src proto.Message) {
func (m *Thing) XXX_Size() int {
func (m *Thing) XXX_DiscardUnknown() {

I can open a new issue if you want me to -- they don't affect the functionality as far as I can tell, it's just noise in godoc.

Happy to close the issue if you are!

[awalterschulze]

The XXX methods is something you should probably take up with golang/protobuf.
They have interesting reasons for this and removing these makes gogo/protobuf more imcompatible with golang/protobuf.

The XXX fields are something that might be removable. XXX_NoUnkeyedLiteral is easy to remove, but XXX_sizecache will be a little tougher. This I see more as a feature request than a bug though, if that makes sense. So if you want structs without XXX fields, I would ask that you rather create a new issue that references this one. I would want to gather a few users, in that issue, that need this feature before going ahead to implement it though, but I can think of some good reasons, even though I am not a user anymore.

[awalterschulze]

I Hope closing this makes sense.

[dotwaffle]

Thanks @awalterschulze, very much appreciated. Given XXX_sizecache implies it should be unexported, I'm surprised it's not xxx_sizecache! I'll read up on the upstream reasoning for the XXX methods, and open a feature request if needed -- it appears I misread the documentation, I believed all the XXX_ prefixed fields/methods would be removed in gogofaster and gogoslick but I see now it only mentions XXX_unrecognized so my mistake!

[awalterschulze]

Yes removing other fields would be another feature, like removing XXX_unrecognized.

It is not xxx_sizecache, since reflect cannot access private fields and the proto library needs to access this field. The XXX methods are also called from the proto library and that is why they need to be exported as well.