specs-go: Consistent pointer policy for invalid-value detection?

[wking]

The nominal original policy for this repository was to use pointers for Go properties that were not always REQUIRED. This was codified in #287, and then reversed in #656 to be anti-pointer. However, in #764, maintainers are again asking for pointers. I think we should have a consistent position on this issue, so here's an attempt to summarize the arguments on each side:

Anti-pointer

• Pointers make the Go code more complicated (see @vbatts here and @vishh here). And as evidence that this is a thing, see @cyphar's recent bugfix in validate: fix spec.Hooks nil dereference runtime-tools#361.

• Distinguishing null / unset from the zero value is not useful when both have the same semantics (see @vishh here and here and @mrunalp here)

  In some specific cases:

  • An explicitly empty-string cgroupsPath can mean “let the runtime handle it” which is what unset means (see @mrunap here).

• Distinguishing null / unset from the zero value is not useful when the zero value is invalid (see @crosbymichael here).

  In some specific cases:

  • No need for pointers on Cpus or Mems, since we can have a runtime no-op for all three cases (null / unset / empty-string) (see @crosbymichael here)

• String pointers are never useful (see @crosbymichael here).

Pro-pointer

• Pointers provide a consistent way for Go consumers to determine if the property was null / unset (see me here; @cyphar here; @crosbymichael “where the zero value means something for our settings” here, @hqhq here, @munralp here, @duglin here).

  In some specific cases:

  • @mrunalp here, @hqhq here, and @vishh here and here, and me here on making DisableOOMKiller a pointer to distinguish “set true”, “set false”, and “leave alone”.

  • @cyphar here on a pointer for Cpus so we can fail on an invalid empty-string, backed up by @mrunalp here

  • @crosbymichael here on a pointer for Timeout so we can fail on an invalid zero.

• proposal: encoding/json, encoding/xml: support zero values of structs with omitempty golang/go#11939 means that omitempty requires pointer properties for structs. More on this in Make the Platform field actually optional. image-spec#625.

Consistent policy

While “pointers for every omitempty property is nice and consistent, the consensus position seems to be that the increased complication of pointer handling outweighs the desire for consistency there. That's fine.

