From fd3fcfe121ab5c70495e3cfc3aac02b5cd67bf34 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 11 Jul 2025 13:47:28 -0700 Subject: [PATCH] Add comments to generated code This makes the output longer, but I think it might have helped catch recent bugs that SHOULD have been caught by inspection. --- .../cmd/validation-gen/validation.go | 26 ++++++++++++++----- .../cmd/validation-gen/validators/each.go | 15 ++++++++--- .../validation-gen/validators/validators.go | 9 +++++-- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go index 80d0596aa0f..3c5529ccf39 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go @@ -451,7 +451,8 @@ func (td *typeDiscoverer) discoverType(t *types.Type, fldPath *field.Path) (*typ // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachVal(fldPath, thisNode.valueType, - validators.Function("iterateListValues", validators.DefaultFlags, funcName)) + validators.Function("iterateListValues", validators.DefaultFlags, funcName). + WithComment("iterate the list and call the type's validation function")) if err != nil { return nil, fmt.Errorf("generating list iteration: %w", err) } else { @@ -482,7 +483,8 @@ func (td *typeDiscoverer) discoverType(t *types.Type, fldPath *field.Path) (*typ // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachKey(fldPath, underlying.childType, - validators.Function("iterateMapKeys", validators.DefaultFlags, funcName)) + validators.Function("iterateMapKeys", validators.DefaultFlags, funcName). + WithComment("iterate the map and call the key type's validation function")) if err != nil { return nil, fmt.Errorf("generating map key iteration: %w", err) } else { @@ -512,7 +514,8 @@ func (td *typeDiscoverer) discoverType(t *types.Type, fldPath *field.Path) (*typ // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachVal(fldPath, underlying.childType, - validators.Function("iterateMapValues", validators.DefaultFlags, funcName)) + validators.Function("iterateMapValues", validators.DefaultFlags, funcName). + WithComment("iterate the map and call the value type's validation function")) if err != nil { return nil, fmt.Errorf("generating map value iteration: %w", err) } else { @@ -695,7 +698,8 @@ func (td *typeDiscoverer) discoverStruct(thisNode *typeNode, fldPath *field.Path // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachVal(childPath, childType, - validators.Function("iterateListValues", validators.DefaultFlags, funcName)) + validators.Function("iterateListValues", validators.DefaultFlags, funcName). + WithComment("iterate the list and call the type's validation function")) if err != nil { return fmt.Errorf("generating list iteration: %w", err) } else { @@ -726,7 +730,8 @@ func (td *typeDiscoverer) discoverStruct(thisNode *typeNode, fldPath *field.Path // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachKey(childPath, childType, - validators.Function("iterateMapKeys", validators.DefaultFlags, funcName)) + validators.Function("iterateMapKeys", validators.DefaultFlags, funcName). + WithComment("iterate the map and call the key type's validation function")) if err != nil { return fmt.Errorf("generating map key iteration: %w", err) } else { @@ -756,7 +761,8 @@ func (td *typeDiscoverer) discoverStruct(thisNode *typeNode, fldPath *field.Path // Note: the first argument to Function() is really // only for debugging. v, err := validators.ForEachVal(childPath, childType, - validators.Function("iterateMapValues", validators.DefaultFlags, funcName)) + validators.Function("iterateMapValues", validators.DefaultFlags, funcName). + WithComment("iterate the map and call the value type's validation function")) if err != nil { return fmt.Errorf("generating map value iteration: %w", err) } else { @@ -879,6 +885,7 @@ func (g *genValidations) emitRegisterFunction(c *generator.Context, schemeRegist // This uses a typed nil pointer, rather than a real instance because // we need the type information, but not an instance of the type. + sw.Do("// type $.rootType|name$\n", targs) sw.Do("scheme.AddValidationFunc(", targs) sw.Do(" ($.typePfx$$.rootType|raw$)(nil), ", targs) sw.Do(" func(ctx $.context.Context$, op $.operation.Operation|raw$, obj, oldObj interface{}) $.field.ErrorList|raw$ {\n", targs) @@ -948,6 +955,8 @@ func (g *genValidations) emitValidationFunction(c *generator.Context, t *types.T if node == nil { panic(fmt.Sprintf("found nil node for root-type %v", t)) } + sw.Do("// $.inType|objectvalidationfn$ validates an instance of $.inType|name$ according\n", targs) + sw.Do("// to declarative validation rules in the API schema.\n", targs) sw.Do("func $.inType|objectvalidationfn$(", targs) sw.Do(" ctx $.context.Context|raw$, ", targs) sw.Do(" op $.operation.Operation|raw$, ", targs) @@ -1050,6 +1059,7 @@ func (g *genValidations) emitValidationForChild(c *generator.Context, thisChild if !validations.Empty() { emitComments(validations.Comments, bufsw) emitRatchetingCheck(c, fld.childType, bufsw) + bufsw.Do("// call field-attached validations\n", nil) emitCallsToValidators(c, validations.Functions, bufsw) fldRatchetingChecked = true } @@ -1168,6 +1178,7 @@ func (g *genValidations) emitCallToOtherTypeFunc(c *generator.Context, node *typ targs := generator.Args{ "funcName": c.Universe.Type(node.funcName), } + sw.Do("// call the type's validation function\n", nil) sw.Do("errs = append(errs, $.funcName|raw$(ctx, op, fldPath, obj, oldObj)...)\n", targs) } @@ -1177,6 +1188,7 @@ func emitRatchetingCheck(c *generator.Context, t *types.Type, sw *generator.Snip targs := generator.Args{ "operation": mkSymbolArgs(c, operationPkgSymbols), } + sw.Do("// don't revalidate unchanged data\n", nil) // If the type is a builtin, we can use a simpler equality check when they are not nil. if util.IsDirectComparable(util.NonPointer(util.NativeType(t))) { // We should never get anything but pointers here, since every other @@ -1192,7 +1204,7 @@ func emitRatchetingCheck(c *generator.Context, t *types.Type, sw *generator.Snip targs["equality"] = mkSymbolArgs(c, equalityPkgSymbols) sw.Do("if op.Type == $.operation.Update|raw$ && $.equality.Semantic|raw$.DeepEqual(obj, oldObj) {\n", targs) } - sw.Do(" return nil // no changes\n", nil) + sw.Do(" return nil\n", nil) sw.Do("}\n", nil) } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go index 9e0a84f8205..d0f3bbc0a02 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go @@ -314,7 +314,8 @@ func (lv listValidator) GetValidations(context Context) (Validations, error) { if util.IsDirectComparable(util.NonPointer(util.NativeType(nt.Elem))) { matchArg = validateDirectEqual } - f := Function("listValidator", DefaultFlags, validateUnique, Identifier(matchArg)) + f := Function("listValidator", DefaultFlags, validateUnique, Identifier(matchArg)). + WithComment("listType=set requires unique values") result.AddFunction(f) } // TODO: enable the following once we have a way to either opt-out from this validation @@ -503,7 +504,9 @@ func (evtv eachValTagValidator) getListValidations(fldPath *field.Path, t *types // The matchArg and equivArg are both nil. } for _, vfn := range validations.Functions { - f := Function(eachValTagName, vfn.Flags, validateEachSliceVal, matchArg, equivArg, WrapperFunction{vfn, nt.Elem}) + comm := vfn.Comments + vfn.Comments = nil + f := Function(eachValTagName, vfn.Flags, validateEachSliceVal, matchArg, equivArg, WrapperFunction{vfn, nt.Elem}).WithComments(comm...) result.AddFunction(f) } @@ -522,7 +525,9 @@ func (evtv eachValTagValidator) getMapValidations(t *types.Type, validations Val equivArg = Identifier(validateDirectEqual) } for _, vfn := range validations.Functions { - f := Function(eachValTagName, vfn.Flags, validateEachMapVal, equivArg, WrapperFunction{vfn, nt.Elem}) + comm := vfn.Comments + vfn.Comments = nil + f := Function(eachValTagName, vfn.Flags, validateEachMapVal, equivArg, WrapperFunction{vfn, nt.Elem}).WithComments(comm...) result.AddFunction(f) } @@ -593,7 +598,9 @@ func (ektv eachKeyTagValidator) getValidations(t *types.Type, validations Valida result := Validations{} result.OpaqueKeyType = validations.OpaqueType for _, vfn := range validations.Functions { - f := Function(eachKeyTagName, vfn.Flags, validateEachMapKey, WrapperFunction{vfn, t.Key}) + comm := vfn.Comments + vfn.Comments = nil + f := Function(eachKeyTagName, vfn.Flags, validateEachMapKey, WrapperFunction{vfn, t.Key}).WithComments(comm...) result.AddFunction(f) } return result, nil diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go index e2f60a80e5f..40e3adc3cb4 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go +++ b/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go @@ -489,10 +489,15 @@ func (fg FunctionGen) WithConditions(conditions Conditions) FunctionGen { return fg } +// WithComments returns a new FunctionGen with a comment. +func (fg FunctionGen) WithComments(comments ...string) FunctionGen { + fg.Comments = append(fg.Comments, comments...) + return fg +} + // WithComment returns a new FunctionGen with a comment. func (fg FunctionGen) WithComment(comment string) FunctionGen { - fg.Comments = append(fg.Comments, comment) - return fg + return fg.WithComments(comment) } // Variable creates a VariableGen for a given variable name and init value.