tiltfile: make probe types play nicer with None

[milas]

There's often a lot of boilerplate involved for optional object arguments
that can be None or an actual value; it's necessary to use a
starlark.Value, check to see if it's nil (kwarg not specified)
or starlark.None (kwarg specified) or present and then make sure
the expected type constraint is met.

This is exacerbated by starlarkstruct.Struct, which does not play
nice with Go nil by design, and when you've got a struct with embedded
structs that can be None, this results in a bunch of extra nil/None/type
checking on top of the above.

Adding an Optional type helps abstract some of this and provides
type-checking and a couple convenience methods depending on whether
you're interoping with Go code (ValueOrNil()) or Starlark types
(ValueOrNone()). There's still some type-casting, unfortunately,
but it makes things safer and cuts down on boilerplate code
substantially.

This adds Unpack() implementations that check for starlark.None first
and then attempt to cast to the correct type.

Closes #4160.

[nicks]

This seems very complex. And I don't want to have to write custom logic for every type for handling None

Have you considered having Probe implement the Unpacker interface and handling None there?
https://pkg.go.dev/go.starlark.net/starlark#Unpacker

Alternatively, have you considered filing a bug upstream to have starlark's UnpackArgs treat "None" the same as unspecified for optional args? Alan might be open to it.

[milas]

I agree it's still a lot of boilerplate -- I'd actually stashed this approach, but the simplified version missed out on explicit handling of None.

In terms of starlark.Unpacker - it's a bit weird, as from a Starlark/language perspective, you shouldn't be able to unpack None into a concrete type, i.e. you're trying to assign null to a value type. (I think this would also necessitate a bunch of weird, mostly redundant code to continue using starlarkstruct under the hood, at which point it'd probably be better to just do everything manually.)

In theory, Starlark should probably behave closer to json.Unmarshal and perform allocations for you, so that you could have var myObj *myType that Starlark would leave as nil if unpacking None (or unspecified) and allocate a myType instance otherwise to unpack into (right now it'll panic with a nil pointer dereference). That's likely a pretty big overhaul within the unpacking code though. I can definitely file an issue and start that conversation if you think it makes sense.

In the interim, I can eliminate the Optional wrapper and just use starlark.Value in the unpack and do extra nil/starlark.None checks + coercion here similar to what it was already doing if you think that's better?

[nicks]

not totally sure i understand what you're saying. you think it would be harder to do it with Unpacker? eh, i've worked with the starlark folks a bunch, i can send them a Q about it

[nicks]

filed as google/starlark-go#347

[milas]

Let me play around with Unpacker. It's definitely not impossible, it just unfortunately won't solve things in a more general case since each type will have some fairly redundant logic.

[milas]

FWIW Here's what it looks like with a per-type Unpacker implementation (I was overthinking it originally, no weirdness with starlarkstruct.Struct): a8aa829.

That could be a reasonable interim until changes are made in Starlark to address the immediate ergonomics issues...let me know what you think and I can rebase this PR or just close it out.

[nicks]

ya that seems reasonable. The starlark folks also seem open to accepting a PR to make the '??' suffix "None means absent", if you want to take a crack at that.

[milas]

Okay, rebased this PR to just use explicit Unpack() implementations. Given the limited scope for the moment, this will solve the ergonomics issues that were reported, and we can look at making the ?? change to Starlark later (I took a quick look and I think it'll require a decent amount of work, so I don't want to massively explode scope of this issue.)

Only unavoidable downside is that for using structs within other structs, we'll always need to take some extra care, as starlarkstruct does not accept Go nil values by design. Thankfully, this seems to be the first time we've stored a structs in another structs, so it's not very common.

[nicks]

Ya, i'd love to get to a place where a lot of the starlark.Struct -> Value -> Go struct management could be autogenerated, or reflection-based, but that's part of what the goal of all this extensibility/apiserver stuff is.