feat: add non-fatal warning system for code generation diagnostics

Add a warning infrastructure that mirrors the existing error reporting
system but is non-fatal. The first use case is warning about pointers
as map values, which is supported but not recommended per Kubernetes
API conventions.

- Add Package.Warnings field and AddWarning method to pkg/loader
- Add PrintWarnings and VisitPackages helper functions
- Add Runtime.PrintWarnings method to pkg/genall
- Call PrintWarnings after successful generation in controller-gen
- Emit warning when pointers are used as map values in CRD schemas
- Add tests for pointer map value warning and non-warning cases

Closes: #434

Author: mattsu2020

cmd/controller-gen/main.go

@@ -188,6 +188,8 @@ func main() {
 				// don't obscure the actual error with a bunch of usage
 				return noUsageError{fmt.Errorf("not all generators ran successfully")}
 			}
+
+			rt.PrintWarnings()
 			return nil
 		},
 		SilenceUsage: true, // silence the usage, then print it out ourselves if it wasn't suppressed

pkg/crd/schema.go

@@ -414,6 +414,7 @@ func mapToSchema(ctx *schemaContext, mapType *ast.MapType) *apiextensionsv1.JSON
 	case *ast.ArrayType:
 		valSchema = arrayToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)
 	case *ast.StarExpr:
+		ctx.pkg.AddWarning(loader.ErrFromNode(fmt.Errorf("using pointers as map values is not recommended per Kubernetes API conventions"), mapType.Value))
 		valSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)
 	case *ast.MapType:
 		valSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)

pkg/crd/schema_test.go

@@ -176,3 +176,80 @@ func (m *testapplyFirstMarker) ApplyToSchema(*crdmarkers.SchemaContext, *apiexte
 	m.callback()
 	return nil
 }
+
+func Test_Schema_PointerMapValue_Warning(t *testing.T) {
+	g := gomega.NewWithT(t)
+
+	moduleName := "sigs.k8s.io/controller-tools/pkg/crd"
+	modules := []pkgstest.Module{
+		{
+			Name: moduleName,
+			Files: map[string]any{
+				"test.go": `package crd
+type Test map[string]*string
+`,
+			},
+		},
+	}
+
+	pkgs, exported, err := testloader.LoadFakeRoots(pkgstest.Modules, modules, moduleName)
+	if exported != nil {
+		t.Cleanup(exported.Cleanup)
+	}
+	if err != nil {
+		t.Fatalf("unable to load fake package: %s", err)
+	}
+	if len(pkgs) != 1 {
+		t.Fatal("expected to parse only one package")
+	}
+
+	pkg := pkgs[0]
+	pkg.NeedTypesInfo()
+	failIfErrors(t, pkg.Errors)
+
+	schemaContext := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
+	definedType := pkg.Syntax[0].Decls[0].(*ast.GenDecl).Specs[0].(*ast.TypeSpec).Type
+	typeToSchema(schemaContext, definedType)
+
+	g.Expect(pkg.Errors).To(gomega.BeEmpty(), "pointer map values should not produce errors")
+	g.Expect(pkg.Warnings).To(gomega.HaveLen(1), "expected exactly one warning")
+	g.Expect(pkg.Warnings[0].Msg).To(gomega.ContainSubstring("pointers as map values"))
+}
+
+func Test_Schema_NonPointerMapValue_NoWarning(t *testing.T) {
+	g := gomega.NewWithT(t)
+
+	moduleName := "sigs.k8s.io/controller-tools/pkg/crd"
+	modules := []pkgstest.Module{
+		{
+			Name: moduleName,
+			Files: map[string]any{
+				"test.go": `package crd
+type Test map[string]string
+`,
+			},
+		},
+	}
+
+	pkgs, exported, err := testloader.LoadFakeRoots(pkgstest.Modules, modules, moduleName)
+	if exported != nil {
+		t.Cleanup(exported.Cleanup)
+	}
+	if err != nil {
+		t.Fatalf("unable to load fake package: %s", err)
+	}
+	if len(pkgs) != 1 {
+		t.Fatal("expected to parse only one package")
+	}
+
+	pkg := pkgs[0]
+	pkg.NeedTypesInfo()
+	failIfErrors(t, pkg.Errors)
+
+	schemaContext := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
+	definedType := pkg.Syntax[0].Decls[0].(*ast.GenDecl).Specs[0].(*ast.TypeSpec).Type
+	typeToSchema(schemaContext, definedType)
+
+	g.Expect(pkg.Errors).To(gomega.BeEmpty())
+	g.Expect(pkg.Warnings).To(gomega.BeEmpty(), "non-pointer map values should not produce warnings")
+}

