When using cmp.Diff with a struct that embeds a proto message, protocmp.Transform ignores all other fields

[nathanperkins]

What version of protobuf and what language are you using? Version: (e.g., v1.1.0, 89a0c16f, etc)

    google.golang.org/grpc v1.64.0
    google.golang.org/protobuf v1.34.2

What did you do?

I used cmp.Diff and protocmp.Transform to compare a struct that has an embedded proto message.

package main

import (
    "fmt"

    "github.com/google/go-cmp/cmp"
    pb "github.com/moby/buildkit/solver/pb"
    "google.golang.org/protobuf/testing/protocmp"
)

type T struct {
    *pb.BuildInput
    X int
}

func main() {
    want := &T{X: 10}
    got := &T{}

    if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
        fmt.Printf("diff:\n%q", diff)
    }
}

What did you expect to see?

I expected to see a diff message showing that the X field is set to 10 in want but missing in got

What did you see instead?

I saw no output, indicating that protocmp.Transform ignored the X field and considered these the two structs to be equivalent.

[dsnet]

This is an unfortunate as T quite literally implements proto.Message that protocmp.Transform is looking for.

Here are some options.

1. Avoid embedding:

type T struct {
    BuildInput *pb.BuildInput
    X int
}

2. Embed but mask out T from implementing proto.Message

type T struct { ... }

// Use deliberate conflicts on method names with wrong signature
func (T) ProtoMessage(bool) {} // any signature that doesn't match protoadapt.MessageV1
func (T) ProtoReflect(bool) {} // any signature that doesn't match protoadapt.MessageV2

3. Add a filter around protocmp.Transform that filters out T

if diff := cmp.Diff(want, got, cmp.FilterPath(func(p cmp.Path) bool {
    t := p.Last().Type()
    return t != reflect.TypeFor[T]() && t != reflect.TypeFor[*T]()
}, protocmp.Transform())); diff != "" {
    fmt.Printf("diff:\n%s", diff)
}

[nathanperkins]

Thanks, we decided to avoid embedding moving forward for structs involving protos. We wanted to report it as a potential bug in protocmp because:

1. This unexpected behavior results in false negatives on tests. We had broken test cases which were improperly passing due to the diff ignoring our fields. We didn't discover it until one of the team members decided they would prefer not to embed this field, then they were confused why the tests broke. Thankfully, the breakage was improper test expectations rather than the code actually being broken.
2. It's unfortunate to have to design our structs around a limitation in protocmp.

Is it possible that protocmp.Transform could be fixed so that it works for embedded fields? AFAIK, cmp.Diff works fine on structs embedding non-proto fields.

If there is a fundamental limitation, maybe its at least worth documenting.

[dsnet]

I don't think this is a limitation in protocmp per-say, but rather a fundamental limitation in the Go language itself.

Embedding is powerful hammer. Most often someone just wants to promote the fields, or a subset of the methods, but usually not all the fields and methods. By embedding, T now implements proto.Message and is a protobuf message for all intents and purposes. Thus, protocmp.Transform is going to treat T as a protobuf message because it claims to be one. In particular, as a protobuf message, it does not have an X field.

This effect isn't that different than doing something like:

type T struct {
    time.Time
    X int
}

json.Marshal(T{X: 10}) // prints "0001-01-01T00:00:00Z" because T implements json.Marshaler

where information about field X was lost during marshaling.

we decided to avoid embedding moving forward for structs involving protos

In general, I recommend against embedding anything with methods that satisfy common interfaces.

[dsnet]

Is it possible that protocmp.Transform could be fixed so that it works for embedded fields?

With Go reflection, we might be able to detect some cases of embedding, but there will be false positives with the detection. We're fundamentally running into a Go language issue here.

[puellanivis]

🤔 Yeah, we would have to check if it implements neither proto message type, then early return false, otherwise iterate through each struct field checking to see if the fields is anonymous and implements either proto message type. 🙈 The number of corner cases that would need to be considered… like we could never tell the difference between “implements a proto message itself but also has an embedded proto message,” which TBF seems like a pretty narrow “you’re playing with fire” corner case…

As well, best practice as noted is to avoid ever embedding a type that you yourself do not control. There’s just too many situations that you can run into just like this, or with a new method name conflicting with a field name, etc. I mean, sure we could conceivably fix protocmp.Transform to avoid conflating a proto message with a struct embedding a proto message, but that doesn’t stop any of the potential bugs from any other code that makes the type assertion themselves. We’d have to then recommend a sort of proto.IsMessage function to do the implementation test for everyone, but it wouldn’t at all stop people from doing the type assertion anyways, and would be completely useless in a type switch anyways.

I have to agree with dsnet… there’s just no practical way to prevent this from happening. It’s intrinsically built into the language, which is why best practice has been to avoid it whenever possible.

[nathanperkins]

Thanks to both of you for the incredible insights and expertise! It's disappointing that this is a limitation on the language but totally understandable.

You can consider this closed :)