Consider exporting oneof interface

[steeling]

Is your feature request related to a problem? Please describe. Yes, I have a complicated set of oneof's, which are all subfields. I'd like to create a set of functions that return an instance of that oneof, but I can't reference the interface, meaning I have resolve each type expliclity.

Describe the solution you'd like Simply export the interface generated for a oneof.

Describe alternatives you've considered N/A

Additional context Following scenario:

message Temp {
  oneof field {
    // In my actual example these are all submessages, but this works too.
    string field1 = 1;
    bool field2 = 2;
  }
}

Generates the following:

type Temp struct {
  Field isTemp_Field // unexpored interface.
}

I'd like to have a func that maps from some types to the a Temp object.

func myConversion() isTemp_Field { // can't reference unexported field
  ...
}

// Temp.Field = myConversion()

[dsnet]

I'm not sure exactly where, but this has been discussed quite a few times at various times in the past.

From a type theory perspective: The generated Go API is trying to match the protobuf data model, where the model treats oneofs as essentially a constraint on a set of message fields. Oneofs are not a first-class type that you can hold onto as a value. For this reason, the wrapper type is not exported in the generated API to better match the protobuf language. Note that the C++ implementation (which is the de-facto representation of the data model) has no ability to represent a oneof as a concrete union value.

From a code-generation perspective: We already have a hard time generating declarations with names that don't conflict with each other. If we were to export the oneof wrapper type, that's yet another named type that we need to make sure doesn't conflict with other generated types in the same package. Most names that we could choose is likely going to conflict somewhere with pre-existing code.

[steeling]

Thanks for the reply! A couple thoughts below, although not necessarily disagreeing with yours:

My understanding (which could be totally off), is that the next version of golang/protobuf is looking at not exporting any fields, and completely relying on getters/setters. If that's the case I think it would make this requirement even more necessary, and maybe we could bring this in prior to the other changes?

for the naming, it's ugly, but could be switched to:

message Temp {
  oneof myfield { // interface named Temp_Myfield
      string mystring = 1; // type named Temp_Myfield_Mystring
   }
}

Although would hurt backwards compatibility.

[neild]

...would make this requirement even more necessary...

I'm not certain which requirement you're referring to, but nothing about an opaque representation of messages (no direct field access, only getters/setters) requires exporting a oneof interface type. To the contrary, it makes the interface type unnecessary, since fields which are part of a oneof are set just like any other: m.SetField1(v).

[dsnet]

Closing as "working as intended".

[storyicon]

I'm not sure exactly where, but this has been discussed quite a few times at various times in the past.

From a type theory perspective: The generated Go API is trying to match the protobuf data model, where the model treats oneofs as essentially a constraint on a set of message fields. Oneofs are not a first-class type that you can hold onto as a value. For this reason, the wrapper type is not exported in the generated API to better match the protobuf language. Note that the C++ implementation (which is the de-facto representation of the data model) has no ability to represent a oneof as a concrete union value.

From a code-generation perspective: We already have a hard time generating declarations with names that don't conflict with each other. If we were to export the oneof wrapper type, that's yet another named type that we need to make sure doesn't conflict with other generated types in the same package. Most names that we could choose is likely going to conflict somewhere with pre-existing code.

@dsnet sorry, I don't think this reason is sufficient.

1. this lame "trying to match" on the exportability of the oneof field has no real value, but affects the developer.
2. if the same name in the same package, even if it is not exported, there will be a naming conflict, I don't understand how you say exportability affects the naming conflict. But even if there is a possible naming conflict, we should solve it instead of avoiding it.
3. this issue will still be brought up again and again in the future.

[storyicon]

In fact, istio's pilot has a lot of lengthy function code that can be attributed to this "non-exportable" design: https://github.com/istio/istio/blob/master/pilot/pkg/networking/core/v1alpha3/route/route.go

[kentquirk]

I'm sure I'm shouting into the wind, but this becomes really obvious when trying to work with OTLP. There is a Value field in the protocol and it's literally impossible to write a function that parses a file and returns something assignable to the Value field, since the type of the exported field is not itself exported.

