Fix defaulter-gen for recursive types #61

nikhita · 2017-06-09T17:26:51Z

For recursive types, the defaulter currently goes into an infinite recursion.
Fix to support recursive types.

k8s-reviewable · 2017-06-09T17:26:57Z

This change is

nikhita · 2017-06-09T17:27:23Z

examples/defaulter-gen/generators/defaulter.go

@@ -320,7 +320,8 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
 				if d.object != nil {
 					continue
 				}
-				if buildCallTreeForType(t, true, existingDefaulters, newDefaulters) != nil {
+				existingTypes := make(map[string]bool)


PTAL if this is the right way to do it.

sttts · 2017-06-09T17:30:16Z

examples/defaulter-gen/generators/defaulter.go

@@ -396,7 +397,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
 // that could be or will be generated. If newDefaulters has an entry for a type, but the 'object' field is nil,
 // this function skips adding that defaulter - this allows us to avoid generating object defaulter functions for
 // list types that call empty defaulters.
-func buildCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap) *callNode {
+func buildCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap, existingTypes map[string]bool) *callNode {


Can you document existingTypes? Is it feasible to have a recursive type in some test? Are there tests here in the generator?

Will document it.
Currently there are no tests. But I'll add one.

sttts · 2017-06-09T17:31:12Z

examples/defaulter-gen/generators/defaulter.go

+		return parent
+	}
+	// if type doesn't exist, mark it as existing
+	existingTypes[t.Name.Name] = true


shouldn't we check that Name is actually files, i.e. not an anonymous type? Otherwise, we stop too early for those, don't we?

So I should check for t.Name?

If I read the type godoc correctly, Name can be empty.

sttts · 2017-06-09T17:35:39Z

Can you link a commit for the Kubernetes defaulters? Do they change after this PR?

thockin · 2017-06-10T19:15:33Z

This could really use output tests like https://github.com/kubernetes/gengo/tree/master/examples/deepcopy-gen/output_tests

nikhita · 2017-06-11T21:48:09Z

Updated the defaulter code. PTAL.

I have added some tests. However, I come across the following error when I run make: https://gist.github.com/nikhita/379c3789b8049038109b1ea1509b483b. What could be the possible reason for this error?

It works when there is no defaults.go file though. I have also pushed the file generated in this case.

After making the changes in this PR and fixing the conversion in kubernetes/kubernetes#47263, nothing breaks/changes in Kubernetes (tested against HEAD). This PR shows the cumulative changes in both the defaulter and converter: nikhita/kubernetes#3.

nikhita · 2017-06-11T21:52:36Z

examples/defaulter-gen/output_tests/builtins/defaults.go

@@ -0,0 +1,36 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Oops, will fix the year after the review.

nikhita · 2017-06-12T08:55:56Z

The tests are being added in #55. Will create a PR to add a test for recursive types on top of #55.

nikhita · 2017-06-12T12:32:40Z

Added tests on top of #55 here: caesarxuchao#1.

sttts · 2017-06-12T13:15:34Z

lgtm.

@thockin ptal at https://github.com/caesarxuchao/gengo/pull/1/files for example output.

sttts · 2017-07-03T15:10:44Z

examples/defaulter-gen/generators/defaulter.go

-				if buildCallTreeForType(t, true, existingDefaulters, newDefaulters) != nil {
+				// existingTypes keeps track of types that have already been visited in the tree.
+				// This is used to avoid recursion for recursive types.
+				existingTypes := make(map[*types.Type]bool)


s/existingTypes/alreadyVisitedTypes/

sttts · 2017-07-03T15:19:10Z

@wojtek-t can this go in? (you are in the OWNERs files, together with @lavalamp. Though the merge+review policy here is unclear to me looking at previous PRs).

lavalamp · 2017-07-05T22:08:53Z

examples/defaulter-gen/generators/defaulter.go

+
+	// The type now acts as a parent, not a nested recursive type.
+	// We can now build the tree for it safely.
+	alreadyVisitedTypes[t] = false


The name is wrong if you set it to false after you are done. Should be "currentlyBuildingTypes" or "typesInProgress" or something.

lavalamp · 2017-07-05T22:10:00Z

examples/defaulter-gen/generators/defaulter.go

@@ -396,7 +399,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
 // that could be or will be generated. If newDefaulters has an entry for a type, but the 'object' field is nil,
 // this function skips adding that defaulter - this allows us to avoid generating object defaulter functions for
 // list types that call empty defaulters.
-func buildCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap) *callNode {
+func buildCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap, alreadyVisitedTypes map[*types.Type]bool) *callNode {


Consider making a struct to hold all of the parameters that don't change, and make this function a method on that struct. Will make the code a lot easier to read.

lavalamp · 2017-07-05T22:11:11Z

(happy to lgtm & merge after those comments are addressed)

nikhita · 2017-07-06T13:02:47Z

@lavalamp Updated.

lavalamp

Sorry for not being clear but that wasn't quite what I had in mind.

lavalamp · 2017-07-06T16:58:48Z

examples/defaulter-gen/generators/defaulter.go

@@ -387,6 +391,25 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
 	return packages
 }

