Take union of all filters when typechecking

This takes the union of all node filters when type-checking, so that
different generators can't interfere with each other.  Previously, if
you ran a generator with a narrower type filter before one with a broad
filter, you'd get weird results because the narrower filter is the one
that's actually applied to all the shared packages.  Now, we gather all
filters ahead of time, and then use their union: if any filter thinks a
reference is important, we consider that reference important for the
whole context.

Concretely, this manifested if you ran the CRD generator before deepcopy
-- the CRD generator has a much narrower node filter, so you'd get
weird deepcopy results.  If you ran them the other way around, it would
be fine.  Now, either way works.

Author: DirectXMan12

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)
 }
@@ -277,3 +281,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/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,
@@ -196,11 +204,7 @@ func (ctx *ObjectGenCtx) generateForPackage(root *loader.Package) []byte {
 		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()
 

pkg/genall/genall.go

@@ -46,6 +46,32 @@ func (g Generators) RegisterMarkers(reg *markers.Registry) error {
 	return nil
 }
 
+// CheckFilters returns the set of NodeFilters for all Generators that
+// implement NeedsTypeChecking.
+func (g Generators) CheckFilters() []loader.NodeFilter {
+	var filters []loader.NodeFilter
+	for _, gen := range g {
+		withFilter, needsChecking := (*gen).(NeedsTypeChecking)
+		if !needsChecking {
+			continue
+		}
+		filters = append(filters, withFilter.CheckFilter())
+	}
+	return filters
+}
+
+// NeedsTypeChecking indicates that a particular generator needs & has opinions
+// on typechecking.  If this is not implemented, a generator will be given a
+// context with a nil typechecker.
+type NeedsTypeChecking interface {
+	// CheckFilter indicates the loader.NodeFilter (if any) that should be used
+	// to prune out unused types/packages when type-checking (nodes for which
+	// the filter returns true are considered "interesting").  This filter acts
+	// as a baseline -- all types the pass through this filter will be checked,
+	// but more than that may also be checked due to other generators' filters.
+	CheckFilter() loader.NodeFilter
+}
+
 // Generator knows how to register some set of markers, and then produce
 // output artifacts based on loaded code containing those markers,
 // sharing common loaded data.
@@ -144,7 +170,9 @@ func (g Generators) ForRoots(rootPaths ...string) (*Runtime, error) {
 			},
 			Roots:     roots,
 			InputRule: InputFromFileSystem,
-			Checker:   &loader.TypeChecker{},
+			Checker: &loader.TypeChecker{
+				NodeFilters: g.CheckFilters(),
+			},
 		},
 		OutputRules: OutputRules{Default: OutputToNothing},
 	}
@@ -169,6 +197,13 @@ func (r *Runtime) Run() bool {
 	for _, gen := range r.Generators {
 		ctx := r.GenerationContext // make a shallow copy
 		ctx.OutputRule = r.OutputRules.ForGenerator(gen)
+
+		// don't pass a typechecker to generators that don't provide a filter
+		// to avoid accidents
+		if _, needsChecking := (*gen).(NeedsTypeChecking); !needsChecking {
+			ctx.Checker = nil
+		}
+
 		if err := (*gen).Generate(&ctx); err != nil {
 			fmt.Fprintln(os.Stderr, err)
 			hadErrs = true

pkg/loader/refs.go

@@ -185,37 +185,51 @@ func allReferencedPackages(pkg *Package, filterNodes NodeFilter) []*Package {
 // checking each package's types' externally-referenced types, and only
 // type-checking those packages.
 type TypeChecker struct {
+	// NodeFilters are used to filter the set of references that are followed
+	// when typechecking.  If any of the filters returns true for a given node,
+	// its package will be added to the set of packages to check.
+	//
+	// If no filters are specified, all references are followed (this may be slow).
+	//
+	// Modifying this after the first call to check may yield strange/invalid
+	// results.
+	NodeFilters []NodeFilter
+
 	checkedPackages map[*Package]struct{}
-	filterNodes     NodeFilter
 	sync.Mutex
 }
 
-// Check type-checks the given package and all packages referenced
-// by types that pass through (have true returned by) filterNodes.
-func (c *TypeChecker) Check(root *Package, filterNodes NodeFilter) {
+// Check type-checks the given package and all packages referenced by types
+// that pass through (have true returned by) any of the NodeFilters.
+func (c *TypeChecker) Check(root *Package) {
 	c.init()
 
-	if filterNodes == nil {
-		filterNodes = c.filterNodes
-	}
-
 	// use a sub-checker with the appropriate settings
 	(&TypeChecker{
-		filterNodes:     filterNodes,
+		NodeFilters:     c.NodeFilters,
 		checkedPackages: c.checkedPackages,
 	}).check(root)
 }
 
-func (c *TypeChecker) init() {
-	if c.checkedPackages == nil {
-		c.checkedPackages = make(map[*Package]struct{})
+func (c *TypeChecker) isNodeInteresting(node ast.Node) bool {
+	// no filters --> everything is important
+	if len(c.NodeFilters) == 0 {
+		return true
 	}
-	if c.filterNodes == nil {
-		// check every type by default
-		c.filterNodes = func(_ ast.Node) bool {
+
+	// otherwise, passing through any one filter means this node is important
+	for _, filter := range c.NodeFilters {
+		if filter(node) {
 			return true
 		}
 	}
+	return false
+}
+
+func (c *TypeChecker) init() {
+	if c.checkedPackages == nil {
+		c.checkedPackages = make(map[*Package]struct{})
+	}
 }
 
 // check recursively type-checks the given package, only loading packages that
@@ -232,7 +246,7 @@ func (c *TypeChecker) check(root *Package) {
 		return
 	}
 
-	refedPackages := allReferencedPackages(root, c.filterNodes)
+	refedPackages := allReferencedPackages(root, c.isNodeInteresting)
 
 	// first, resolve imports for all leaf packages...
 	var wg sync.WaitGroup

pkg/schemapatcher/gen.go

@@ -32,6 +32,7 @@ import (
 	crdgen "sigs.k8s.io/controller-tools/pkg/crd"
 	crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers"
 	"sigs.k8s.io/controller-tools/pkg/genall"
+	"sigs.k8s.io/controller-tools/pkg/loader"
 	"sigs.k8s.io/controller-tools/pkg/markers"
 	yamlop "sigs.k8s.io/controller-tools/pkg/schemapatcher/internal/yaml"
 )
@@ -87,6 +88,10 @@ type Generator struct {
 
 var _ genall.Generator = &Generator{}
 
+func (Generator) CheckFilter() loader.NodeFilter {
+	return crdgen.Generator{}.CheckFilter()
+}
+
 func (Generator) RegisterMarkers(into *markers.Registry) error {
 	return crdmarkers.Register(into)
 }