Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cue: get go should be tweaked now that we have required fields #2678

Open
mvdan opened this issue Nov 6, 2023 · 2 comments
Open

cmd/cue: get go should be tweaked now that we have required fields #2678

mvdan opened this issue Nov 6, 2023 · 2 comments
Labels
get go issues related to cue get go NeedsInvestigation

Comments

@mvdan
Copy link
Member

mvdan commented Nov 6, 2023

cue get go was written back when CUE fields were either regular or optional. CUE v0.6 added required fields per https://github.com/cue-lang/proposal/blob/main/designs/1951-required-fields-v2.md#proposal:

We refer to optional field constraints and required field constraints collectively as “field constraints”.

As a general rule, we consider that all data and data templating should be defined with regular fields, whereas schemas would be defined in terms of field constraints. Of course, CUE being CUE, mixing these two fields is allowed: this rule is not a restriction but suggested as a matter of style and proper usage.

cue get go is about importing Go types as CUE definitions, basically treating them as schemas rather than data. As such, rather than generating CUE fields which are either regular or optional like cue get go does now, we should generate fields which are either required or optional.

The heuristic to decide whether a Go field should become a CUE optional field or not might also need to be tweaked. Right now, most fields are regular unless they have a detail that marks them as optional, such as an omitempty field tag option. Because of this, most fields end up being regular. If we simply made all of those fields required, it might lead to some amount of user breakage given how required fields are stricter than regular fields, requiring the field to be declared elsewhere. We might want to tweak the heuristic to also take into account whether a field type is nilable, because in those cases defaulting to a CUE optional field is probably better than a required field.

cc @uhthomas @myitcv

@mvdan mvdan added NeedsInvestigation get go issues related to cue get go labels Nov 6, 2023
@myitcv
Copy link
Member

myitcv commented Nov 10, 2023

I see that a change for #2679 has landed. I think we need to think about this issue in the context of https://cuelang.org/cl/1171971.

Just dumping some brief thoughts here for now.

Given:

type s struct {
    f1 string
    f2 *string
}

I think the base interpretation from a CUE perspective (irrespective of field tags, comments etc) has to be:

#s_v1: {
    f1?: string
    f2?: string
}

It's the very purpose of optional fields in CUE to express the constraint "if this field is present that it is constrained by X"

Now consider the following possible interpretation of s:

#s_v2: {
    f1!: string
    f2?: string
}

The actual Go code that uses s might well not require f1, interpreting the empty field as meaning not present. Similarly, that same code might require f2 (either generally or in some situations), a constraint that is lost in this interpretation.

As such I'm not clear we can ever be sure of getting a CUE interpretation 100% correct beyond the base interpretation.

That points to the base interpretation #s_v1 being the minimum we do.

But doing only the minimum feels like it leaves a lot on the table for common use cases/patterns where f1 should be interpreted as required (for example, I can't say I know this to be a common pattern).

On top of this we have field tags and, per #2679, field comments, special directives etc.

Some suggestions on next steps:

  • Trying to identify those patterns which we should support
  • Working out how to support/enable those patterns: the UX for how it is enabled, but also what CUE is generated as a result.

The second point there is, I think, key. Because the addition of required fields constraints, for example, cannot be undone. Hence it might be that generating a different interpretation "layer" helps, a layer that can be composed as required.

@mvdan
Copy link
Member Author

mvdan commented Nov 13, 2023

Agreed, @myitcv. When cue get go generated either regular or optional CUE fields, defaulting to a regular field unless we had some form of signal pointing at an optional field (such as a pointer type or omitempty) was fine. Once we replace regular fields with required fields, I feel like the heuristic needs to lean towards optional fields by default instead, because required fields are quite strict. And, in Go, one can always omit declaring, setting, or unmarshaling a regular field, so they don't behave like required fields at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
get go issues related to cue get go NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

2 participants