+// callTreeForType contains fields necessary to call buildCallTreeForType.
+type callTreeForType struct {
+	t                      *types.Type


remove; this struct should have only the immutable parameters.

lavalamp · 2017-07-06T16:59:02Z

examples/defaulter-gen/generators/defaulter.go

+// callTreeForType contains fields necessary to call buildCallTreeForType.
+type callTreeForType struct {
+	t                      *types.Type
+	root                   bool


remove; this struct should have only the immutable parameters.

lavalamp · 2017-07-06T16:59:16Z

examples/defaulter-gen/generators/defaulter.go

+	currentlyBuildingTypes map[*types.Type]bool
+}
+
+func NewCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap, currentlyBuildingTypes map[*types.Type]bool) callTreeForType {


Delete this or change to:

func NewCallTreeForType(existingDefaulters, newDefaulters defaulterFuncMap) *callTreeForType { return &callTreeForType{ existingDefaulters: existingDefaulters, newDefaulters: newDefaulters, currentlyBuildingTypes: make(map[*types.Type]bool), } }

We can't initialize currentlyBuildingTypes in newCallTreeForType because when this is encountered:

r := newCallTreeForType(c.existingDefaulters, c.newDefaulters) if child := r.build(t.Elem, false); child != nil

it will create a new map for currentlyBuildingTypes - destroying our purpose for storing the types that are currently building. :)

lavalamp · 2017-07-06T17:05:40Z

examples/defaulter-gen/generators/defaulter.go

 	case types.Pointer:
