google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

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

Open negz opened 9 months ago

negz commented 9 months ago

I'm new to Starlark[^1]. I'm attempting to write a little Go function that essentially defers answering a gRPC request to a Starlark script. Like this:

v1beta1 = proto.file("v1beta1/run_function.proto)

def main(req):
  # Do some stuff with req, which is a v1beta1.RunFunctionRequest message...

  return v1beta1.RunFunctionResponse(
    results=[v1beta1.Result(
      severity=v1beta1.Severity.SEVERITY_NORMAL,
      message="Hi from Starlark!"
    )]
  )

My first intuition was to try doing something like this[^2]:

  rsp = v1beta1.RunFunctionResponse()
  rsp.results.append(v1beta1.Result(message="Hi!"))

However, I was surprised to be told that rsp.results was a frozen repeated field. Looking at proto.go I realized this was the default value, so I tried this:

  rsp = v1beta1.RunFunctionResponse()
  rsp.results = []
  rsp.results.append(v1beta1.Result(message="Hi!"))

Unfortunately the field is still frozen. I'm guessing because the Has() method of protoreflect.Message only considers a message to have a repeated field (and thus I guess by extension map field) if it's non-empty.

With all that said, I'm curious:

I'm not sure it's possible to make them writable/unfrozen by default but I wanted to ask before I dug in too deeply.

[^1]: I guess technically I used it in a bunch of BUILD files a decade ago, but I've forgotten all about that. 🙂 [^2]: I added the append method in https://github.com/google/starlark-go/pull/511.

negz commented 9 months ago

I think part of what I'm looking for, especially for maps (in the context of https://github.com/google/starlark-go/pull/511), is an API like this:

if msg.map_field == None:
  msg.map_field = {}
msg.map_field["key"] = "value"

This is similar to what was mentioned in https://github.com/stripe/skycfg/issues/23#issuecomment-444221347. I can use not proto.has instead of == None easily enough, but I'll still end up with a frozen field because the msg.map_field = {} doesn't effectively replace the default value with a usable dict/MapField.