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

[should gengo add] Support for 1.18 Generics #225

Closed
austince opened this issue Jun 6, 2022 · 10 comments
Closed

[should gengo add] Support for 1.18 Generics #225

austince opened this issue Jun 6, 2022 · 10 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@austince
Copy link

austince commented Jun 6, 2022

When experimenting with using go 1.18 generics in some k8s type definitions, I'm hitting errors (as one would expect with such a large language change). I'm running conversion, defaulter, and deepcopy generation on the following types.

type ListItems[T any] struct {
	Items []T `json:"items"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

type TestObjectList struct {
	metav1.TypeMeta              `json:",inline"`
	metav1.ListMeta              `json:"metadata"`
	metav1.ListItems[TestObject] `json:",inline"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

type TestObject struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata"`
}

The errors seem to start with the gengo parser not having a case for the generic type param:

gengo/parser/parse.go

Lines 857 to 865 in 4627b89

default:
out := u.Type(name)
if out.Kind != types.Unknown {
return out
}
out.Kind = types.Unsupported
klog.Warningf("Making unsupported type entry %q for: %#v\n", out, t)
return out
}

conversion generation
W0606 16:15:58.292211  133284 parse.go:862] Making unsupported type entry "T" for: &types.TypeParam{check:(*types.Checker)(nil), id:0x1, obj:(*types.TypeName)(0xc008bb1630), index:0, bound:(*types.Interface)(0xc0000942a0)}

defaulter generation
W0606 16:16:03.505939  135029 parse.go:862] Making unsupported type entry "T" for: &types.TypeParam{check:(*types.Checker)(nil), id:0x1, obj:(*types.TypeName)(0xc009fa7e50), index:0, bound:(*types.Interface)(0xc00007e2a0)}

deepcopy generation
W0606 16:16:14.790210  136792 parse.go:862] Making unsupported type entry "T" for: &types.TypeParam{check:(*types.Checker)(nil), id:0x1, obj:(*types.TypeName)(0xc00b6160f0), index:0, bound:(*types.Interface)(0xc00007e2a0)}
F0606 16:16:14.806387  136792 deepcopy.go:816] Hit an unsupported type T for []T
goroutine 1 [running]:
k8s.io/klog/v2.stacks(0x1)
        /home/austin/go/pkg/mod/k8s.io/klog/v2@v2.60.1/klog.go:860 +0x8a
k8s.io/klog/v2.(*loggingT).output(0x9cd3a0, 0x3, 0x0, 0xc00b7a0fc0, 0x1, {0x85c821?, 0x2?}, 0xc000800400?, 0x0)
        /home/austin/go/pkg/mod/k8s.io/klog/v2@v2.60.1/klog.go:825 +0x686
k8s.io/klog/v2.(*loggingT).printfDepth(0x9cd3a0, 0x0?, 0x0, {0x0, 0x0}, 0xc00c068180?, {0x77f15b, 0x21}, {0xc00c036a40, 0x2, ...})
        /home/austin/go/pkg/mod/k8s.io/klog/v2@v2.60.1/klog.go:630 +0x1f2
k8s.io/klog/v2.(*loggingT).printf(...)
        /home/austin/go/pkg/mod/k8s.io/klog/v2@v2.60.1/klog.go:612
k8s.io/klog/v2.Fatalf(...)
        /home/austin/go/pkg/mod/k8s.io/klog/v2@v2.60.1/klog.go:1516