-		if child := buildCallTreeForType(t.Elem, false, existingDefaulters, newDefaulters); child != nil {
+		r := NewCallTreeForType(c.t.Elem, false, c.existingDefaulters, c.newDefaulters, c.currentlyBuildingTypes)
+		if child := r.buildCallTreeForType(); child != nil {


All of these should look like if child := r.Build(c.t.Elem, false); child != nil {

lavalamp · 2017-07-06T17:05:52Z

examples/defaulter-gen/generators/defaulter.go

@@ -396,22 +419,22 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
 // that could be or will be generated. If newDefaulters has an entry for a type, but the 'object' field is nil,
 // this function skips adding that defaulter - this allows us to avoid generating object defaulter functions for
 // list types that call empty defaulters.
-func buildCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap) *callNode {
+func (c callTreeForType) buildCallTreeForType() *callNode {


func (c *callTreeForType) Build(t *types.Type, root bool) *callNode {

lavalamp · 2017-07-06T17:06:00Z

examples/defaulter-gen/generators/defaulter.go

+				// This is used to avoid recursion for recursive types.
+				currentlyBuildingTypes := make(map[*types.Type]bool)
+				c := NewCallTreeForType(t, true, existingDefaulters, newDefaulters, currentlyBuildingTypes)
+				if c.buildCallTreeForType() != nil {


This should be a one-liner:

if NewCallTreeForType(existingDefaulters, newDefaulters).Build(t, true) != nil {

lavalamp · 2017-07-06T17:07:51Z

examples/defaulter-gen/generators/defaulter.go

+	currentlyBuildingTypes map[*types.Type]bool
+}
+
+func NewCallTreeForType(t *types.Type, root bool, existingDefaulters, newDefaulters defaulterFuncMap, currentlyBuildingTypes map[*types.Type]bool) callTreeForType {


Actually both this and Build should be private.

nikhita · 2017-07-06T21:23:43Z

@lavalamp Updated. PTAL, thanks.

lavalamp · 2017-07-06T21:26:46Z

examples/defaulter-gen/generators/defaulter.go

 	switch t.Kind {
 	case types.Pointer:
-		if child := buildCallTreeForType(t.Elem, false, existingDefaulters, newDefaulters); child != nil {
+		r := newCallTreeForType(c.existingDefaulters, c.newDefaulters, c.currentlyBuildingTypes)


What I was trying to communicate is that you don't call newCallTreeForType here, you reuse c.

I think I wrote an r where I meant a c--sorry about that.

Right, sorry - should have done this in the first place.

Updated. Please check if this is better.

lavalamp · 2017-07-06T22:47:06Z

examples/defaulter-gen/generators/defaulter.go

+
+	// The type now acts as a parent, not a nested recursive type.
+	// We can now build the tree for it safely.
+	c.currentlyBuildingTypes[t] = false


Looking good--last question, should this be in a defer block? The return on line 496 skips it, for example.

Rename Address comments Address further comments Use defer

lavalamp · 2017-07-06T23:38:53Z

LGTM, thanks for bearing with me :)

@sttts

Automatic merge from submit-queue apiextensions: validation for customresources - [x] Add types for validation of CustomResources - [x] Fix conversion-gen: #49747 - [x] Fix defaulter-gen: kubernetes/gengo#61 - [x] Convert to OpenAPI types - [x] Validate CR using go-openapi - [x] Validate CRD Schema - [x] Add integration tests - [x] Fix round trip tests: #51204 - [x] Add custom fuzzer functions - [x] Add custom conversion functions - [x] Fix data race while updating CRD: #50098 - [x] Add feature gate for CustomResourceValidation - [x] Fix protobuf generation Proposal: kubernetes/community#708 Additional discussion: #49879, #50625 **Release note**: ```release-note Add validation for CustomResources via JSON Schema. ``` /cc @sttts @deads2k

@sttts

Automatic merge from submit-queue apiextensions: validation for customresources - [x] Add types for validation of CustomResources - [x] Fix conversion-gen: #49747 - [x] Fix defaulter-gen: kubernetes/gengo#61 - [x] Convert to OpenAPI types - [x] Validate CR using go-openapi - [x] Validate CRD Schema - [x] Add integration tests - [x] Fix round trip tests: #51204 - [x] Add custom fuzzer functions - [x] Add custom conversion functions - [x] Fix data race while updating CRD: #50098 - [x] Add feature gate for CustomResourceValidation - [x] Fix protobuf generation Proposal: kubernetes/community#708 Additional discussion: kubernetes/kubernetes#49879, kubernetes/kubernetes#50625 **Release note**: ```release-note Add validation for CustomResources via JSON Schema. ``` /cc @sttts @deads2k Kubernetes-commit: 4457e43e7b789586096bfb564330295cf0438e70

k8s-ci-robot requested a review from sttts June 9, 2017 17:26

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017

nikhita commented Jun 9, 2017

View reviewed changes

sttts reviewed Jun 9, 2017

View reviewed changes

nikhita mentioned this pull request Jun 9, 2017

apiextensions: validation for customresources kubernetes/kubernetes#47263

Merged

13 tasks

nikhita changed the title ~~Fix defaulter for recursive types~~ [WIP] Fix defaulter for recursive types Jun 9, 2017

nikhita mentioned this pull request Jun 10, 2017

validation: fix code generation for recursive types nikhita/gsoc-meta-k8s#19

Closed

nikhita force-pushed the fix-recursive-types branch from 331e754 to ba7ac3b Compare June 11, 2017 21:14

nikhita commented Jun 11, 2017

View reviewed changes

nikhita force-pushed the fix-recursive-types branch from 9929cff to ba7ac3b Compare June 12, 2017 08:35

nikhita changed the title ~~[WIP] Fix defaulter for recursive types~~ Fix defaulter-gen for recursive types Jun 12, 2017

nikhita mentioned this pull request Jun 12, 2017

Add tests for recursive types in defaulter-gen caesarxuchao/gengo#1

Closed

sttts reviewed Jul 3, 2017

View reviewed changes

nikhita force-pushed the fix-recursive-types branch from ba7ac3b to 5f0312b Compare July 3, 2017 15:27

lavalamp reviewed Jul 5, 2017

View reviewed changes

nikhita force-pushed the fix-recursive-types branch from 5f0312b to ce68359 Compare July 6, 2017 12:48

lavalamp requested changes Jul 6, 2017

View reviewed changes

nikhita force-pushed the fix-recursive-types branch from ce68359 to 644e318 Compare July 6, 2017 21:21

lavalamp reviewed Jul 6, 2017

View reviewed changes

nikhita force-pushed the fix-recursive-types branch from 644e318 to 9a0932b Compare July 6, 2017 21:38

lavalamp reviewed Jul 6, 2017

View reviewed changes

Fix defaulter for nested recursive types

96fe5d0

Rename Address comments Address further comments Use defer

nikhita force-pushed the fix-recursive-types branch from 9a0932b to 96fe5d0 Compare July 6, 2017 23:15

lavalamp approved these changes Jul 6, 2017

View reviewed changes

lavalamp merged commit d354881 into kubernetes:master Jul 6, 2017

nikhita deleted the fix-recursive-types branch July 6, 2017 23:39

nikhita restored the fix-recursive-types branch July 7, 2017 07:32

nikhita mentioned this pull request Jul 7, 2017

update vendored gengo kubernetes/kubernetes#48630

Closed

Fix defaulter-gen for recursive types #61

Fix defaulter-gen for recursive types #61

Conversation

nikhita commented Jun 9, 2017

k8s-reviewable commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jun 9, 2017

thockin commented Jun 10, 2017

nikhita commented Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

nikhita commented Jun 12, 2017 • edited Loading

nikhita commented Jun 12, 2017

sttts commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jul 5, 2017

nikhita commented Jul 6, 2017

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jul 6, 2017

nikhita commented Jun 11, 2017 •

edited

Loading

nikhita commented Jun 12, 2017 •

edited

Loading