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

Cannot generate pointer fields using casttype with syntax proto3 #421

Closed
silasdavis opened this issue Jul 4, 2018 · 7 comments
Closed

Comments

@silasdavis
Copy link

silasdavis commented Jul 4, 2018

I would like to have a pointer generated with gogoproto.casttype using proto3 syntax, but when I have:

syntax = 'proto3';

message PermArgs {
    uint64 PermFlag = 1 [(gogoproto.casttype) = "PermFlag"];
}

I get a PermFlag not a *PermFlag field. I also tried setting (gogoproto.nullable) = true but that does not work. Having looked at the test case https://github.com/gogo/protobuf/blob/master/test/casttype/casttype.proto I found if I use proto2 syntax:

syntax = 'proto2';

message PermArgs {
    optional uint64 PermFlag = 1 [(gogoproto.casttype) = "PermFlag"];
}

With the optional keyword I get a *PermFlag generated. My understanding once that all fields are optional in proto3 so I was hoping I'd be able to control the generation of a pointer field using nullable with the default being to make a pointer.

@awalterschulze
Copy link
Member

I am sorry, but I have some bad news.

Yes this is true proto3 does not support pointers for native types, even for casttypes, since it supports sensible defaults, in place of nil values.

tl;dr
nullable=false is a feature for proto2 native types and most other types.
Making nullable=true work for proto3 native types, is quite a lot of work.
Personally I am not volunteering to do this.

nullable=false was necessary to remove pointers to get real performance (less garbage collection and memory allocation) and more go like structures generated from protobufs, but proto3 fixed this by giving fields sensible defaults, just like in Go. So this feature is rarely useful in proto3.

With the proto3 protocol nullable=true won't allow you to actually transfer null over the wire. So it also breaks the protobuf protocol in a similar way to how nullable=false does. But does not provide any speed benefit. The only reason to do this is to generate the go struct you actually need, which I am all for, but the amount of work, for this single benefit is quite high.

@silasdavis
Copy link
Author

Agree with your analysis of the only benefit being getting the Go structure that one wants.

It would appear to work for me to just use a syntax = 'proto2' file where I need nullable native types (not very often. Are there any caveats I should be aware with with this? Just for context my real life proto file is:

// Needed to proto2 rather than proto3 to get pointer field for PermArg
syntax = 'proto2';

option go_package = "github.com/hyperledger/burrow/permission/snatives";

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

package snatives;

// Enable custom Marshal method.
option (gogoproto.marshaler_all) = true;
// Enable custom Unmarshal method.
option (gogoproto.unmarshaler_all) = true;
// Enable custom Size method (Required by Marshal and Unmarshal).
option (gogoproto.sizer_all) = true;
// Enable registration with golang/protobuf for the grpc-gateway.
option (gogoproto.goproto_registration) = true;
// Enable generation of XXX_MessageName methods for grpc-go/status.
option (gogoproto.messagename_all) = true;

message PermArgs {
    option (gogoproto.goproto_unrecognized) = false;
    option (gogoproto.goproto_stringer) = false;
    optional uint64 PermFlag = 1 [(gogoproto.casttype) = "github.com/hyperledger/burrow/permission.PermFlag", (gogoproto.nullable) = false];
    optional bytes Address = 2 [(gogoproto.customtype) = "github.com/hyperledger/burrow/crypto.Address"];
    optional uint64 Permission = 3 [(gogoproto.casttype) = "github.com/hyperledger/burrow/permission.PermFlag"];
    optional string Role = 4;
    optional bool Value = 5;
}

@awalterschulze
Copy link
Member

Yes I think proto2 fits your usecase better, but if the rest of your codebase is proto3 that might be a problem.

Or if one day google decides proto2 is done, then that might also be a problem.

@awalterschulze
Copy link
Member

Do you think I can close this issue?

@silasdavis
Copy link
Author

Yeah the rest is proto3, I've managed to use compatible proto2 declarations though. I feel like it is definitely a nice to have... Sometimes the ergonomics are better having pointers to primitive types so I think it's within the remit of gogo's aims. Having said that if the payoff isn't there given the complexity to implement then I think you can close unless someone wants to work on it. It's not a big enough deal for me. Could always revisit the eventualities you describe. Also I can wrap a type in a message with a boolean flag for it to act as a nullable quantity, which does have the advantage of being a bit more explicit.

@awalterschulze
Copy link
Member

If you are willing to wrap your types in a message then you don't need the boolean flag. In proto3 you can tell whether a message is set or not. That is also why there is well known types for StringValue, BoolValue, etc.

I think this is what you want?
golang/protobuf#414

@silasdavis
Copy link
Author

Yeah it is really thanks for the pointer. I can perhaps work with wrappers, or repeated has some kind of nice properties as a maybe type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants