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: protoreflect: consider Message nilness test #966

Closed
neild opened this issue Oct 9, 2019 · 2 comments
Closed

APIv2: protoreflect: consider Message nilness test #966

neild opened this issue Oct 9, 2019 · 2 comments

Comments

@neild
Copy link
Contributor

neild commented Oct 9, 2019

As mentioned in #965 and #964, while the protobuf data model does not include a distinction between nil and empty messages, the v1 API does treat the two differently in various places. For example, the v1 proto.Equal treats a nil message as not equal to an empty one.

Should we consider adding a method to the protoreflect.Message interface to test for nil messages, to let us handle these cases without dropping to Go reflection?

If we do add this, what should the method be called? IsNil? IsZero?

Should we add equivalents for Map and List? If so, does List.IsNil test whether the slice in a generated repeated field is nil, or that the List is a read-only, immutable one as returned by some protoreflect methods?

@neild neild added the blocks-v2 label Oct 9, 2019
@neild
Copy link
Contributor Author

neild commented Nov 26, 2019

Thinking about this some more: The protobuf data model doesn't have a concept of nil-ness, but the reflection API does, in a sense. A Message or Map with an underlying nil is read-only, and will panic on writes. List has a similar concept, when providing a view of a repeated field in a nil message. In all these cases, the read-only state of the value is significant to the user, but can only be detected by trying to mutate the object and encountering a panic.

I propose adding an IsValid method to Message, Map, and List to test for this condition. I'm open to a better name for this method. (IsReadOnly isn't quite right, since one could imagine a read-only, *non-*empty object to which this should apply.)

// IsValid reports whether the message is valid.
//
// An invalid message is an empty, read-only value.
// An invalid message often corresponds to a nil pointer of the concrete
// message type.
IsValid() bool

@dsnet
Copy link
Member

dsnet commented Nov 26, 2019

IsValid SGTM. It parallels the Go reflect.Value.IsValid API. The comment should explicitly mention that it is beyond the protobuf data model.

@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants