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: revisit get go embedding logic #1026

Open
cueckoo opened this issue Jul 3, 2021 · 2 comments
Open

cmd/cue: revisit get go embedding logic #1026

cueckoo opened this issue Jul 3, 2021 · 2 comments
Labels
NeedsDesign Functionality seems desirable, but not sure how it should look like.

Comments

@cueckoo
Copy link
Collaborator

cueckoo commented Jul 3, 2021

Originally opened by @myitcv in cuelang/cue#1026

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?

cuelang/cue#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 cuelang/cue#848 (comment) 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: golang/go#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:

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

@cueckoo cueckoo added NeedsInvestigation Triage Requires triage/attention labels Jul 3, 2021
@cueckoo
Copy link
Collaborator Author

cueckoo commented Jul 3, 2021

Original reply by @verdverm in cuelang/cue#1026 (comment)

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?

@rogpeppe rogpeppe added NeedsDesign Functionality seems desirable, but not sure how it should look like. and removed NeedsInvestigation Triage Requires triage/attention labels Apr 13, 2022
@myitcv myitcv added the zGarden label Jun 15, 2023
@mvdan mvdan removed the zGarden label Feb 8, 2024
@mvdan
Copy link
Member

mvdan commented Aug 13, 2024

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:

I would gently push back against that, because json v2 will support an inline tag (https://pkg.go.dev/github.com/go-json-experiment/json#hdr-JSON_Representation_of_Go_structs), precisely because Go embedding semantics have footguns which are unrelated to encoding semantics. For example, they "promote" methods, which is often undesired when all you want is to inline when encoding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDesign Functionality seems desirable, but not sure how it should look like.
Projects
None yet
Development

No branches or pull requests

4 participants