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

docs: mutability of List/Map/Message values in Get and Set #1346

Closed
fdymylja opened this issue Aug 2, 2021 · 2 comments
Closed

docs: mutability of List/Map/Message values in Get and Set #1346

fdymylja opened this issue Aug 2, 2021 · 2 comments

Comments

@fdymylja
Copy link

fdymylja commented Aug 2, 2021

The protoreflect.Message interface documentation does not specify how mutabilty of protoreflect.Value for Message, List and Map types works when Getting them or Setting them.

For example, something that I found inconsistent was the fact that if you Set a MapValue or MessageValue and you modify it after, the changes are reflected in the message, this does not appear to be the case for ListValue.

@neild
Copy link
Contributor

neild commented Aug 2, 2021

We're missing a line of documentation on Message.Set. List.Set, List.Append, and Map.Set all state:

// When setting a composite type, it is unspecified whether the set
// value aliases the source's memory in any way.

Message.Set should have the same text.

In addition, it would be good to have a clear description of aliasing behavior somewhere. The summary is that a protoreflect.Value acquired from a composite value's Get or Mutable method is guaranteed to alias the original value, and that it is otherwise unspecified when aliasing occurs.

So this is a valid way to append to a repeated field:

list := m.Mutable(someRepeatedFieldDesc).List()
list.Append(v) // appends v to the field in m

But this is not:

list := m.NewField(someRepeatedFieldDesc).List()
m.Set(someRepeatedFieldDesc, list)

// It is unspecified whether `list` aliases the field in `m`,
// so the next line may or may not modify m.
list.Append(v)

@neild
Copy link
Contributor

neild commented Aug 3, 2021

We're missing a line of documentation on Message.Set. List.Set, List.Append, and Map.Set all state:

Actually, this line is there. No idea how I missed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants