fix golang oneof with a typed-nil value causes panic.

[anzboi]

The existing generated code for a oneof field causes a nil-panic if the field contains a typed-nil value. Go correctly infers the value is one of the switch cases but the validation code assumes the value is not a typed-nil.

NOTE: THERE IS AN ARGUMENT THAT THIS BEHAVIOUR IS CORRECT. The authors of go protobuf have stated that any typed-nil in a oneof field is invalid, and so this maybe should cause a panic. Nil panic however is not a helpful error message. I see a few ways forward.

1. We leave the generated code as is
2. We adopt this change, since validation error is not as harsh as nil panic
3. We update this PR, instead of adding to the validation errors, we trigger a panic with a more helpful error message
4. We add an option to the validator plugin that allows the invoker to select between options 2 and 3, with default 3.

[elliotmjackson]

Some points from the wonderful @jhump

Bullet two (the change in the PR) seems like the most appropriate course of action. This should be a validation error, not a panic.

Bullets 1 and 3 just seem overly harsh -- despite the assertion that a typed nil is not valid (similarly, a nil message inside a repeated field and a nil value in a map with message values are also invalid), these cases are still handled by the core runtime library without panic*.

Bullet 4 is overly complicated: I don't think the choice to not panic would be controversial, so a new knob to control the behavior is overkill.

An example of all three:

syntax = "proto3";

package foo.bar;

message Foo {
  map<string, Foo> bar = 1;
  repeated Foo baz = 2;
  oneof val {
    string s = 3;
    uint64 i = 4;
  }
}

Generate Go code and then use the following, which tries to marshal to JSON a message with all three problems: a nil element in a repeated field, a nil value in a map, and a typed-nil oneof.

f := Foo{Bar: map[string]*Foo{"foo": &Foo{}, "bar": nil}, Baz: []*Foo{nil}, Val: (*Foo_S)(nil)}
d, err := protojson.Marshal(&f)
if err != nil {
	fmt.Print(err)
} else {
	fmt.Print(string(d))
}

It doesn't even return an error (much less panic)! I just succeeds as if the nil messages were empty and as if the typed-nil were a nil interface (e.g. no oneof set):

{"bar":{"bar":{}, "foo":{}}, "baz":[{}]}

[anzboi]

FYI, there is relevant discussion on typed nils in https://github.com/golang/protobuf/issues/1415. From this I gather that treatment of typed-nils is at best inconsistent.

For example, from your example proto spec, the generated getter panics (thats actually why I raised that issue in the first place).

f := Foo{Val: (*Foo_S)(nil)}
f.GetS() // panics
f.GetI() // correctly returns zero

In any case, from your response I can see a couple more options... 5. Treat typed-nil as valid, oneof is unset (this matches protojson) 6. Treat typed-nil as valid, value is the zero value of the represented field.

I don't see the value in 6 as the linked discussion has justification why protojson chose one way over the other, so I'm inclined to go with option 5 (or 2 if we still want to treat oneof as invalid).

[jhump]

My 2 cents: option 2 still makes the most sense. While some runtime functions may accept questionable input without complaining, this is a validation framework after all, so it seems totally fair to report ostensibly incorrect messages as errors. To that end, it may be worth opening a separate issue to track similar treatment for repeated fields/slices that contain nil messages and map fields with values that are nil messages.

[CLAassistant]

All committers have signed the CLA.