pkg/genall/genall.go

@@ -281,3 +281,8 @@ func (r *Runtime) Run() bool {
 	// skip TypeErrors -- they're probably just from partial typechecking in crd-gen
 	return loader.PrintErrors(r.Roots, packages.TypeError) || hadErrs
 }
+
+// PrintWarnings prints any non-fatal warnings accumulated during generation.
+func (r *Runtime) PrintWarnings() {
+	loader.PrintWarnings(r.Roots)
+}

pkg/loader/loader.go

@@ -63,6 +63,41 @@ func PrintErrors(pkgs []*Package, filterKinds ...packages.ErrorKind) bool {
 	return hadErrors
 }
 
+// PrintWarnings prints warnings associated with all packages
+// in the given package graph, starting at the given root
+// packages and traversing through all imports.  It will
+// return true if any warnings were printed.
+func PrintWarnings(pkgs []*Package) bool {
+	hadWarnings := false
+	VisitPackages(pkgs, func(pkg *Package) {
+		for _, warn := range pkg.Warnings {
+			hadWarnings = true
+			fmt.Fprintln(os.Stderr, "Warning: ", warn)
+		}
+	})
+	return hadWarnings
+}
+
+// VisitPackages visits all packages reachable from the given root packages,
+// calling the given visitor for each unique package exactly once.
+func VisitPackages(pkgs []*Package, visitor func(*Package)) {
+	visited := make(map[*Package]struct{})
+	var visit func(pkg *Package)
+	visit = func(pkg *Package) {
+		if _, done := visited[pkg]; done {
+			return
+		}
+		visited[pkg] = struct{}{}
+		visitor(pkg)
+		for _, imported := range pkg.Imports() {
+			visit(imported)
+		}
+	}
+	for _, pkg := range pkgs {
+		visit(pkg)
+	}
+}
+
 // Package is a single, unique Go package that can be
 // lazily parsed and type-checked.  Packages should not
 // be constructed directly -- instead, use LoadRoots.
@@ -72,6 +107,10 @@ func PrintErrors(pkgs []*Package, filterKinds ...packages.ErrorKind) bool {
 type Package struct {
 	*packages.Package
 
+	// Warnings stores non-fatal warnings encountered during code generation.
+	// Unlike errors, warnings do not cause generation to fail.
+	Warnings []packages.Error
+
 	imports map[string]*Package
 
 	loader *loader
@@ -177,6 +216,49 @@ func (p *Package) AddError(err error) {
 	}
 }
 
+// AddWarning adds a non-fatal warning to the warnings associated with the
+// given package.  It accepts the same error types as AddError.
+func (p *Package) AddWarning(err error) {
+	switch typedErr := err.(type) {
+	case *os.PathError:
+		p.Warnings = append(p.Warnings, packages.Error{
+			Pos:  typedErr.Path + ":1",
+			Msg:  typedErr.Err.Error(),
+			Kind: packages.ParseError,
+		})
+	case scanner.ErrorList:
+		for _, subErr := range typedErr {
+			p.Warnings = append(p.Warnings, packages.Error{
+				Pos:  subErr.Pos.String(),
+				Msg:  subErr.Msg,
+				Kind: packages.ParseError,
+			})
+		}
+	case types.Error:
+		p.Warnings = append(p.Warnings, packages.Error{
+			Pos:  typedErr.Fset.Position(typedErr.Pos).String(),
+			Msg:  typedErr.Msg,
+			Kind: packages.TypeError,
+		})
+	case ErrList:
+		for _, subErr := range typedErr {
+			p.AddWarning(subErr)
+		}
+	case PositionedError:
+		p.Warnings = append(p.Warnings, packages.Error{
+			Pos:  p.loader.cfg.Fset.Position(typedErr.Pos).String(),
+			Msg:  typedErr.Error(),
+			Kind: packages.UnknownError,
+		})
+	default:
+		p.Warnings = append(p.Warnings, packages.Error{
+			Pos:  p.ID + ":-",
+			Msg:  err.Error(),
+			Kind: packages.UnknownError,
+		})
+	}
+}
+
 // loader loads packages and their imports.  Loaded packages will have
 // type size, imports, and exports file information populated.  Additional
 // information, like ASTs and type-checking information, can be accessed