We only explicitly support null for some properties inside linux.resources.devices (which I should have removed #656). I haven't heard anyone calling for the ability to distinguish between null and unset in Go, but that's probably orthogonal to the pointer policy because you'd have to use interface{} to do it.

We have to use pointers for optional structs, because of golang/go#11939.

So the contentious point seems to be:

For optional properties where the zero value is invalid (e.g. cpus and, if #764 lands, timeout), do we want to be able to distinguish the cases in Go? Or can Go consumers assume that validation has already been done and treat Go's zero-value cases as “unset”?

We have had maintainers on both sides of that line, and while having an evolving policy is fine, I'd rather not have an inconsistent policy. I'm happy to submit pull requests to unify the repository around whichever position we want to adopt.

[erikh]

What about not addressing the question at all? Just prescribe where nulls are appropriate and let go do what it needs to accomplish that. Seems like the sanest course to me. Not everyone needs/wants to use go anyway.

…

On Sat, Apr 15, 2017 at 12:05 PM W. Trevor King ***@***.***> wrote: The nominal original policy for this repository was to use pointers for Go properties that were not always REQUIRED. This was codified in #287 <#287>, and then reversed in #656 <#656> to be anti-pointer. However, in #764 <#764>, maintainers are again asking for pointers <#764 (comment)>. I think we should have a consistent position on this issue, so here's an attempt to summarize the arguments on each side: Anti-pointer - Pointers make the Go code more complicated (see @vbatts <https://github.com/vbatts> here <#233 (comment)> and @vishh <https://github.com/vishh> here <#233 (comment)>). And as evidence that this is a thing, see @cyphar <https://github.com/cyphar>'s recent bugfix in opencontainers/runtime-tools#361 <opencontainers/runtime-tools#361>. - Distinguishing null / unset from the zero value is not useful when both have the same semantics (see @vishh <https://github.com/vishh> here <#290 (comment)> and here <#316 (comment)> and @mrunalp <https://github.com/mrunalp> here <#290 (comment)>) In some specific cases: - An explicitly empty-string cgroupsPath can mean “let the runtime handle it” which is what unset means <https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config-linux.md#L176> (see @mrunap here <#653 (comment)> ). - Distinguishing null / unset from the zero value is not useful when the zero value is invalid (see @crosbymichael <https://github.com/crosbymichael> here <#653 (comment)> ). In some specific cases: - No need for pointers on Cpus or Mems, since we can have a runtime no-op for all three cases (null / unset / empty-string) (see @crosbymichael <https://github.com/crosbymichael> here <#653 (comment)> ) - String pointers are never useful (see @crosbymichael <https://github.com/crosbymichael> here <http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-01-11-22.04.log.html#l-108> ). Pro-pointer - Pointers provide a consistent way for Go consumers to determine if the property was null / unset (see me here <#233 (comment)>; @cyphar <https://github.com/cyphar> here <#233 (comment)>; @crosbymichael <https://github.com/crosbymichael> “where the zero value means something for our settings” here <#233 (comment)>, @hqhq <https://github.com/hqhq> here <#233 (comment)>, @munralp here <#233 (comment)>, @duglin <https://github.com/duglin> here <#653 (comment)> ). In some specific cases: - @mrunalp <https://github.com/mrunalp> here <#233 (comment)>, @hqhq <https://github.com/hqhq> here <#233 (comment)>, and @vishh <https://github.com/vishh> here <#233 (comment)> and here <#317 (comment)>, and me here <#317 (comment)> on making DisableOOMKiller a pointer to distinguish “set true”, “set false”, and “leave alone”. - @cyphar <https://github.com/cyphar> here <#233 (comment)> on a pointer for Cpus so we can fail on an invalid empty-string, backed up by @mrunalp <https://github.com/mrunalp> here <#233 (comment)> - @crosbymichael <https://github.com/crosbymichael> here <#764 (comment)> on a pointer for Timeout so we can fail on an invalid zero. - golang/go#11939 <golang/go#11939> means that omitempty requires pointer properties for structs. More on this in opencontainers/image-spec#625 <opencontainers/image-spec#625>. Consistent policy While “pointers for every omitempty property is nice and consistent, the consensus position seems to be that the increased complication of pointer handling outweighs the desire for consistency there. That's fine. We only explicitly support null for some properties inside linux.resources.devices <https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config-linux.md#L218-L221> (which I should have removed #656 <#656>). I haven't heard anyone calling for the ability to distinguish between null and unset in Go, but that's probably orthogonal to the pointer policy because you'd have to use interface{} to do it <https://blog.golang.org/json-and-go#TOC_4.>. We have to use pointers for optional structs, because of golang/go#11939 <golang/go#11939>. So the contentious point seems to be: For optional properties where the zero value is invalid (e.g. cpus and, if #764 <#764> lands, timeout), do we want to be able to distinguish the cases in Go? Or can Go consumers assume that validation has already been done and treat Go's zero-value cases as “unset”? We have had maintainers on both sides of that line, and while having an evolving policy is fine, I'd rather not have an inconsistent policy. I'm happy to submit pull requests to unify the repository around whichever position we want to adopt. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#772>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6xyYYp5fbc0QAsTN_n-uGpUZDF1Qks5rwRTmgaJpZM4M-bqm> .

[cyphar]

IMO the primary concern should be the semantics of the spec. Does the spec make distinctions between unset fields and fields set to a "default" value (the Go default value is implementation specific but I've noticed that this usually does co-incide) -- and if it does then we need to make that distinction consistent. I've noted that many of these pointer discussions end up with a bunch of flip-flopping over whether the config should make a distinction between empty and "set to zero".

Once we have an agreement on that point implementing it in a way that makes Go's encoding/json happy is a matter of implementation detail. It might end up having to be done with pointers, but we could also implement it by having a default structure that is then unmarshalled into (AFAIK omitempty attributes won't be modified by encoding/json if they aren't present) or by writing wrappers.

[wking]

On Sun, Apr 16, 2017 at 03:29:35PM -0700, Aleksa Sarai wrote: IMO the primary concern should be the semantics of the spec. Does the spec make distinctions between unset fields and fields set to a "default" value…

Right. In the case where the zero value is invalid (e.g. ‘cpus’ and, if #764 lands, ‘timeout’), I think the spec is making a distinction (at least until it lands wording like “If ‘timeout’ is set to zero it has the same semantics as when it is unset”). And I have no problem with the spec distinguishing between unset (meaning “this hook has no timeout”) and zero (meaning “I haven't thought this property through clearly”). What I'm not clear on is whether the Go types provided by this repository are intending to expose that distinction to their consumers.

I've noted that many of these pointer discussions end up with a bunch of flip-flopping over whether the config should make a distinction between empty and "set to zero".

In cases where there is no distinction (e.g. for cgroupsPath [1]?), I think the current spec style clearly calls for no pointer, and that's fine. I'd like the spec to be clearer about where there is a distinction and where there isn't, but this issue is more about what the Go type should look like when there is a distinction but the zero value is invalid. [1]: #653 (comment)

[wking]

Pointer policy is just about Go, and the Go is non-normative, so this one can get tagged v1.NEXT-maybe. If we don't address it by 1.0, it makes life a bit awkward for folks who'd like to use the 1.0-tagged Go bindings for config compliance testing, but they can always write their own bindings.