Merge pull request #493 from DirectXMan12/bug/deterministic-type-checking

🐛 Don't allow generators to interfere with eachother's type checking

Author: k8s-ci-robot

pkg/crd/gen.go

@@ -18,6 +18,7 @@ package crd
 
 import (
 	"fmt"
+	"go/ast"
 	"go/types"
 
 	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -84,6 +85,9 @@ type Generator struct {
 	CRDVersions []string `marker:"crdVersions,optional"`
 }
 
+func (Generator) CheckFilter() loader.NodeFilter {
+	return filterTypesForCRDs
+}
 func (Generator) RegisterMarkers(into *markers.Registry) error {
 	return crdmarkers.Register(into)
 }
@@ -319,3 +323,22 @@ func FindKubeKinds(parser *Parser, metav1Pkg *loader.Package) map[schema.GroupKi
 
 	return kubeKinds
 }
+
+// filterTypesForCRDs filters out all nodes that aren't used in CRD generation,
+// like interfaces and struct fields without JSON tag.
+func filterTypesForCRDs(node ast.Node) bool {
+	switch node := node.(type) {
+	case *ast.InterfaceType:
+		// skip interfaces, we never care about references in them
+		return false
+	case *ast.StructType:
+		return true
+	case *ast.Field:
+		_, hasTag := loader.ParseAstTag(node.Tag).Lookup("json")
+		// fields without JSON tags mean we have custom serialization,
+		// so only visit fields with tags.
+		return hasTag
+	default:
+		return true
+	}
+}

pkg/crd/parser.go

@@ -18,7 +18,6 @@ package crd
 
 import (
 	"fmt"
-	"go/ast"
 
 	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -216,7 +215,7 @@ func (p *Parser) AddPackage(pkg *loader.Package) {
 		return
 	}
 	p.indexTypes(pkg)
-	p.Checker.Check(pkg, filterTypesForCRDs)
+	p.Checker.Check(pkg)
 	p.packages[pkg] = struct{}{}
 }
 
@@ -236,22 +235,3 @@ func (p *Parser) NeedPackage(pkg *loader.Package) {
 	}
 	p.AddPackage(pkg)
 }
-
-// filterTypesForCRDs filters out all nodes that aren't used in CRD generation,
-// like interfaces and struct fields without JSON tag.
-func filterTypesForCRDs(node ast.Node) bool {
-	switch node := node.(type) {
-	case *ast.InterfaceType:
-		// skip interfaces, we never care about references in them
-		return false
-	case *ast.StructType:
-		return true
-	case *ast.Field:
-		_, hasTag := loader.ParseAstTag(node.Tag).Lookup("json")
-		// fields without JSON tags mean we have custom serialization,
-		// so only visit fields with tags.
-		return hasTag
-	default:
-		return true
-	}
-}

pkg/crd/schema_visitor.go

@@ -27,6 +27,10 @@ type SchemaVisitor interface {
 	// this visitor will be called again with `nil` to indicate that
 	// all children have been visited.  If a nil visitor is returned,
 	// children are not visited.
+	//
+	// It is *NOT* safe to save references to the given schema.
+	// Make deepcopies if you need to keep things around beyond
+	// the lifetime of the call.
 	Visit(schema *apiext.JSONSchemaProps) SchemaVisitor
 }
 
