protoc-gen-go: unexpected trailing underscore in oneof field name

[zellyn]

What version of protobuf and what language are you using?
Version: go1.11.4 darwin/amd64

What did you do?

syntax = "proto2";

package oneoftest;

// If you rename this to `PutBar`, the problem goes away.
message GetBar {
    optional string a_string = 1;
}

message Foo {
  oneof bar {
    GetBar get_bar = 1;
  }
}

What did you expect to see?
The oneof field on the struct should be named Bar, since there's nothing else for it to conflict with.

What did you see instead?
The oneof field is named Bar_.

type Foo struct {
	// Types that are valid to be assigned to Bar_:
	//	*Foo_GetBar
	Bar_                 isFoo_Bar_ `protobuf_oneof:"bar"`
	XXX_NoUnkeyedLiteral struct{}   `json:"-"`
	XXX_unrecognized     []byte     `json:"-"`
	XXX_sizecache        int32      `json:"-"`
}

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment?

IRL, we have things named GetRequest and PutRequest, and oneof fields named Request. I'm trying to upgrade our version of golang/protobuf, and this is causing widespread breakage.

I think it's happening because of the conflict with the GetBar() method on the struct, but the renaming of the field itself was unexpected, and caused breakage.

[dsnet]

Bisect blames 93b26e6.

\cc @bufflig @neild

[dsnet]

I suspect this is an issue with the name mangling algorithm that protoc-gen-go does where the refactoring subtly changed the order in how fields were processed. The algorithm is unfortunate :/. However, this is still a bug, and we should preserve the output of the previous mangling.

[dsnet]

The issue is because...

Prior to 93b26e6, the algorithm for naming conflict resolution was roughly equivalent to (in Python psuedo-code):

defer DeriveFieldNames(message):
    used_names = set()
    seen_oneofs = set()
    for field in message:
        # Handle fields.
        field.go_name = CamelCase(field.name)
        while used_names & set([field.go_name, "Get"+field.go_name]):
            field.go_name += "_"
        used_names.update(set([field.go_name, "Get"+field.go_name]))

        # Handle oneofs.
        if field.oneof is None or field.oneof in seen_oneof:
            continue
        field.oneof.go_name = CamelCase(field.oneof.name)
        while used_names & set([field.oneof.go_name]):
            field.oneof.go_name += "_"
        used_names.update(set([field.oneof.go_name]))
        seen_oneof.add(field.oneof)

93b26e6, which was a "refactoring" change, also slightly changed the algorithm to:

defer DeriveFieldNames(message):
    used_names = set()
    seen_oneofs = set()
    for field in message:
        # Handle fields.
        field.go_name = CamelCase(field.name)
        while used_names & set([field.go_name, "Get"+field.go_name]):
            field.go_name += "_"
        used_names.update(set([field.go_name, "Get"+field.go_name]))

        # Handle oneofs.
        if field.oneof is None or field.oneof in seen_oneofs:
            continue
        oneof = field.oneof
        oneof.go_name = CamelCase(oneof.name)
        while used_names & set([oneof.go_name, "Get"+oneof.go_name]): # difference here
            oneof.go_name += "_"
        used_names.update(set([oneof.go_name, "Get"+oneof.go_name]))  # difference here
        seen_oneofs.add(oneof)

That subtle change is actually a step in the right direction, since it correctly records the fact that a Get method is actually generated for the parent oneof. However, a problem is that it did not address the fact that the conflict resolution algorithm is still checking for field.go_name when the field belongs in a oneof. For that case, a struct field of that name is not generated in the Go struct, so no conflict exists.

The stateful name mangling logic is unfortunate and broken and I wish we could strip it all out. If we revert to pre- 93b26e6 behavior, then we could still generate broken code since the name mangling logic did not account for the Get method on the parent oneof. If we stay on 93b26e6 behavior, then that breaks @zellyn's use case. If we "roll forward" and fix the algorithm to avoid checking field.go_name for fields that are members of a oneof, then that would fix @zellyn's use case, but potentially break other users:

defer DeriveFieldNames(message):
    used_names = set()
    seen_oneofs = set()
    for field in message:
        # Handle fields not part of a oneof.
        field.go_name = CamelCase(field.name)
        if field.oneof is None:                                       # difference here
            while used_names & set([field.go_name, "Get"+field.go_name]):
                field.go_name += "_"
            used_names.update(set([field.go_name, "Get"+field.go_name]))
            continue                                                  # difference here

        # Handle fields that are part of a oneof.
        while used_names & set(["Get"+field.go_name]):                # difference here
            field.go_name += "_"
        used_names.update(set(["Get"+field.go_name]))                 # difference here

        # Handle oneofs.
        if field.oneof in seen_oneofs:
            continue
        oneof = field.oneof
        oneof.go_name = CamelCase(oneof.name)
        while used_names & set([oneof.go_name, "Get"+oneof.go_name]):
            oneof.go_name += "_"
        used_names.update(set([oneof.go_name, "Get"+oneof.go_name]))
        seen_oneofs.add(oneof)

[zellyn]

Hey @dsnet - thanks for the speedy analysis. What would you recommend I do? I'm luckily in a monorepo, so I could just make changes everywhere to add underscores, but I'd like to match the intended future behavior.

My 2 cents is that adding a new alternative to a oneof field — which syntactically nests inside the oneof declaration — should not suddenly change the name of the “parent” oneof.

[neild]

How hard is it to restore the exact previous behavior?

As @dsnet says, we really shouldn't be doing any of this conflict resolution; it's inherently confusing and makes it incredibly difficult to avoid subtle changes in behavior like this. As long as we're stuck with it, however, I think we need to prioritize stability (i.e., don't change existing behavior).

[dsnet]

@zellyn, can you verify that #782 fixes the problem for you?

[zellyn]

Yep, that fixes it. Thanks!