I don't think that the cited reason justifies having an exported field with a non-exported type. That's just frustrating API design.

[EricRabil]

Sorry, but failing to export a pillar type from Protobuf to Go means this generator does not work as intended. It makes it difficult to write functions with type signatures that accept any of the oneof members, making it difficult to use oneof in the first place, making it difficult to choose Protobuf to power code generation. Workaround: https://github.com/golang/protobuf/issues/261#issuecomment-430496210

[puellanivis]

oneof is not a type in protobuf though. The type that exists in Go is an artifact of trying to shoe-horn oneof into Golang.

If you find that this is making it difficult to use oneof, I agree. And this is one of the pillars I hold towards cheekily said, “oneof considered harmful”. The semantics of oneof do not fit at all well with the semantics of Go, which is why I recommend always ignoring that oneof exists, and implementing the semantics from the raw fields themselves.

[steeling]

First, I think that the aforementioned reason of naming conflicts is fairly moot. There's already no conflict on the unexported version of the name, so simply upper-casing it would be sufficient.

On the constraint vs type argument, I feel that arguing oneof is not a type based on type theory instead of heeding the community's desire to use a oneof as a type feels a little bit rigid. Do we prefer to be theoretical, or do we prefer to be practical?

Also, Interfaces in go are barely "types" (in the type theory sense) themselves, and the implicit type implementation furthers that notion. They're defined as a "named collection of method signatures". Can you please clarify why you think exporting the interface of oneof doesn't fit that definition?

At the very least what about adding a Set<oneof> method? There's already a Get<oneof> method. You wouldn't even have to export the type!

[puellanivis]

The argument is not that oneof is not a type in terms of type theory. The argument is that protobuf oneofs do not exist in the encoded formats.

For the:

message SampleMessage {
  oneof test_oneof {
    string name = 4;
    SubMessage sub_message = 9;
  }
}

We already generate a SetName and a SetSubMessage. We do not generate a SetTestOneof because it does not semantically exist in protobuf. A oneof is simply a bit of syntactic sugar ontop of the protobuf where if you set one field, it clears all the other fields.

Note, this is entirely within the same parameters of how they are handled in C++:

