cue: decide what a zero Value should be

[mvdan]

@rogpeppe mentions that cue.Value{}.Equals(cue.Value{}) returns false, which is surprising.

He also argues that the zero value should not count as incomplete, because it can't become more complete via any Go API operations.

The behavior of cue.Value{} is currently undocumented and unclear. Go encourages zero values to be meaningful in some way, often as the default behavior for an option or an initial valid state for a method receiver type.

@mpvl's intuition appears to be that a zero cue.Value{} should be _, i.e. "top". Note that this may go against some of Roger's intuition above, e.g. the CUE spec doesn't say that top is comparable, and I'm not sure whether top is incomplete or not.

[myitcv]

Agree with @mpvl's intuition.

He also argues that the zero value should not count as incomplete

@rogpeppe can you provide some more detail on what you mean here? Concretely (no pun intended) what is not possible/allowed today because of the condition you describe? And how would this change it the zero value were made _?

The _ value is not, to my understand, concrete: we cannot manifest it in JSON, Yaml etc. A configuration where a field has a value of _ is therefore considered to be incomplete. What I'm not clear on is how this influences whether we should make _ comparable, or the proposed change to the zero value of cue.Value.

Note that this may go against some of Roger's intuition above, e.g. the CUE spec doesn't say that top is comparable, and I'm not sure whether top is incomplete or not.

Again would appreciate some more clarity on what is allowed/not allowed today in terms of the API with respect to being incomplete or not.

_ not being defined as being comparable is something we need to fix I suspect as part of this change I agree.

[mvdan]

To recap - it seems like @rogpeppe would like cue.Value{} to be "bottom", i.e. equivalent to an error. Advantages:

• The API currently behaves like that somewhat; e.g. cue.Value{}.Kind() == BottomKind, though other methods panic
• Making zero values be "nothing" or a failure in Go is a good practice in terms of defensive programming, e.g. if the API user ends up leaving the value unset by accident, it is way more obvious

And @mpvl would like cue.Value{} to be "top", i.e. any value. Advantages:

• It conceptually aligns with the language; the starting point for any evaluation is "top"
• If we make cue.Value a more flexible and cue.Context-free API in the future, the zero value could be more generally useful as "top" than "bottom", given that "bottom" could be a sentinel value like cue.Bottom

I can edit the list above if I got anything wrong or forgot any arguments for either direction. I currently lean towards "bottom" for the sake of defensive programming, but it's not a strongly held opinion.

[rogpeppe]

Here's my rationale for treating cue.Value as bottom, not top.

To my mind the zero Value is strongly akin to a value that does not exist, as returned (for example) from Value.LookupPath for a non-existent field. I'd find it hard to justify cue.Value{}.Exists() returning true, for example.

The existing behaviour is also strongly aligned with that; for the zero Value:

• Exists returns false
• Kind and IncompleteKind return cue.BottomKind
• Err returns an "undefined value" error
• Expr returns NoOp and an empty list of args
• FillPath returns an error value

Making the zero Value behave more like top would require changing all of those, and hence it would be be a very significant breaking change that I don't think we could do without making an entirely new major version of the API.

I haven't yet seen strong arguments for making that change and, as I said above, I think there's a good intuitive sense for treating it as a non-existent value. As @mvdan says, it's also more defensive that way around.

[mvdan]

Making the zero Value behave more like top would require changing all of those, and hence it would be be a very significant breaking change that I don't think we could do without making an entirely new major version of the API.

I generally agree with your position and reasoning, but I don't think we need to worry too much about existing behavior, given that a zero cue.Value{} was never defined behavior and it often resulted in wrong or panicking results.

[rogpeppe]

Making the zero Value behave more like top would require changing all of those, and hence it would be be a very significant breaking change that I don't think we could do without making an entirely new major version of the API.

I generally agree with your position and reasoning, but I don't think we need to worry too much about existing behavior, given that a zero cue.Value{} was never defined behavior and it often resulted in wrong or panicking results.

In practice, AFAIK the only way to check that a cue.Value is valid to use is by using Value.Exists, and I've certainly relied on that behaviour in the past, so I suspect many others will do. Similarly for Value.Err: it seems reasonable that people can rely on that method returning an error for the zero Value both now and in the future.

I think it would be hard to argue that a value could both be "top" and "non-existent" or have an error.

[mvdan]

I think it would be hard to argue that a value could both be "top" and "non-existent" or have an error.

I'm not saying that. I'm saying that we could make the zero value be anything we want at this point, because the current behavior is undocumented, buggy, and leads to some panics. I don't think we need to consider it a hard breaking change in terms of the Go API. Being clear in the release notes about the change should suffice.

[rogpeppe]

I'm saying that we could make the zero value be anything we want at this point

I disagree. Although not documented, the current API has at least been stable for a long time in many respects, and there are times when it's truly necessary to invoke methods on the zero Value.

For example:

package main

import (
	"fmt"
	"log"

	"cuelang.org/go/cue"
	"cuelang.org/go/cue/cuecontext"
)

func main() {
	ctx := cuecontext.New()

	var s struct {
		F cue.Value `json:"f"`
		G cue.Value `json:"g"`
	}
	err := ctx.CompileString("{f: 1}").Decode(&s)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(s.F.Exists(), s.G.Exists())
}

In the above example, using Exists to determine whether a field has been filled in seems perfectly reasonable: I'm not sure there's a better way to do it, and I'm not sure how this functionality would work if the zero Value were to become "top".

[mpvl]

It really should be top. And the API should be fixed if it is inconsistent.

We probably should start coming up with a better API alltogheter. But for now it is what it is. Ive used the top behavior a lot and it would be a badly breaking change to change it, even if it were the right thing to do.

[rogpeppe]

Ive used the top behavior a lot and it would be a badly breaking change to change it,

Which aspect of the behaviour is that? The only current behaviour I can see that aligns with top is that of Unify.

I experimented by changing Value.Unify to panic if called with a zero value (either receiver or argument), and all our code continues to run fine and all the tests pass (†)

On the other hand, when I change Value.Exists to return true on the zero value, many tests fail or panic.

So from a purely pragmatic point of view, it seems more appropriate to treat the zero value as a non-existent (bottom) value.

Note that I'm not proposing to change the behaviour of Unify at this point, just rationalise the rest of the methods that currently panic. Going with the "non-existent" semantics seems to end up at a considerably more consistent place to my mind.

For the record, here are some of the places our current code relies on Value.Exists returning false for the zero Value:

https://github.com/cue-lang/cue/blob/53ace208689e6362645d0c6d48abe12e6c731b3f/encoding/jsonschema/constraints_combinator.go#L196 https://github.com/cue-lang/cue/blob/53ace208689e6362645d0c6d48abe12e6c731b3f/encoding/jsonschema/decode.go#L180 https://github.com/cue-lang/cue/blob/53ace208689e6362645d0c6d48abe12e6c731b3f/internal/task/task.go#L152 https://github.com/cue-lang/cue/blob/53ace208689e6362645d0c6d48abe12e6c731b3f/encoding/protobuf/jsonpb/encoder_test.go#L59

† with one exception in encoding/protobuf/textproto/encoder_test.go, which is trivially worked around.