Merge pull request #121 from yongruilin/fix-enum-on-type

fix(optionalfields): lookup types markers of fields when inspect fields

Author: k8s-ci-robot

pkg/analysis/optionalfields/testdata/src/a/a.go

@@ -98,6 +98,20 @@ type A struct {
 	// PointerMapAlias is a pointer map alias field.
 	// +optional
 	PointerMapAlias *MapAlias `json:"pointerMapAlias,omitempty"` // want "field PointerMapAlias is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// With the "Always" pointer preference, optional fields should be pointers regardless of zero value validity.
+	// +optional
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum is optional and should be a pointer"
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This is correctly a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"`
+
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
 }
 
 type B struct {
@@ -129,3 +143,8 @@ type BoolAlias bool
 type SliceAlias []string
 
 type MapAlias map[string]string
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string

pkg/analysis/optionalfields/testdata/src/a/a.go.golden

@@ -98,6 +98,20 @@ type A struct {
 	// PointerMapAlias is a pointer map alias field.
 	// +optional
 	PointerMapAlias MapAlias `json:"pointerMapAlias,omitempty"` // want "field PointerMapAlias is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// With the "Always" pointer preference, optional fields should be pointers regardless of zero value validity.
+	// +optional
+	StringAliasWithEnum *StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"` // want "field StringAliasWithEnum is optional and should be a pointer"
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This is correctly a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"`
+	
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty *StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
 }
 
 type B struct {
@@ -129,3 +143,8 @@ type BoolAlias bool
 type SliceAlias []string
 
 type MapAlias map[string]string
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string

pkg/analysis/optionalfields/testdata/src/b/a.go

@@ -270,6 +270,24 @@ type A struct {
 	// MapAlias is a map alias field.
 	// +optional
 	MapAlias MapAlias `json:"mapAlias,omitempty"`
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// The zero value ("") is not in the enum, so this should NOT be a pointer.
+	// +optional
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This should NOT be a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
+
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
+
+	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
+	// +optional
+	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
 }
 
 type B struct {
@@ -308,3 +326,12 @@ type BoolAlias bool
 type SliceAlias []string
 
 type MapAlias map[string]string
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string
+
+// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
+// +kubebuilder:validation:Enum=value1;value2;""
+type StringAliasWithEnumEmptyValue string

pkg/analysis/optionalfields/testdata/src/b/a.go.golden

@@ -270,6 +270,24 @@ type A struct {
 	// MapAlias is a map alias field.
 	// +optional
 	MapAlias MapAlias `json:"mapAlias,omitempty"`
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// The zero value ("") is not in the enum, so this should NOT be a pointer.
+	// +optional
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This should NOT be a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
+
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and should have the omitempty tag"
+
+	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
+	// +optional
+	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
 }
 
 type B struct {
@@ -308,3 +326,12 @@ type BoolAlias bool
 type SliceAlias []string
 
 type MapAlias map[string]string
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string
+
+// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
+// +kubebuilder:validation:Enum=value1;value2;""
+type StringAliasWithEnumEmptyValue string

pkg/analysis/optionalfields/testdata/src/c/a.go

@@ -421,6 +421,24 @@ type A struct {
 	// PointerPointerString is a double pointer string field.
 	// +optional
 	DoublePointerString **string `json:"doublePointerString,omitempty"` // want "field DoublePointerString is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// The zero value ("") is not in the enum, so this should NOT be a pointer.
+	// +optional
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This should NOT be a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer *StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
+
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
+
+	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
+	// +optional
+	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
 }
 
 type B struct {
@@ -526,3 +544,12 @@ type G struct {
 	// +kubebuilder:validation:Enum=foo;bar
 	EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty"` // want "field EnumWithoutOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
 }
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string
+
+// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
+// +kubebuilder:validation:Enum=value1;value2;""
+type StringAliasWithEnumEmptyValue string

pkg/analysis/optionalfields/testdata/src/c/a.go.golden

@@ -421,6 +421,24 @@ type A struct {
 	// PointerPointerString is a double pointer string field.
 	// +optional
 	DoublePointerString *string `json:"doublePointerString,omitempty"` // want "field DoublePointerString is optional but the underlying type does not need to be a pointer. The pointer should be removed."
+
+	// StringAliasWithEnum is a string alias field with enum validation.
+	// The zero value ("") is not in the enum, so this should NOT be a pointer.
+	// +optional
+	StringAliasWithEnum StringAliasWithEnum `json:"stringAliasWithEnum,omitempty"`
+
+	// StringAliasWithEnumPointer is a pointer string alias field with enum validation.
+	// This should NOT be a pointer since the zero value is not valid.
+	// +optional
+	StringAliasWithEnumPointer StringAliasWithEnum `json:"stringAliasWithEnumPointer,omitempty"` // want "field StringAliasWithEnumPointer is optional and does not allow the zero value. The field does not need to be a pointer."
+
+	// StringAliasWithEnumNoOmitEmpty is a string alias field with enum validation and no omitempty.
+	// +optional
+	StringAliasWithEnumNoOmitEmpty StringAliasWithEnum `json:"stringAliasWithEnumNoOmitEmpty,omitempty"` // want "field StringAliasWithEnumNoOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
+
+	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
+	// +optional
+	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
 }
 
 type B struct {
@@ -526,3 +544,12 @@ type G struct {
 	// +kubebuilder:validation:Enum=foo;bar
 	EnumWithoutOmitEmpty string `json:"enumWithoutOmitEmpty,omitempty"` // want "field EnumWithoutOmitEmpty is optional and does not allow the zero value. It must have the omitempty tag."
 }
+
+// StringAliasWithEnum is a string alias with enum validation.
+// The zero value ("") is not in the enum, making it invalid.
+// +kubebuilder:validation:Enum=value1;value2
+type StringAliasWithEnum string
+
+// StringAliasWithEnumEmptyValue is a string alias with enum validation and empty value.
+// +kubebuilder:validation:Enum=value1;value2;""
+type StringAliasWithEnumEmptyValue string

pkg/analysis/optionalfields/util.go

@@ -165,10 +165,10 @@ func isZeroValueValid(pass *analysis.Pass, field *ast.Field, typeExpr ast.Expr,
 	case *ast.Ident:
 		return isIdentZeroValueValid(pass, field, t, markersAccess)
 	case *ast.MapType:
-		return isMapZeroValueValid(field, markersAccess)
+		return isMapZeroValueValid(pass, field, markersAccess)
 	case *ast.ArrayType:
 		// For arrays, we can use a zero value if the array is not required to have a minimum number of items.
-		return isArrayZeroValueValid(field, t, markersAccess)
+		return isArrayZeroValueValid(pass, field, t, markersAccess)
 	case *ast.StarExpr:
 		return isZeroValueValid(pass, field, t.X, markersAccess)
 	}
@@ -249,7 +249,7 @@ func isIdentZeroValueValid(pass *analysis.Pass, field *ast.Field, ident *ast.Ide
 	// Check if the identifier is a known type that can have a zero value.
 	switch {
 	case isStringIdent(ident):
-		return isStringZeroValueValid(field, markersAccess)
+		return isStringZeroValueValid(pass, field, markersAccess)
 	case isIntegerIdent(ident):
 		return isNumericZeroValueValid[int](pass, field, markersAccess)
 	case isFloatIdent(ident):
@@ -270,8 +270,8 @@ func isIdentZeroValueValid(pass *analysis.Pass, field *ast.Field, ident *ast.Ide
 
 // isStringZeroValueValid checks if a string field can have a zero value.
 // This would be true when either there is no minimum length marker, or when the minimmum length marker is set to 0.
-func isStringZeroValueValid(field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
-	fieldMarkers := markersAccess.FieldMarkers(field)
+func isStringZeroValueValid(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
+	fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
 
 	if stringFieldIsEnum(fieldMarkers) {
 		return enumFieldAllowsEmpty(fieldMarkers), true
@@ -285,8 +285,8 @@ func isStringZeroValueValid(field *ast.Field, markersAccess markershelper.Marker
 
 // isMapZeroValueValid checks if a map field can have a zero value.
 // For maps, this means there is no minProperties marker, or the minProperties marker is set to 0.
-func isMapZeroValueValid(field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
-	fieldMarkers := markersAccess.FieldMarkers(field)
+func isMapZeroValueValid(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
+	fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
 
 	hasMinPropertiesMarker := fieldMarkers.Has(markers.KubebuilderMinPropertiesMarker)
 	minPropertiesMarkerIsZero := fieldMarkers.HasWithValue(fmt.Sprintf("%s=0", markers.KubebuilderMinPropertiesMarker))
@@ -295,13 +295,13 @@ func isMapZeroValueValid(field *ast.Field, markersAccess markershelper.Markers)
 }
 
 // isArrayZeroValueValid checks if an array field can have a zero value.
-func isArrayZeroValueValid(field *ast.Field, arrayType *ast.ArrayType, markersAccess markershelper.Markers) (bool, bool) {
+func isArrayZeroValueValid(pass *analysis.Pass, field *ast.Field, arrayType *ast.ArrayType, markersAccess markershelper.Markers) (bool, bool) {
 	// Arrays of bytes are special cased and treated as strings.
 	if ident, ok := arrayType.Elt.(*ast.Ident); ok && ident.Name == "byte" {
-		return isStringZeroValueValid(field, markersAccess)
+		return isStringZeroValueValid(pass, field, markersAccess)
 	}
 
-	fieldMarkers := markersAccess.FieldMarkers(field)
+	fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
 
 	// For arrays, we can use a zero value if the array is not required to have a minimum number of items.
 	minItems, err := getMarkerNumericValueByName[int](fieldMarkers, markers.KubebuilderMinItemsMarker)
@@ -341,7 +341,7 @@ type number interface {
 //
 //nolint:cyclop
 func isNumericZeroValueValid[N number](pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) (bool, bool) {
-	fieldMarkers := markersAccess.FieldMarkers(field)
+	fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
 
 	minimum, err := getMarkerNumericValueByName[N](fieldMarkers, markers.KubebuilderMinimumMarker)
 	if err != nil && !errors.Is(err, errMarkerMissingValue) {

pkg/analysis/utils/utils.go

@@ -122,11 +122,23 @@ func isInPassPackage(pass *analysis.Pass, namedType *types.Named) bool {
 // TypeAwareMarkerCollectionForField collects the markers for a given field into a single markers.MarkerSet.
 // If the field has a type that is not a basic type (i.e a custom type) then it will also gather any markers from
 // the type and include them in the markers.MarkerSet that is returned.
+// It will look through *ast.StarExpr to the underlying type.
 // Markers on the type will always come before markers on the field in the list of markers for an identifier.
 func TypeAwareMarkerCollectionForField(pass *analysis.Pass, markersAccess markers.Markers, field *ast.Field) markers.MarkerSet {
 	markers := markersAccess.FieldMarkers(field)
 
-	ident, ok := field.Type.(*ast.Ident)
+	var underlyingType ast.Expr
+
+	switch t := field.Type.(type) {
+	case *ast.Ident:
+		underlyingType = t
+	case *ast.StarExpr:
+		underlyingType = t.X
+	default:
+		return markers
+	}
+
+	ident, ok := underlyingType.(*ast.Ident)
 	if !ok {
 		return markers
 	}