Support setting and iterating over map keys #511

negz · 2023-10-02T00:54:14Z

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]>

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]>

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]>

Signed-off-by: Nic Cope <[email protected]>

negz · 2024-09-12T18:49:25Z

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

adonovan · 2024-09-12T20:33:57Z

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.

adonovan

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.

adonovan · 2024-09-17T19:30:14Z

lib/proto/proto.go

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

+var _ starlark.Iterable = (*RepeatedField)(nil)


Use var (...) for neat alignment:

var ( _ ... ... )

adonovan · 2024-09-17T19:31:24Z

lib/proto/proto.go

+	rf := b.Receiver().(*RepeatedField)
+
+	if err := rf.checkMutable("append to"); err != nil {
+		return nil, fmt.Errorf("%s: %v", b.Name(), err)


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

adonovan · 2024-09-17T19:33:14Z

lib/proto/proto.go

-	}
-	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 {


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

adonovan · 2024-09-17T19:34:35Z

lib/proto/proto.go

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

+type MapField struct {


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

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

adonovan · 2024-09-17T19:34:47Z

lib/proto/proto.go

+	itercount int
+}
+
+var _ starlark.HasSetKey = (*MapField)(nil)


var ( ... )

adonovan · 2024-09-17T19:57:15Z

lib/proto/proto.go

+	})
+
+	// 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


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.

adonovan · 2024-09-17T20:00:43Z

lib/proto/proto.go

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


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

adonovan · 2024-09-17T20:30:46Z

starlark/eval_test.go

@@ -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:


adonovan · 2024-09-17T20:31:33Z

starlark/testdata/proto.star

+
+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"})


Could we have some tests of the various failure modes?

adonovan · 2024-09-17T20:33:45Z

starlark/testdata/proto/test.proto

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


// 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.)

negz added 7 commits September 30, 2023 22:24

gofmt proto.go

ca7b665

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]>

Support appending to protobuf repeated fields

95e925a

Signed-off-by: Nic Cope <[email protected]>

Support protobuf maps

82ea9f9

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

Sort protobuf maps by key

d1f4465

Signed-off-by: Nic Cope <[email protected]>

Add tests for protobuf map fields

171160d

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]>

Remove mention of proto_test.go

2531872

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]>

Add brief documentation for the MapField type

eb7d2e4

Signed-off-by: Nic Cope <[email protected]>

negz force-pushed the maps-and-friends branch from 1aae515 to eb7d2e4 Compare October 2, 2023 01:00

negz mentioned this pull request Oct 2, 2023

Should repeated and map proto fields by writable by default? #512

Open

adonovan reviewed Sep 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting and iterating over map keys #511

Support setting and iterating over map keys #511

negz commented Oct 2, 2023 •

edited

Loading

negz commented Sep 12, 2024

adonovan commented Sep 12, 2024

adonovan left a comment

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

adonovan Sep 17, 2024

Support setting and iterating over map keys #511

Are you sure you want to change the base?

Support setting and iterating over map keys #511

Conversation

negz commented Oct 2, 2023 • edited Loading

negz commented Sep 12, 2024

adonovan commented Sep 12, 2024

adonovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz commented Oct 2, 2023 •

edited

Loading