Skip to content

Commit 835bb2e

Browse files
committed
Change v8n context.Type and Parent on typedefs
I did an audit of how we are handling pointerness and aliases in validators. In a word - inconsistent. I went through each validator and added test cases where they were missing (previous commits). This commit changes how Context.Type is defined for typedefs. Previously Context.Type was the potentially aliased and potentially pointerful type for struct fields, but the "real" concrete type for typedefs. Now it is consistent, and validators need to unalias and unpointer types (which they had to do anyway because it was inconsistent.
1 parent 6ea1db5 commit 835bb2e

File tree

5 files changed

+32
-38
lines changed

5 files changed

+32
-38
lines changed

staging/src/k8s.io/code-generator/cmd/validation-gen/validation.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -461,13 +461,10 @@ func (td *typeDiscoverer) discover(t *types.Type, fldPath *field.Path) (*typeNod
461461
// This should only ever hit Struct and Alias types, since that is the only
462462
// opportunity to have type-attached comments to process.
463463
context := validators.Context{
464-
Scope: validators.ScopeType,
465-
Type: t,
466-
Path: fldPath,
467-
}
468-
if t.Kind == types.Alias {
469-
context.Parent = t
470-
context.Type = t.Underlying
464+
Scope: validators.ScopeType,
465+
Type: t,
466+
Parent: nil,
467+
Path: fldPath,
471468
}
472469
if validations, err := td.validator.ExtractValidations(context, t.CommentLines); err != nil {
473470
return nil, fmt.Errorf("%v: %w", fldPath, err)

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/each.go

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,9 @@ func (listTypeTagValidator) ValidScopes() sets.Set[Scope] {
7676
}
7777

7878
func (lttv listTypeTagValidator) GetValidations(context Context, _ []string, payload string) (Validations, error) {
79-
t := context.Type
80-
if t.Kind == types.Alias {
81-
t = t.Underlying
82-
}
79+
// We don't support pointers to lists, but other validators use realType()
80+
// for this sort of check, so let's be consistent.
81+
t := realType(context.Type)
8382
if t.Kind != types.Slice && t.Kind != types.Array {
8483
return Validations{}, fmt.Errorf("can only be used on list types")
8584
}
@@ -88,6 +87,7 @@ func (lttv listTypeTagValidator) GetValidations(context Context, _ []string, pay
8887
case "atomic", "set":
8988
// Allowed but no special handling.
9089
case "map":
90+
// NOTE: maps of pointers are not supported, and that is enforced way before this point.
9191
if realType(t.Elem).Kind != types.Struct {
9292
return Validations{}, fmt.Errorf("only lists of structs can be list-maps")
9393
}
@@ -135,10 +135,9 @@ func (listMapKeyTagValidator) ValidScopes() sets.Set[Scope] {
135135
}
136136

137137
func (lmktv listMapKeyTagValidator) GetValidations(context Context, _ []string, payload string) (Validations, error) {
138-
t := context.Type
139-
if t.Kind == types.Alias {
140-
t = t.Underlying
141-
}
138+
// We don't support pointers to lists, but other validators use realType()
139+
// for this sort of check, so let's be consistent.
140+
t := realType(context.Type)
142141
if t.Kind != types.Slice && t.Kind != types.Array {
143142
return Validations{}, fmt.Errorf("can only be used on list types")
144143
}
@@ -218,10 +217,9 @@ var (
218217
)
219218

220219
func (evtv eachValTagValidator) GetValidations(context Context, _ []string, payload string) (Validations, error) {
221-
t := context.Type
222-
if t.Kind == types.Alias {
223-
t = t.Underlying
224-
}
220+
// We don't support pointers to lists, but other validators use realType()
221+
// for this sort of check, so let's be consistent.
222+
t := realType(context.Type)
225223
switch t.Kind {
226224
case types.Slice, types.Array, types.Map:
227225
default:
@@ -353,10 +351,9 @@ var (
353351
)
354352

355353
func (ektv eachKeyTagValidator) GetValidations(context Context, _ []string, payload string) (Validations, error) {
356-
t := context.Type
357-
if t.Kind == types.Alias {
358-
t = t.Underlying
359-
}
354+
// We don't support pointers to lists, but other validators use realType()
355+
// for this sort of check, so let's be consistent.
356+
t := realType(context.Type)
360357
if t.Kind != types.Map {
361358
return Validations{}, fmt.Errorf("can only be used on map types")
362359
}

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/limits.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ var (
5555
func (minimumTagValidator) GetValidations(context Context, _ []string, payload string) (Validations, error) {
5656
var result Validations
5757

58+
// This tag can apply to value and pointer fields, as well as typedefs
59+
// (which should never be pointers). We need to check the concrete type.
5860
if t := realType(context.Type); !types.IsInteger(t) {
5961
return result, fmt.Errorf("can only be used on integer types (%s)", rootTypeString(context.Type, t))
6062
}

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/subfield.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ var (
5454
)
5555

5656
func (stv subfieldTagValidator) GetValidations(context Context, args []string, payload string) (Validations, error) {
57+
// This tag can apply to value and pointer fields, as well as typedefs
58+
// (which should never be pointers). We need to check the concrete type.
5759
t := realType(context.Type)
5860
if t.Kind != types.Struct {
5961
return Validations{}, fmt.Errorf("can only be used on struct types")

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/validators.go

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -133,25 +133,21 @@ type Context struct {
133133
Scope Scope
134134

135135
// Type provides details about the type being validated. When Scope is
136-
// ScopeType, this is the underlying type. When Scope is ScopeField, this
137-
// is the field's type (including any pointerness). When Scope indicates a
138-
// list-value, map-key, or map-value, this is the type of that key or
139-
// value.
136+
// ScopeType, this is the newly defined type. When Scope is ScopeField,
137+
// this is the field's type (which may be a pointer, an alias, or both).
138+
// When Scope indicates a list-value, map-key, or map-value, this is the
139+
// type of that key or value (which, again, may be a pointer, and alias, or
140+
// both).
140141
Type *types.Type
141142

142-
// Parent provides details about the logical parent type of the type being
143-
// validated, when applicable. When Scope is ScopeType, this is the
144-
// newly-defined type (when it exists - gengo handles struct-type
145-
// definitions differently that other "alias" type definitions). When
146-
// Scope is ScopeField, this is the field's parent struct's type. When
147-
// Scope indicates a list-value, map-key, or map-value, this is the type of
148-
// the whole list or map.
149-
//
150-
// Because of how gengo handles struct-type definitions, this field may be
151-
// nil in those cases.
143+
// Parent provides details about the logical parent type of the object
144+
// being validated, when applicable. When Scope is ScopeField, this is the
145+
// containing struct's type. When Scope indicates a list-value, map-key,
146+
// or map-value, this is the type of the whole list or map. When Scope is
147+
// ScopeType, this is nil.
152148
Parent *types.Type
153149

154-
// Member provides details about a field within a struct, when Scope is
150+
// Member provides details about a field within a struct when Scope is
155151
// ScopeField. For all other values of Scope, this will be nil.
156152
Member *types.Member
157153

0 commit comments

Comments
 (0)