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

Support setting and iterating over map keys #511

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

negz
Copy link

@negz negz commented Oct 2, 2023

Fixes #381

This PR introduces a new MapField type, which can be used to set and access map field values by key.

Without this PR I get a panic in toStarlark1 where x.Message() is called on a map field value. (Perhaps because my proto has some map<string, Message> fields):

panic: type mismatch: cannot convert map to message

I've also updated the nascent proto.star unit tests to use their own proto file, to test repeated and map fields.

Separately from the main change, I've included a commit that adds a list-like append method to RepeatedField. I can break that out into a separate PR if you'd prefer.

I'm running go1.20.8, and my gofmt insists on this formatting. Not sure
why it's different from what's in master.

Signed-off-by: Nic Cope <[email protected]>
For now with random iteration. I'll work on stable iteration in a future
commit.

Signed-off-by: Nic Cope <[email protected]>
This switches to using a dedicated test.proto file, since the
descriptor.proto file previously being used doesn't have any map fields.

Signed-off-by: Nic Cope <[email protected]>
It doesn't seem to exist - I don't think it was ever committed to this
public repo.

Signed-off-by: Nic Cope <[email protected]>
@negz
Copy link
Author

negz commented Sep 12, 2024

@adonovan Gentle ping. 🙂 Is this a contribution you'd be interested in accepting?

@adonovan
Copy link
Collaborator

Sorry for the very long delay. I just spent 20m playing with this and it looks like the right approach. It'll take me a little longer to review the code change properly but I'll try to get to it this week.

Thanks for your patience.

Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution, and sorry it took so long to review. Overall it looks good. I've left a lot of comments but don't be discouraged--they're mostly pretty superficial.

@@ -898,18 +927,48 @@ type RepeatedField struct {
itercount int
}

var _ starlark.Iterable = (*RepeatedField)(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use var (...) for neat alignment:

var (
    _ ...
    ...
)

rf := b.Receiver().(*RepeatedField)

if err := rf.checkMutable("append to"); err != nil {
return nil, fmt.Errorf("%s: %v", b.Name(), err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return err should suffice here: the error includes the string "append" already.

}
if rf.itercount > 0 {
return fmt.Errorf("cannot insert value in repeated field with active iterators")
if err := rf.checkMutable("insert value in"); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"insert into" is the string used by list.append; let's use it here too.

@@ -969,6 +1040,145 @@ func (it *repeatedFieldIterator) Done() {
}
}

type MapField struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration warrants a comment; use RepeatedField as a guide.

// A MapField represents a protocol message field of type 'map'. etc

itercount int
}

var _ starlark.HasSetKey = (*MapField)(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var ( ... )

})

// A map key can be any scalar protobuf type except floats and bytes.
// In practice in starlark they should be Int, String, or Bool and thus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to enforce that the keys are all int, string, or bool, and return an error if not.

Otherwise, the Compare operation may fail, the error will be ignored, and the sort order will be undefined, or we'll expose the underlying randomness of the protoreflect.Map, which is the kind of nondeterminism Starlark was designed to avoid. If we insist on these three key types, we can guarantee that the sorted key sequence is fully determined.


func (mf *MapField) String() string {
// We want to use {k1: v1, k2: v2} notation, like a dict.
buf := new(bytes.Buffer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strings.Builder to save an allocation, since the result is desired as a string.

@@ -115,7 +117,23 @@ func TestExecFile(t *testing.T) {
testdata := starlarktest.DataFile("starlark", ".")
thread := &starlark.Thread{Load: load}
starlarktest.SetReporter(thread, t)
proto.SetPool(thread, protoregistry.GlobalFiles)

// This proto is used for the proto.star tests. It's generated by running:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.


assert.eq(dict(m.map_field), {"a": "A", "b": "B"})
m.map_field["c"] = "C"
assert.eq(dict(m.map_field), {"a": "A", "b": "B", "c": "C"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have some tests of the various failure modes?

@@ -0,0 +1,11 @@
syntax = "proto3";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// This proto schema is used for tests in proto.star.
// Re-generate the binary FileDescriptorSet file by
// running: protoc ...(etc).
// (Requires the "protobuf" brew/apt package.)

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

Successfully merging this pull request may close these issues.

proto: Support map fields
2 participants