Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv2: proto: Equal behavior on nil and empty messages #965

Open
dsnet opened this issue Oct 8, 2019 · 5 comments

Comments

@dsnet
Copy link
Member

commented Oct 8, 2019

What should the behavior of the following be?

nilMessage = (*MyMessage)(nil)
emptyMessage = new(MyMessage)

Equal(nil, nil)                   // true
Equal(nil, nilMessage)            // false
Equal(nil, emptyMessage)          // false
Equal(nilMessage, nilMessage)     // true
Equal(nilMessage, emptyMessage)   // ???
Equal(emptyMessage, emptyMessage) // true

In v1, the answer is false, but in v2 it is true since a typed nil pointer is functionally equivalent to a read-only empty message.

The challenge right now is that the generated API gives illusions of both:

  • The generated API represents presence of messages with a typed nil pointer.
  • However, calling the Get methods on a typed nil pointer presents the illusion that the message is empty.
  • The protobuf language has no concept of message pointers. Presence is a property of the message field, and not a first-class property of the message type.
@neild

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

I think the v2 answer is the correct one: A nil message pointer is equal to an empty message.

A nil pointer is used to represent an unset message field. A nil pointer is also used to represent the default value of a message field, which is an empty message. If proto.Equal compares a nil message as unequal to an empty one, then the default value of a message field (a nil pointer) isn't equal to the actual default value (a message), which is weird.

I'm increasingly convinced, however, that we should surface nil-ness in the protoreflect.Message interface. It's not part of the proto language, but it's a sufficiently pervasive component of the existing Go proto API that we may as well ensure that we have the tools to work with it without dropping into Go reflection.

@puellanivis

This comment has been minimized.

Copy link

commented Oct 9, 2019

Solid arguments.

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

I'm looking at a number of proto.Equal use-cases and the v2 definition would break them.

A number of both tests and production code do something like the following:

if proto.Equal(m1.GetFoo(), m2.GetFoo()) {
    ...
}

In such a use-case, they are depending on presence of the top-level message. That is, this type of code heavily depends on the fact that (*MyMessage)(nil) != new(MyMessage) according to v1's proto.Equal.

@neild

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Unfortunate.

Perhaps we need to retain the v1 definition of equality as a matter of practicality. A small inconsistency that eases the transition to the v2 API is preferable to a theoretically perfect API that nobody uses.

The v1 API mostly treats a nil message as invalid: It refuses to marshal a nil message and it disallows nils in repeated fields. Maintaining those limitations is probably not important, however.

@puellanivis

This comment has been minimized.

Copy link

commented Oct 10, 2019

Ouch… well, the only thing worse than breaking other people’s code is doing it silently. :(

Changing semantics is always iffy, no matter how simple it appears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.