cmd/cue: revisit get go embedding logic

myitcv commented 3 years ago

What version of CUE are you using (`cue version`)?

$ cue version
cue version +aa61ee7f linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

https://github.com/cuelang/cue/issues/848 raises a question about whether the CUE generated by cue get go (which will become cue import go under #646) is in fact valid. #848 has been closed as "working as intended" but per https://github.com/cuelang/cue/issues/848#issuecomment-805006596 it does appear to raise a couple of issues/questions.

The cue help get go text talks about the JSON inline tag:

        - embedded structs marked with a json inline tag unify with struct
          definition. For instance, the Go struct

However, this tag does not exist as part of encoding/json, it only exists as a proposal: https://github.com/golang/go/issues/6213. This tag does however exist as part of gopkg.in/yaml.v1 and later major versions.

My suggestion would therefore be that we drop this rule for the inline tag and instead, by default, adopt the semantics implied by JSON marshalling. Consider this example:

https://gist.github.com/myitcv/fbf930c45892bbbc22cced812bfbe346

This demonstrates how:

encoding/json marshals values of types S1 and S2 identically, regardless of the presence of the inline tag
encoding/json respects the embedding of T1 and T2 in both S1 and S2, hence values s1 and s2 marshal to the same value (see stdout.golden)
cue get go does behave differently depending on the inline tag: see how #S1 and #S2 differ in main_go_gen.cue.golden

But this raises one further question. The sigs.k8s.io types referenced in #848 have lots of Yaml tags, which implies these values of these types are fairly regularly marshalled to/from Yaml. Given the proposed change in struct rules above, this might well imply a number of users would be discontent with the default:

The rules for Yaml marshalling differ from JSON
There is not a Yaml package in the standard library (https://pkg.go.dev/gopkg.in/yaml.v3 perhaps comes closest)

This seems to point to another requirement: that the caller of cue get go should be able to specify the "rules" applied during the conversion. By default, encoding/json semantics would be applied, with further options available (perhaps in future we could define what it would mean for a Go package to implement and "interface"/API for such conversions?) behind a flag.

cc @nyarly and @verdverm from #848

verdverm commented 3 years ago

Some thoughts

Go struct tags are much like CUE attributes, in that they are purely string based metadata for tools build to read the languages code. There are many Go tools & libraries which make use of these in interesting ways. A JSON inline tag is an interesting grey area case...
Marshalling and Import (get go) are quite different processes. I would think that we want an import to make use of as much information as possible, including any extra struct tags, anti-unification, user driven edge cases, patterns, and overrides through a to-be flag... and also the cue.mod/usr dir
What happens in your example if T1 and T2 share a field name and have the inline tag? This would seem problematic... but what is the user intention here? Could it be to merge some schemas which have some overlap, or unmarshal the same field into multiple structs? I'll have to think on this more in the import vs marshal comparison...
Sane, consistent, and workable default behavior is always a win. Definitely like the idea of users configuration on the import process. Might tie in well if we consider it with a --out go for structs (and/or) import/export for other languages?
If we drop inline, are there cases where a breakage occurs? Do we already have a test that might show this?

cueckoo commented 3 years ago

This issue has been migrated to https://github.com/cue-lang/cue/issues/1026.

For more details about CUE's migration to a new home, please see https://github.com/cue-lang/cue/issues/1078.

cuelang / cue