Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc-gen-go: XXX_ fields break code back compatibility #607

Closed
sitano opened this issue May 15, 2018 · 4 comments
Closed

protoc-gen-go: XXX_ fields break code back compatibility #607

sitano opened this issue May 15, 2018 · 4 comments

Comments

@sitano
Copy link

@sitano sitano commented May 15, 2018

having []byte XXX_ fields in generated code prevents those structs to be able to be compared by value.

package main

import (
	"fmt"
)

type Hash struct {
	XXX_NoUnkeyedLiteral struct{} `json:"-"`
	XXX_unrecognized     []byte   `json:"-"`
	XXX_sizecache        int32    `json:"-"`
}

func main() {
	fmt.Println(Hash{} == Hash{})
}

This breaks an existent code.

Another argument to the issue #276

Even if the []byte field will be removed, those third-party states being embed into the structs makes it pointless.

@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented May 17, 2018

This breakage is really unfortunate, but it's a consequence of Go protobufs being compliant with the updated proto3 specification. See #594 (comment).

Another argument to the issue #276

This is orthogonal to #276, which is about removing XXX fields from the exported API. The implementation will still require unexported fields of some form, some of which may still be incomparable.

@dsnet dsnet closed this May 17, 2018
@sitano

This comment has been minimized.

Copy link
Author

@sitano sitano commented May 28, 2018

Just to add more on what these changes broke.

  1. Generated types can NOT be used as map keys.
  2. New generated code does not import go packages for imported proto types, so it just does not compile.
@tandr

This comment has been minimized.

Copy link

@tandr tandr commented Aug 2, 2018

@dsnet Then I would say that an update to the proto3 spec broke it.

First (many moons ago) when proto2 was introduced, Optional Fields were all the rage and unrecognized fields just sit quietly in reserved by them "slots". Then proto3 came in, and put it as "optional is no more - everything is optional" (and broke separation of "not supplied" and "default" by the way). Now, by introducing XXX_ mess it pretty much went back, just storing unrecognized fields under XXX_. Again, breaking perfectly normal code that was comparing proto.Timestamp, and other simple generated structs.

proto generated code in the current form is unusable for anything but storage - with no comparison, simple, small, and fast common structures used between different components neither not small nor fast anymore.

@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented Aug 2, 2018

Please see my comment here (#594 (comment)).

@sitano sitano mentioned this issue Aug 23, 2018
msteffen added a commit to pachyderm/pachyderm that referenced this issue Dec 6, 2018
Necessary due to updates in github.com/golang/protobuf. See golang/protobuf#607
msteffen added a commit to pachyderm/pachyderm that referenced this issue Dec 6, 2018
Necessary due to updates in github.com/golang/protobuf. See golang/protobuf#607
msteffen added a commit to pachyderm/pachyderm that referenced this issue Dec 6, 2018
Necessary due to updates in github.com/golang/protobuf. See golang/protobuf#607
Nick-Harvey pushed a commit to pachyderm/pachyderm that referenced this issue Jan 24, 2019
Necessary due to updates in github.com/golang/protobuf. See golang/protobuf#607
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.