SampleMessage message;
message.set_name("name");
CHECK(message.has_name());
message.mutable_sub_message();   // Will clear name field.
CHECK(!message.has_name());`

Note as well the additional text:

… there's no way to know if an unknown field on the wire is a member of the oneof.

Again, this is because the oneof does not actually exist in the wire or JSON format. This isn’t about being theoretical, or practical. This is the way the standard is defined, and we must adhere to that.

[jroper]

The type is already exposed - it's exposed as the type of the field. It's just not exported, so you can't use it. Any argument that starts with "we don't want to expose oneof as a type" makes no sense, because oneof is already exposed as a type. Either implement it as a type, or don't, but don't implement it as a type and then say it's not a type.

[puellanivis]

There are two semantic spaces in this issue: Golang, and protobuf. Protobuf exists as a semantic space apart from any implementing language. And each language has to find some way to coax that semantic into their native semantics.

Plainly put, on the wire, oneof does not exist. Period. In none of protowire, protojson, or prototext is it encoded.

[jroper]

Plainly put, on the wire, repeated does not exist either. Or perhaps more correctly, the concept of non repeated doesn't exist on the wire. Fields can have 0 or more values, regardless of what the schema is. Yet, no one questions mapping repeated fields to slices, and non repeated fields to unary values in Go, even though these do not exist as distinct concepts on the wire. Why? Because the concept of repeated and unary fields in the schema maps idiomatically to the concept of slices and unary fields in Go.

Go may not have first level support for disjoint union types, but it is able to idiomatically represent them using its type system.

But that's not even my criticism here. My point is, if we follow your argument, then the current way that Go maps protobuf is completely wrong. Protobuf maps oneofs to a single field Go. That doesn't match what's on the wire. You can't say "we won't do that because it doesn't match what's on the wire" when what you're already doing doesn't match what's on the wire. It doesn't make sense.

[puellanivis]

I’m sorry that you’re having difficulty understanding what I’m expressing. Golang cannot simply represent each field of the oneof as a separate field, because then it would not automatically clear the other fields if it is set.

C++ protobuf does not generate a set_oneof_example() only clear_oneof_example() and oneof_example_case(). neither does Java generate a setOneofExample() but a getOneofExampleCase() and clearOneofExample().

This type does not exist in any other language, it exist in Golang because it is an artifact of the only way possible to enforce the oneof restriction in Golang.

[texuf]

Here you go, just run this after generating protobufs

package main

import (
    "fmt"
    "go/ast"
    "go/parser"
    "go/token"
    "os"
    "strings"
)

func main() {
    inputFileName := "../protocol/protocol.pb.go"
    outputFileName := "../protocol/extensions.pb.go"

    fset := token.NewFileSet()
    file, err := parser.ParseFile(fset, inputFileName, nil, parser.ParseComments)
    if err != nil {
        fmt.Println("Error parsing file:", err)
        return
    }

    var oneOfTypes []string
    ast.Inspect(file, func(n ast.Node) bool {
        switch x := n.(type) {
        case *ast.GenDecl:
            if x.Tok == token.TYPE {
                for _, spec := range x.Specs {
                    typeSpec := spec.(*ast.TypeSpec)
                    if structType, ok := typeSpec.Type.(*ast.StructType); ok {
                        for _, field := range structType.Fields.List {
                            if field.Tag != nil && strings.Contains(field.Tag.Value, "protobuf_oneof") {
                                fieldType, ok := field.Type.(*ast.Ident)
                                if ok {
                                    oneOfTypes = append(oneOfTypes, fieldType.Name)
                                }
                            }
                        }
                    }
                }
            }
        }
        return true
    })

    outputFile, err := os.Create(outputFileName)
    if err != nil {
        fmt.Println("Error creating output file:", err)
        return
    }
    defer outputFile.Close()

    packageName := file.Name.Name
    outputFile.WriteString(fmt.Sprintf("package %s\n\n", packageName))

    for _, oneOfTypeName := range oneOfTypes {
        exportedTypeName := strings.Title(oneOfTypeName)
        outputFile.WriteString(fmt.Sprintf("type %s = %s\n", exportedTypeName, oneOfTypeName))
    }

    fmt.Println("Generated extensions.go with oneOf exported types.")
}

[jordan-bonecutter]

Nothing to add to the conversation other than please export this type. Exported/unexported makes it no more/less idiomatic, it will only save time and mild annoyance.

[puellanivis]

It’s never been about what is idiomatic to Go. It’s been about what is proper given the semantics of Protobuf.

[jordan-bonecutter]

How does the visibility of a field's type adhere more/less closely to protobuf semantics? oneof isn't a type, it's a constraint on the message so the type of the autogenerated field is completely irrelevant to protobuf semantics.

Even still, some languages don't allow you to make such a design decision (C# for example doesn't allow public fields to have private types) and for good reason, it makes anything but basic usage difficult. I ran into this because I was building a telemetry API which assists in grabbing specific protobuf fields and running a function on them. The problem is that I cannot write a function for a oneof field because the type is private. I can see a good argument for either a private field + private type which is only available through a getter or a public field and public type. In between makes no sense. It's hard to disagree that the current state is painful and makes oneof fields a bummer to use.

[texuf]

Agree 100% this is very frustrating. I'm running the code I posted earlier in the thread every time we export protobufs and it's working well for me. Maintainers, I wish you would take some time and implement a oneOf in Swift and Typescript, which allow us to use the typed payloads checked at compile time. It's so nice.

And for a second if I can grovel and plead... It's one thing to implement the library however you choose, but it's another to dictate how other people can and can't use it. We're all professionals here, we don't want to make your job harder, but by changing few lines of code you could make a lot of people's jobs a little bit easier.

[puellanivis]

How does the visibility of a field's type adhere more/less closely to protobuf semantics?

A good question.

oneof isn't a type, it's a constraint on the message so the type of the autogenerated field is completely irrelevant to protobuf semantics.

So, you can answer your own question. The type doesn’t actually exist. It’s merely an artifact of fitting the constraint defined by protobuf semantics into Golang.

Unexporting the field and then giving receiver methods to access it doesn’t make any sense, because 1) Golang eschews accessor methods, and A) the proper way to switch/case on a set of different types is with a type-switch.

Again, going back to other implementations: Java provides getOneofExampleCase and C++ provides oneof_example_case() both of which return “fictional” enums allowing one to switch on which field is set. They have to do this because their languages do not support type-switch, so this is the only way to fit the protobuf semantics into their own language semantics. But Golang offers as per point A above, a type-switch, so we don’t need a receiver method that returns an invented value just to allow us to know which field we should read.

[jordan-bonecutter]

Agreed, type switch is the way to do this in go. But all of the things you brought up can still be said if the type were exported. The value will just be easier to ingest, it can be passed around in functions without needing to cast it to any, which is problematic.

An exported interface with an unexported method could only be implemented by types in the same package. This has both benefits: the type is still easy to pass around (it's exported) but the control over who gets to implement the type is maintained by the autogenerated code (the method required to implement the interface is not exported).

[puellanivis]

Again, the type does not actually exist. It is an artifact of implementing the protobuf semantics in Golang.

[jordan-bonecutter]

Unexported != nonexistent. I see no argument for not exporting the type.

[steeling]

Unexporting the field and then giving receiver methods to access it doesn’t make any sense, because 1) Golang eschews accessor methods,

But you do generate accessor methods on other exported types already...

and A) the proper way to switch/case on a set of different types is with a type-switch.

Sure, or you can use an interface to abstract that away and create a method that "accept interfaces [and return structs]" as the famous proverb says. There's even linters that warn against exported methods returning unexported types.

@puellanivis I disagree with the reasoning above. The evidence 1) and 2) do not logically conclude your point that it "doesn't make sense". Additionally, I fully understand your prior points about oneof not existing on the wire, but so what? An interface is more of an abstraction of an underlying collection of types than a type itself. IMO this fits near perfectly with the definition you're seeking.

Taking a step back, what is gained by enforcing this? Or on the other hand what is lost by unexporting the type? Some semantic relation that the generated go code doesn't 100% match what's on the wire? Isn't that the current state of affairs anyways? This decision is directly leading to more verbose code, and wasted developer time spent both reading/writing this boilerplate. Is that really a desirable goal to strive for as a maintainer? See the istio code linked above for a prime example.

[jroper]

As an aside, Java does now support pattern matching (ie, switch based on type).

I mostly work with protobufs in two languages, Scala and Go. ScalaPB synthesises a type for oneofs, and it's a dream to work with. It means when I'm constructing protobuf messages, I can very easily factor out reusable code snippets away from the base message construction and access. It enables me to produce more DRY code, easier to read code. In Go, the same messages are far more frustrating to work with. I either end up with massive switch statements, repeated in many places, because I can't factor out shared code because it can only work at the message access/construction site, or I have to pass interface {} around everywhere and then still have to do a switch when I want to set it on the message I'm constructing.

Yes, the type doesn't exist. But a use case for the type that facilitates productive development with protobufs in Go does exist.

[puellanivis]

I do really hate to throw a cold blanket on all of this, but we are just running in circles on a closed issue. Unfortunately, I have nothing new or different that I can explain here, as I am here chiefly to assist with triaging issues, and otherwise have no greater direction of the code or design than you. The only thing I can do here is relay the decisions and reasoning already made as best as I understand them. You can disagree with my reasoning all you want, but I don’t actually have any ability to change the position here.

As I’ve already said here—but which I think needs to be repeated most clearly: the only thing I can really do here is parrot the same arguments already made.

[jroper]

If you're not a maintainer of the project with decision making powers, how does the community actually interact with the maintainers?

[puellanivis]

The maintainers are here, and do look at this issue board. I’m just not one of them.

[lfolger]

I'm one of the maintainers and we will follow up on this discussion shortly.

[lfolger]

Thank you all for the discussion so far. Rest assured that the Go Protobuf maintainers are listening and taking your input seriously. Please keep in mind that any API change might seem small (as the pending Gerrit change might suggest), but has potentially large costs associated with it on the external and Google-internal Go ecosystem. Therefore, any change needs careful consideration.

---

I would like to understand the specific use case for this feature request better. Could you give one or more detailed examples to explain what you mean by:

[Exporting the oneof type] enables me to produce more DRY code, easier to read code.

What kind of function would you like to write that accepts or returns values of oneof field types?

Would it be possible for the functions to take the proto message as input and then use it as input or output parameter?

I think a oneof field does not make sense outside of the proto message that contains it and thus it should always be handled together with its associated message — or, put another way: any function that would take an exported oneof type can (today) just as well take the proto message type that contains the oneof.

[jordan-bonecutter]

Hi Ifolger, thanks for looking into this.

Here's the case I ran into:

message Telemetry {
  google.protobuf.Timestamp timestamp = 1;
  float voltage = 2;

  enum FooPowerSupplyAlert{
    AlarmComponentXFault1 = 1;
    AlarmComponentYFault = 2;
  }

  enum BarPowerSupplyAlert{
    AlarmTamperFault1 = 1;
    AlarmTamperFault2 = 2;
  }

  oneof powerSupplyAlert {
    google.protobuf.Empty ok = 3;
    FooPowerSupplyAlert = 4;
    BarPowerSupplyAlert = 5;
  }
}

Basically, I have a telemetry message from an electrical component which has a subfield for a power supply alert. This electrical component has a power supply from either manufacturer Foo or Bar, each of which have their own specific data format for representing device faults. I have a oneof message which could either be for a foo or a bar power supply.

Then, I have an alerting service which takes in protobuf messages and runs validation methods on the given fields. For instance, we may want to validate the voltage field, so we'd create an alert like follows:

template := &protos.Telemetry{}
alerts.New(
  "voltage",
  BuildMonitor().Validate(InRange(10, 12, "V")).Run(Extract(&template, &template.Voltage)),
  "output voltage",
)

I'll also want to create an alert for the alert oneof field on the message, so first I had to do the following:

alerts.New(
  "power-supply-fault",
  BuildMonitor().Validate(func(alert any) (err error) {
    // alert validation code...
  }).Run(Transform(func(message *protos.Telemetry) any {
    return message.PowerSupplyAlert
  })),
)

I could refactor the above code to take in an entire message, as you suggested:

alerts.New(
  "power-supply-fault",
  BuildMonitor().Validate(func(message *protos.Telemetry) {
    // alert validation code...
  }).Run(),
)

This code technically works, but I have a few problems with it:

1. When reading it's not clear why an entire message is being validated. I'd likely have to put a comment explaining the reasoning, which is never ideal.
2. This removes the separation of concerns the code should have, between getting the necessary data out of the protobuf message and validating its individual fields.

In terms of your argument

or, put another way: any function that would take an exported oneof type can (today) just as well take the proto message type that contains the oneof.

I suppose you are correct however you can make this point about every field of the message, so should we unexport all of them? Just because we technically can unexport the field I have yet to see a good reason why we should unexport it. I am happy to accept an unexported field if someone can explain why this is a better design choice, not something that is technically workable.

[neild]

For what it's worth, I'd write that as:

message Telemetry {
  google.protobuf.Timestamp timestamp = 1;
  float voltage = 2;
  PowerSupplyAlert alert = 3;
}

message PowerSupplyAlert {
  enum FooPowerSupplyAlert{
     AlarmComponentXFault1 = 1;
     AlarmComponentYFault = 2;
  }
  enum BarPowerSupplyAlert{
     AlarmTamperFault1 = 1;
     AlarmTamperFault2 = 2;
  }
  oneof alert {
    FooPowerSupplyAlert = 1;
    BarPowerSupplyAlert = 2;
  }
}

Or as:

message Telemetry {
  google.protobuf.Timestamp timestamp = 1;
  float voltage = 2;
  google.protobuf.Any alert = 3;
}

message FooPowerSupplyAlert {
  enum Fault {
     AlarmComponentXFault1 = 1;
     AlarmComponentYFault = 2;
  }
  Fault fault = 1;
}
// BarPowerSupplyAlert, BazPowerSupplyAlert, etc.

In either case, you have an object which you can pass around which contains only the fault.

This is what you'd need to do if you were using the C++ or Java protobuf APIs, which do not include any concept of a type representing a oneof. Go is, so far as I know, unique among first-party implementations in having a type that represents all the possible values of a oneof. And that type is sadly pretty unergonomic; the experience of working with oneofs is much better (IMO) in languages which use setters/getters for field access, where a field in a oneof behaves exactly like any other field.

[jordan-bonecutter]

Definitely, I can refactor the code to work around the oneof but I really want to question the decision of having an exported field with an unexported type. In all my time working in Go I have never seen this pattern and I think it would be great to make it more ergonomic.

[jroper]

Here's an example that I've have in my project. We're using the Google API guidelines for paging with a nextPageToken being used to fetch a particular page, we have a gRPC list call that allows disjoint criteria for filtering via a oneof, so we have:

message ListSomethingRequest {
  oneof criteria {
    string criteria_a = 1;
    string criteria_b = 2;
    string criteria_c = 3;
  }
  string next_page_token = 4;
}

I then have a CLI that fetches the pages, and renders them. I want to invoke this from many places in the code, and in each of the different places I'm going to be passing a different criteria. So, the code that fetches and renders them should be shared, but because I can't express the disjoint criteria as a type, I end up doing things like this:

func listSomething(string criteriaA, string criteriaB, string criteriaC) {
  done := false
  nextPageToken := ""
  for !done {
    request := &ListSomethingRequest {
      NextPageToken: ""
    }
    if criteriaA != "" {
      request.Criteria = &ListSomethingRequest_CriteriaA {CriteriaA: criteriaA}
    } else if criteriaB != "" {
      request.Criteria = &ListSomethingRequest_CriteriaB {CriteriaB: criteriaB}
    } else {
      request.Criteria = &ListSomethingRequest_CriteriaC {CriteriaC: criteriaC}
    }
    response := service.ListSometing(request)
    handleResponse(response)
    nextPageToken = response.NextPageToken
    done = nextPageToken == ""
  }
}

Now, I could accept a ListSomethingRequest to the method, but that doesn't make sense from a type perspective - the struct has only been partially constructed when it's passed, and this is just a trivial example, there could be a lot more information that needs to be set in the listSomething method. The result is a fragile method interface where the type system is unable to enforce what information should or shouldn't be provided to the method. The interface is made even more fragile by the fact that the method is going to be modifying the passed in message type, updating NextPageToken for each page fetched, which might be surprising to the caller from a method that appears to be doing nothing other than reading data.

Much better would be being able to express the criteria as a disjoint type that the method accepts, then I can write a method signature that expresses exactly what the method needs, no more, no less. But that requires exporting the disjoint type.

[neild]

For ListSomethingRequest, I'd write this as:

message ListSomethingRequest {
  ListSomethingCriteria criteria = 1;
  string next_page_token = 2;
}

message ListSomethingCriteria {
  oneof criteria {
    string criteria_a = 1;
    string criteria_b = 2;
    string criteria_c = 3;
  }
}

That is, if you want to pass around the criteria as an object, then define a message that contains the criteria. This is a very conventional approach from what I've seen of protobuf APIs that need to deal with more than just Go.

[jordan-bonecutter]

Having examples is well and good, but the core of the problem is having a struct field with an unexported type. The protobuf compiler should generate good code and IMO having an exported field with an unexported type is not a good programming practice. Is there not a better way to do this?

[neild]

The better way (IMO) is to have setters as well as getters for all fields, but that's a bit beyond the scope of discussion here.

req.SetCriteriaA("a")

is much nicer than (again IMO):

req.Criteria = &ListSomethingRequest_CriteriaA{CriteriaA: "a"}

[jordan-bonecutter]

Can you explain why unexporting this type leads to better code than exporting the type? I think that is the simple question we need to answer.

[lfolger]

Thanks for the helpful examples.

---

Generally I agree with neild@ that if you want to use a type, you should declare a message instead of using oneofs, which are constraints (as mentioned by puellanivis@).

---

When thinking about these kinds of features it is important to consider the broader protobuf ecosystem. For Go Protobuf, we aim to implement the protobuf spec — not less, and also not more. Whenever we add extra features that the spec doesn’t include, there is a risk of the spec later changing in ways that turn out to be incompatible with our implementation.

Exporting new types in particular is a change we can never take back! This means it deserves extra careful consideration.

---

In the discussion it was mentioned that fields are exported, so their types should be exported, too. But in fact, we think that exporting Protobuf message fields as Go struct fields as Go struct fields was not the best choice, as it makes a number of performance optimizations impossible to implement.

[jordan-bonecutter]

Hi Ifloger, thanks for the reply. I have a few thoughts:

Generally I agree with neild@ that if you want to use a type, you should declare a message instead of using oneofs, which are constraints (as mentioned by puellanivis@).

As I discussed in my Gerrit change request, this is a moot point. A type is a constraint. For instance, the type int32 constrains values to only those who may be represented as 32 bit integers. To say that this isn't a type, it's a constraint is contradictory because types are constraints.

Whenever we add extra features that the spec doesn’t include, there is a risk of the spec later changing in ways that turn out to be incompatible with our implementation.

Let's think about possible ways in which the implementation of oneof in Go could be changed.

1. Add new methods
2. Implement it as something other than an interface

These are the only possible changes one could make to the implementation (either changing the interface or changing the implementation entirely). If the implementation is changed via route 1 the methods can be added as unexported, irrespective of whether the interface itself is exported. If the implementation is changed via route 2 this will be a breaking change, whether we like it or not. Furthermore, saying that we shouldn't change feature X because the constraints on feature X may change down the line seems like an escape hatch to me. This thinking can be applied to any possible change. I think it's worth considering on codebases in their infancy or if there were some significant requests to change oneof in the pipeline, neither of which I see here. This thinking leads to a path of complacency and decision paralysis, neither of which will lead to the improvement of the project in the long term

As for the point that other languages do this differently (something I've seen pop up a few times in this thread), this is to be expected. From the protobuf website:

Protocol Buffers are language-neutral, platform-neutral extensible mechanisms for serializing structured data

We have language neutrality as a feature of protobuf because languages differ in how they represent types. So to say that Go shouldn't do X because C++ doesn't is a weird point to make. We should expect the generated code to look different in each language because, well, that's kind of the point. If all languages agreed on how to represent types protobuf would be much less useful (yes, there's still the issue of getting things on the wire in a way that is compatible with different platforms and evolving message formats). Language agnosticism is the first listed feature of protobuf, so let's view the Go code on its own, irrespective of how C++, Java, Scratch, ... do it.

---

If I can, let me make the best case as to why exporting the type actually adheres to the specification more closely. As previously established, types are constraints. Therefore, any constraint represented in protobuf land should be resolved as a type in Go land. There is no part of the protobuf spec which says that the fields of a oneof should be private to a protobuf message. The concept of privacy isn't really represented in protobuf, all properties of a message are public so to speak. By hiding the interface generated for a oneof, we are acting as if this constraint were hidden, which is not a feature in protobuf. As the protobuf constraint is not hidden, it should generate an exported Go type.

[puellanivis]

Furthermore, saying that we shouldn't change feature X because the constraints on feature X may change down the line seems like an escape hatch to me.

I want to point out that this project has absolutely been bitten by this precise issue before. We added the json struct field tag because it was so easy in Go, and then the official JSON mappings came out, and were entirely incompatible with not just the json struct field tags, but also the encoding/json library itself.

This is also an entirely normal part of building packages fitting widespread standards. We have to evaluate every change feature for how it will impact compatibility, because we are beholden first to standards compatibility itself. So, when you say:

This thinking can be applied to any possible change.

Yes, this can and must be applied to every possible change. This is just what the world of programming to standards is.

[jordan-bonecutter]

Yes, you are totally correct. Let me clarify my point.

We shouldn't use the reasoning of change X may break things because of some possible change down the line without consideration of what could break. In my comment I tried to justify why this change is unlikely to break anything. Basically, changing the interface can still easily be done even if we export it. We can add as many unexported methods as we want without breaking anything for the end user. Now, this will break if the implementation of oneof changes from an interface to something else but this will break downstream code, type switches won't work. If we can come up with a reason why this is likely to break in a way that an unexported interface wouldn't, I'm all ears. But we shouldn't use this as an excuse not to improve the codebase.

[lfolger]

[…] or if there were some significant requests to change oneof in the pipeline, neither of which I see here.

We are in fact busy with significant changes and will share more when the time is ready.

We have heard your arguments on this issue and will take it into consideration. Thanks.

[jordan-bonecutter]

Can we leave this open until a discussion is had on the merits of leaving this unexported? If the community can't understand why certain decisions are made, why open source this?

[EricRabil]

Can the issue please be reopened until this can be properly addressed?

[texuf]

First of all, thank you for building and maintaining this library. My project depends on it and we’re not paying a dime. Your time is valuable and I respect that.

That said, I would also appreciate if this stayed open until it was properly resolved. I’ve been very impressed with the civility of this conversation. I would like to applaud bonecutter for his professional tone, I would not have been able to keep it together for this long.

My recap of the conversation so far…

Devs: There’s a type that isn’t exported, it seems like an over-site. Can we please make it public? Our code will be simpler and easier to maintain.

Maintainers: No.

Devs: Why not?

Maintainers: I don’t understand why it would be useful.

Devs: here are some examples from our code

Maintainers: that’s dumb, you should re-write the protobufs.

Devs: we can’t because we don’t have control of the protobufs, or they are already in production, or our team would prefer to do it this way.

Maintainers: we can’t change this because it hurts forward comparability

Devs: In what way? If you refactor this type away we’re going to have to change all our code anyway. Our code already depends on this type, we just can’t use it directly.

Maintainers: it’s a secret. Closed.

Did I miss anything? I realize this exposition probably isn’t going to help the situation, but I feel like we’re being trolled. The type is obviously useful, because it’s in use already. All I want to do is write a function that accepts the oneof type as a parameter, so that I can construct my protobuf in one place. I solved my problem with custom tooling, but I would love to delete that hack out of our pipeline.

This will be my last message in the thread, I hope this message is received in good faith. Again, I respect your time and genuinely appreciate that you provide this library for free. Thank you.

[jordan-bonecutter]

It's possible that they're making some large changes to the repo in general (I think ifolger's comment which was cut off was about to say they will only use setters and getters.) Maybe they will incorporate this change then? At least I hope so.

In general, it would be great if we could be in on the changes they're making as that's half the fun of open source.

[lfolger]

The reason I closed this issue is that I don't think we will reach an agreement on this.

From our point of view exposing this type does not only increase the maintenance cost for this repository but also leads to worse. Not necessarily worse Go code but worse proto code. By using a oneof as a type, you are using protos in a way that is only compatible with Go. As mentioned before protos are primarily a language interoperability tool. If something is important enough that you need define APIs on it, you should define a type for it, that is a message.

A oneof field is not a type it is a constraint on the message. It defines multiple fields in the message and constraints the message to only set one of these fields at a time. Treating oneof fields as types has another disadvantage and that is users need to decide which mechanism to use to define a type: a oneof field of a message? By not treating oneof fields as types, users will intuitively use the correct mechanism to define types.

As a matter of fact, in Google the type was exported in the past and it let to issues (language interop problems and misuse of the oneof API in Go) which is why we didn't export it in the open source implementation.

We will not engage further in this discussion for now. Thank you for your understanding.

[jordan-bonecutter]

From our point of view exposing this type does not only increase the maintenance cost for this repository but also leads to worse.

Can you explain how this increases maintenance costs/leads to worse proto code?

A oneof field is not a type it is a constraint on the message.

What is the difference between types and constraints?

As a matter of fact, in Google the type was exported in the past and it let to issues

Can you provide an example of an issue which arose from exporting this type?