@@ -102,6 +106,8 @@ func (w schemaWalker) walkSchema(schema *apiext.JSONSchemaProps) {
 // walkMap walks over values of the given map, saving changes to them.
 func (w schemaWalker) walkMap(defs map[string]apiext.JSONSchemaProps) {
 	for name, def := range defs {
+		// this is iter var reference is because we immediately preseve it below
+		//nolint:gosec
 		w.walkSchema(&def)
 		// make sure the edits actually go through since we can't
 		// take a reference to the value in the map

pkg/deepcopy/deepcopy_integration_test.go

@@ -17,34 +17,52 @@ limitations under the License.
 package deepcopy_test
 
 import (
+	"io"
 	"io/ioutil"
 	"os"
+	"sort"
 
 	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	"golang.org/x/tools/go/packages"
 
+	"sigs.k8s.io/controller-tools/pkg/crd"
 	"sigs.k8s.io/controller-tools/pkg/deepcopy"
+	"sigs.k8s.io/controller-tools/pkg/genall"
 	"sigs.k8s.io/controller-tools/pkg/loader"
 	"sigs.k8s.io/controller-tools/pkg/markers"
 )
 
-func packageErrors(pkg *loader.Package, filterKinds ...packages.ErrorKind) error {
-	toSkip := make(map[packages.ErrorKind]struct{})
-	for _, errKind := range filterKinds {
-		toSkip[errKind] = struct{}{}
+type outputToMap map[string]*outputFile
+
+// Open implements genall.OutputRule.
+func (m outputToMap) Open(_ *loader.Package, path string) (io.WriteCloser, error) {
+	if _, ok := m[path]; !ok {
+		m[path] = &outputFile{}
 	}
-	var outErr error
-	packages.Visit([]*packages.Package{pkg.Package}, nil, func(pkgRaw *packages.Package) {
-		for _, err := range pkgRaw.Errors {
-			if _, skip := toSkip[err.Kind]; skip {
-				continue
-			}
-			outErr = err
-		}
-	})
-	return outErr
+	return m[path], nil
+}
+
+func (m outputToMap) fileList() []string {
+	ret := make([]string, 0, len(m))
+	for path := range m {
+		ret = append(ret, path)
+	}
+	sort.Strings(ret)
+	return ret
+}
+
+type outputFile struct {
+	contents []byte
+}
+
+func (o *outputFile) Write(p []byte) (int, error) {
+	o.contents = append(o.contents, p...)
+	return len(p), nil
+}
+
+func (o *outputFile) Close() error {
+	return nil
 }
 
 var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func() {
@@ -55,35 +73,37 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func
 		Expect(os.Chdir("./testdata")).To(Succeed()) // go modules are directory-sensitive
 		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
 
-		By("loading the roots")
-		pkgs, err := loader.LoadRoots(".")
-		Expect(err).NotTo(HaveOccurred())
-		Expect(pkgs).To(HaveLen(1))
-		cronJobPkg := pkgs[0]
+		output := make(outputToMap)
 
-		By("setting up the generation context")
-		collector := &markers.Collector{Registry: &markers.Registry{}}
-		Expect(deepcopy.Generator{}.RegisterMarkers(collector.Registry)).To(Succeed())
-		checker := &loader.TypeChecker{}
-		ctx := &deepcopy.ObjectGenCtx{
-			Collector: collector,
-			Checker:   checker,
-		}
-
-		By("requesting that types be generated")
-		outContents := ctx.GenerateForPackage(cronJobPkg)
+		By("initializing the runtime")
+		optionsRegistry := &markers.Registry{}
+		Expect(optionsRegistry.Register(markers.Must(markers.MakeDefinition("crd", markers.DescribesPackage, crd.Generator{})))).To(Succeed())
+		Expect(optionsRegistry.Register(markers.Must(markers.MakeDefinition("object", markers.DescribesPackage, deepcopy.Generator{})))).To(Succeed())
+		rt, err := genall.FromOptions(optionsRegistry, []string{
+			"crd", // Run another generator first to make sure they don't interfere; see also: the comment on cronjob_types.go:UntypedBlob
+			"object",
+		})
+		Expect(err).NotTo(HaveOccurred())
+		rt.OutputRules = genall.OutputRules{Default: output}
 
-		By("checking that no errors occurred along the way (expect for type errors)")
-		Expect(packageErrors(cronJobPkg, packages.TypeError)).NotTo(HaveOccurred())
+		By("running the generator and checking for errors")
+		hadErrs := rt.Run()
 
 		By("checking that we got output contents")
+		Expect(output.fileList()).To(ContainElement("zz_generated.deepcopy.go")) // Don't use HaveKey--the output is too verbose to be usable
+		outFile := output["zz_generated.deepcopy.go"]
+		Expect(outFile).NotTo(BeNil())
+		outContents := outFile.contents
 		Expect(outContents).NotTo(BeNil())
 
 		By("loading the desired code")
 		expectedFile, err := ioutil.ReadFile("zz_generated.deepcopy.go")
 		Expect(err).NotTo(HaveOccurred())
 
 		By("comparing the two")
-		Expect(outContents).To(Equal(expectedFile), "generated code not as expected, check pkg/deepcopy/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(outContents, expectedFile))
+		Expect(string(outContents)).To(Equal(string(expectedFile)), "generated code not as expected, check pkg/deepcopy/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(outContents, expectedFile))
+
+		By("checking for errors")
+		Expect(hadErrs).To(BeFalse())
 	})
 })

pkg/deepcopy/gen.go

@@ -58,6 +58,14 @@ type Generator struct {
 	Year string `marker:",optional"`
 }
 
+func (Generator) CheckFilter() loader.NodeFilter {
+	return func(node ast.Node) bool {
+		// ignore interfaces
+		_, isIface := node.(*ast.InterfaceType)
+		return !isIface
+	}
+}
+
 func (Generator) RegisterMarkers(into *markers.Registry) error {
 	if err := markers.RegisterAll(into,
 		enablePkgMarker, legacyEnablePkgMarker, enableTypeMarker,
@@ -144,7 +152,7 @@ func (d Generator) Generate(ctx *genall.GenerationContext) error {
 	}
 
 	for _, root := range ctx.Roots {
-		outContents := objGenCtx.GenerateForPackage(root)
+		outContents := objGenCtx.generateForPackage(root)
 		if outContents == nil {
 			continue
 		}
@@ -186,21 +194,17 @@ import (
 
 }
 
-// GenerateForPackage generates DeepCopy and runtime.Object implementations for
+// generateForPackage generates DeepCopy and runtime.Object implementations for
 // types in the given package, writing the formatted result to given writer.
 // May return nil if source could not be generated.
-func (ctx *ObjectGenCtx) GenerateForPackage(root *loader.Package) []byte {
+func (ctx *ObjectGenCtx) generateForPackage(root *loader.Package) []byte {
 	allTypes, err := enabledOnPackage(ctx.Collector, root)
 	if err != nil {
 		root.AddError(err)
 		return nil
 	}
 
-	ctx.Checker.Check(root, func(node ast.Node) bool {
-		// ignore interfaces
-		_, isIface := node.(*ast.InterfaceType)
-		return !isIface
-	})
+	ctx.Checker.Check(root)
 
 	root.NeedTypesInfo()