k8s.io/gengo/examples/deepcopy-gen/generators.(*genDeepCopy).doSlice(0xc00c068690?, 0xc00c0686c0?, 0x1?)
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/examples/deepcopy-gen/generators/deepcopy.go:816 +0x4fa
k8s.io/gengo/examples/deepcopy-gen/generators.(*genDeepCopy).generateFor(0xc00c058190?, 0x781bd2?, 0x25?)
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/examples/deepcopy-gen/generators/deepcopy.go:698 +0x403
k8s.io/gengo/examples/deepcopy-gen/generators.(*genDeepCopy).doStruct(0xc00c053f80?, 0x75a5ab?, 0x8?)
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/examples/deepcopy-gen/generators/deepcopy.go:868 +0x46c
k8s.io/gengo/examples/deepcopy-gen/generators.(*genDeepCopy).generateFor(0xc00c058190?, 0x78793f?, 0x38?)
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/examples/deepcopy-gen/generators/deepcopy.go:698 +0x403
k8s.io/gengo/examples/deepcopy-gen/generators.(*genDeepCopy).GenerateType(0xf0?, 0xc00b5fbf00, 0xc00b95afc0, {0x7e4a88?, 0xc00b70e8a0})
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/examples/deepcopy-gen/generators/deepcopy.go:611 +0x765
k8s.io/gengo/generator.(*Context).executeBody(0xc00b5fbf00, {0x7e4388?, 0xc00a64c2a0}, {0x7e83c8, 0xc00b5fbc80})
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/generator/execute.go:321 +0x132
k8s.io/gengo/generator.(*Context).ExecutePackage(0xc00b791700, {0x7ffe15c6bd3d, 0x2}, {0x7e7b50, 0xc00ba3e240})
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/generator/execute.go:282 +0xf9a
k8s.io/gengo/generator.(*Context).ExecutePackages(0xc00b791700?, {0x7ffe15c6bd3d, 0x2}, {0xc00ba4ec00?, 0x4, 0xc0000e3d78?})
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/generator/execute.go:51 +0x1dc
k8s.io/gengo/args.(*GeneratorArgs).Execute(0xc0000f2420, 0xc0000e3e30?, {0x758a59, 0x6}, 0x78acc8)
        /home/austin/go/pkg/mod/k8s.io/gengo@v0.0.0-20211129171323-c02415ce4185/args/args.go:213 +0x1c8
main.main()

goroutine 355 [runnable]:
text/template/parse.(*lexer).emit(...)
        /home/austin/.asdf/installs/golang/1.18/go/src/text/template/parse/lex.go:163
text/template/parse.lexText(0xc00b64c900)
        /home/austin/.asdf/installs/golang/1.18/go/src/text/template/parse/lex.go:275 +0x426
text/template/parse.(*lexer).run(0xc00b64c900)
        /home/austin/.asdf/installs/golang/1.18/go/src/text/template/parse/lex.go:236 +0x2a
created by text/template/parse.lex
        /home/austin/.asdf/installs/golang/1.18/go/src/text/template/parse/lex.go:229 +0x1ca

I'm wondering if anyone would take a guess on how complex this would be to add support for? From brief experiments, it seems the answer may be "very hard" 😬 .

@lavalamp
Copy link
Member

lavalamp commented Jun 6, 2022

I think the question we need to answer first is if gengo is still needed at all post-generics.

@austince
Copy link
Author

austince commented Jun 7, 2022

That's an interesting question. I guess I don't fully understand the responsibilities of gengo, but naively would think it loosely breaks down into: parsing comment markers to make up for lack of annotations (and more generally, comments on fields/types) & generating functions for given types. If we were able to write generic functions for deepcopy, conversion, defaulting, we'd still need something to parse comment markers, no?

@lavalamp
Copy link
Member

lavalamp commented Jun 7, 2022

I mean, if we had C++ template power, we could definitely get rid of gengo and just use various template magics to put in doc strings, add attributes etc. I think what we got in go 1.18 isn't powerful enough for that, but I haven't thought about it super hard yet?

@austince
Copy link
Author

austince commented Jun 7, 2022

Yeah, agreed — not sure the go 1.18 type parameters are enough, though maybe there's some creative thinking to be done. In any case, being able to leverage them in API types (and elsewhere) may get us closer to removing a lot of nasty reflection business in kube-apiserver.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 5, 2022
@austince
Copy link
Author

austince commented Sep 6, 2022

Any thoughts come up in the last few months @lavalamp? Should we just close this?

@lavalamp
Copy link
Member

lavalamp commented Sep 6, 2022

Nope haven't had time to consider this much at all, I think we should leave the issue open, I'd like more voices to weigh in.

@lavalamp lavalamp changed the title Support for 1.18 Generics [should gengo add] Support for 1.18 Generics Sep 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

4 participants