Protobuf: generator: export `oneof` interface types

Created on 29 Nov 2016  Â·  9Comments  Â·  Source: golang/protobuf

We currently generate unexported interface types for oneof fields.
(https://github.com/golang/protobuf/blob/master/protoc-gen-go/generator/generator.go#L2027)

That makes it extremely difficult to write helper-functions that accept oneof values as parameters and use them to set fields (e.g. to generate skeleton messages with the oneof field set within a couple layers of nesting).

Beyond that, it's awkward and confusing to have unexported types in the exported documentation for the package — users need to be familiar with the details of our oneof implementation to make sense of it.

We should export the oneof interface types.

Most helpful comment

I solved this issue by creating a new Go file in the same package as generated code with the following content:

type MessageNameMyField = isMessageName_MyField

Yep, that's type alias. IMO, that's one of the few valid reasons to use it.

But I really hope #588 will be merged soon.

All 9 comments

Because isMessageName_MyField visibility is package only, it makes a code operating on it unnecessary complex, e.g. when one want to return a isMessageName_MyField she must return interface{}, cast it to the right underlying type of value and just then can assign to MyField.

Also a function which normally would have a signature:
func (IsMessageName_MyField), right now must take any value: func (interface{}).

The change is trivial, but I cannot find any instruction how to run tests against it (I expect them to fail):
https://github.com/orian/protobuf/commit/f72201aca5615112c2793af0b656979e062b535f

Hitting the same issue when attempting to operate on union types generically. Exporting would help a lot with the use case of a message containing a header and body, with the body being a oneof and the header being generated based off other conditions independent of the body type.

Can we get a response to this issue? Not exporting the interface or the methods that satisfy said interface makes for some really terrible code with no conceivable benefit. It blows my mind that some of the exported helper methods return this unexpected interface, in direct contradiction to Go conventions (see: golint), but for some reason the interface was not exported.

This seems reasonable. Want to take this on @bcmills?

I don't have the bandwidth for it at the moment, but it should be a minor (i.e., fully backward-compatible) change.

@dsnet I can prepare change if you can instruct me how this package is tested.

How does Google deal with it internally? I guess a need for passing a type somewhere exist.

I solved this issue by creating a new Go file in the same package as generated code with the following content:

type MessageNameMyField = isMessageName_MyField

Yep, that's type alias. IMO, that's one of the few valid reasons to use it.

But I really hope #588 will be merged soon.

Copying my comment from #588:

I don't think we should do this.

In the protobuf data model, there is no such thing as a single "oneof field" that aggregates the individual components of a oneof. Instead a oneof is just a set of mutually exclusive fields which behave just like non-oneof fields, except setting one implicitly clears all the others.

Exporting this type would introduce a concept into the Go generated code which has no equivalent in most (all?) other languages' protobuf implementions. I don't think that's a good idea.

In addition, the current Go implementation oneofs leaks too many details of the implementation to the user. I think we can do a lot to improve that; in particular, adding setters for oneof fields would be a big help. This PR goes in the wrong direction, by exposing even more of those implementation details.

Closing per @neild's comment: https://github.com/golang/protobuf/issues/261#issuecomment-435081813

Was this page helpful?
0 / 5 - 0 ratings