Allow pointer-to-slice/map with explicit MinItems=0/MinProperties=0

This change fixes the linter incorrectly suggesting removal of pointers
for slice and map fields that have explicit MinItems=0 or MinProperties=0
markers.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

Author: saschagrunert

pkg/analysis/utils/serialization/serialization_check.go

@@ -98,31 +98,31 @@ func (s *serializationCheck) Check(pass *analysis.Pass, field *ast.Field, marker
 	switch s.pointerPreference {
 	case PointersPreferenceAlways:
 		// The field must always be a pointer, pointers require omitempty, so enforce that too.
-		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, "should be a pointer.")
+		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, "should be a pointer.")
 		s.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
 	case PointersPreferenceWhenRequired:
-		s.handleFieldOmitZero(pass, field, fieldName, jsonTags, hasOmitZero, hasValidZeroValue, isPointer, isStruct)
+		s.handleFieldOmitZero(pass, field, fieldName, jsonTags, underlying, hasOmitZero, hasValidZeroValue, isPointer, isStruct, markersAccess)
 
 		if s.omitEmptyPolicy != OmitEmptyPolicyIgnore || hasOmitEmpty {
 			// If we require omitempty, or the field has omitempty, we can check the field properties based on it being an omitempty field.
-			s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct)
+			s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct, markersAccess)
 		} else {
 			// The field does not have omitempty, and does not require it.
-			s.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct)
+			s.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct, markersAccess)
 		}
 	default:
 		panic(fmt.Sprintf("unknown pointer preference: %s", s.pointerPreference))
 	}
 }
 
-func (s *serializationCheck) handleFieldOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, hasValidZeroValue, isPointer, isStruct bool) {
+func (s *serializationCheck) handleFieldOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitZero, hasValidZeroValue, isPointer, isStruct bool, markersAccess markershelper.Markers) {
 	switch s.omitZeroPolicy {
 	case OmitZeroPolicyForbid:
 		// when the omitzero policy is set to forbid, we need to report removing omitzero if set on the struct fields.
 		s.checkFieldPropertiesWithOmitZeroForbidPolicy(pass, field, fieldName, isStruct, hasOmitZero, jsonTags)
 	case OmitZeroPolicyWarn, OmitZeroPolicySuggestFix:
 		// If we require omitzero, or the field has omitzero, we can check the field properties based on it being an omitzero field.
-		s.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, isPointer, isStruct, hasValidZeroValue)
+		s.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, underlying, hasOmitZero, isPointer, isStruct, hasValidZeroValue, markersAccess)
 	default:
 		panic(fmt.Sprintf("unknown omit zero policy: %s", s.omitZeroPolicy))
 	}
@@ -136,7 +136,7 @@ func (s *serializationCheck) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass,
 	reportShouldAddOmitEmpty(pass, field, s.omitEmptyPolicy, fieldName, "field %s should have the omitempty tag.", jsonTags)
 }
 
-func (s *serializationCheck) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
+func (s *serializationCheck) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct bool, markersAccess markershelper.Markers) {
 	switch {
 	case isStruct && !hasValidZeroValue && s.omitZeroPolicy != OmitZeroPolicyForbid:
 		// The struct field need not be pointer if it does not have a valid zero value.
@@ -145,27 +145,27 @@ func (s *serializationCheck) checkFieldPropertiesWithOmitEmptyRequired(pass *ana
 		zeroValue := utils.GetTypedZeroValue(pass, underlying)
 		validationHint := utils.GetTypedValidationHint(pass, underlying)
 
-		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, fmt.Sprintf("has a valid zero value (%s), but the validation is not complete (e.g. %s). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.", zeroValue, validationHint))
+		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, fmt.Sprintf("has a valid zero value (%s), but the validation is not complete (e.g. %s). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.", zeroValue, validationHint))
 	case hasValidZeroValue, isStruct:
 		// The field validation infers that the zero value is valid, the field needs to be a pointer.
 		// Structs with omitempty should always be pointers, else they won't actually be omitted.
 		zeroValue := utils.GetTypedZeroValue(pass, underlying)
 
-		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, fmt.Sprintf("has a valid zero value (%s) and should be a pointer.", zeroValue))
+		s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, fmt.Sprintf("has a valid zero value (%s) and should be a pointer.", zeroValue))
 	case !hasValidZeroValue && completeValidation && !isStruct:
 		// The validation is fully complete, and the zero value is not valid, so we don't need a pointer.
-		s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not allow the zero value. The field does not need to be a pointer.")
+		s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, "field %s does not allow the zero value. The field does not need to be a pointer.")
 	}
 
 	// In this case, we should always add the omitempty if it isn't present.
 	s.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
 }
 
-func (s *serializationCheck) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
+func (s *serializationCheck) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool, markersAccess markershelper.Markers) {
 	switch {
 	case hasValidZeroValue:
 		// The field is not omitempty, and the zero value is valid, the field does not need to be a pointer.
-		s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not have omitempty and allows the zero value. The field does not need to be a pointer.")
+		s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, "field %s does not have omitempty and allows the zero value. The field does not need to be a pointer.")
 	case !hasValidZeroValue:
 		// The zero value would not be accepted, so the field needs to have omitempty.
 		// Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore.
@@ -174,17 +174,17 @@ func (s *serializationCheck) checkFieldPropertiesWithoutOmitEmpty(pass *analysis
 		// Once it has the omitempty tag, it will also need to be a pointer in some cases.
 		// Now handle it as if it had the omitempty already.
 		// We already handle the omitempty tag above, so force the `hasOmitEmpty` to true.
-		s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, hasValidZeroValue, completeValidation, isPointer, isStruct)
+		s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, hasValidZeroValue, completeValidation, isPointer, isStruct, markersAccess)
 	}
 }
 
-func (s *serializationCheck) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool) {
+func (s *serializationCheck) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool, markersAccess markershelper.Markers) {
 	if !isStruct || hasValidZeroValue {
 		return
 	}
 
 	s.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, jsonTags)
-	s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not allow the zero value. The field does not need to be a pointer.")
+	s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, underlying, markersAccess, "field %s does not allow the zero value. The field does not need to be a pointer.")
 }
 
 func (s *serializationCheck) checkFieldPropertiesWithOmitZeroForbidPolicy(pass *analysis.Pass, field *ast.Field, fieldName string, isStruct, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) {
@@ -205,9 +205,15 @@ func (s *serializationCheck) handleFieldShouldHaveOmitZero(pass *analysis.Pass,
 	reportShouldAddOmitZero(pass, field, s.omitZeroPolicy, fieldName, "field %s does not allow the zero value. It must have the omitzero tag.", jsonTags)
 }
 
-func (s *serializationCheck) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr, reason string) {
+func (s *serializationCheck) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr, markersAccess markershelper.Markers, reason string) {
 	if utils.IsPointerType(pass, underlying) {
 		if isPointer {
+			// Check if this is a pointer-to-slice/map with explicit MinItems=0 or MinProperties=0
+			// In this case, the pointer is intentional to distinguish nil from empty
+			if hasExplicitZeroMinValidation(pass, field, underlying, markersAccess) {
+				return
+			}
+
 			switch s.pointerPolicy {
 			case PointersPolicySuggestFix:
 				reportShouldRemovePointer(pass, field, PointersPolicySuggestFix, fieldName, "field %s underlying type does not need to be a pointer. The pointer should be removed.", fieldName)
@@ -231,10 +237,33 @@ func (s *serializationCheck) handleFieldShouldBePointer(pass *analysis.Pass, fie
 	}
 }
 
-func (s *serializationCheck) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, message string) {
+func (s *serializationCheck) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr, markersAccess markershelper.Markers, message string) {
 	if !isPointer {
 		return
 	}
 
+	// Check if this is a pointer-to-slice/map with explicit MinItems=0 or MinProperties=0
+	// In this case, the pointer is intentional to distinguish nil from empty
+	if hasExplicitZeroMinValidation(pass, field, underlying, markersAccess) {
+		return
+	}
+
 	reportShouldRemovePointer(pass, field, s.pointerPolicy, fieldName, message, fieldName)
 }
+
+// hasExplicitZeroMinValidation checks if a field has an explicit MinItems=0 or MinProperties=0 marker.
+// This indicates the developer intentionally wants to distinguish between nil and empty for slices/maps.
+func hasExplicitZeroMinValidation(pass *analysis.Pass, field *ast.Field, underlying ast.Expr, markersAccess markershelper.Markers) bool {
+	fieldMarkers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field)
+
+	switch underlying.(type) {
+	case *ast.ArrayType:
+		// Check for explicit MinItems=0
+		return fieldMarkers.HasWithValue("kubebuilder:validation:MinItems=0")
+	case *ast.MapType:
+		// Check for explicit MinProperties=0
+		return fieldMarkers.HasWithValue("kubebuilder:validation:MinProperties=0")
+	}
+
+	return false
+}

pkg/analysis/utils/serialization/testdata/src/pointers_always/pointer_to_slice_with_minzero.go

@@ -0,0 +1,34 @@
+package a
+
+type TestPointerToSliceWithMinZeroAlways struct {
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItems *[]string `json:"ptrArrayWithZeroMinItems,omitempty"`
+
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItemsNoOmitEmpty *[]string `json:"ptrArrayWithZeroMinItemsNoOmitEmpty"` // want "field PtrArrayWithZeroMinItemsNoOmitEmpty should have the omitempty tag."
+}
+
+type TestPointerToMapWithMinZeroAlways struct {
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinProperties *map[string]string `json:"mapPtrWithZeroMinProperties,omitempty"`
+
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinPropertiesNoOmitEmpty *map[string]string `json:"mapPtrWithZeroMinPropertiesNoOmitEmpty"` // want "field MapPtrWithZeroMinPropertiesNoOmitEmpty should have the omitempty tag."
+}
+
+// Test that pointers ARE still flagged when MinItems/MinProperties is NOT zero
+type TestPointerToSliceWithNonZeroMinAlways struct {
+	// +kubebuilder:validation:MinItems=1
+	PtrArrayWithNonZeroMinItems *[]string `json:"ptrArrayWithNonZeroMinItems,omitempty"` // want "field PtrArrayWithNonZeroMinItems underlying type does not need to be a pointer. The pointer should be removed."
+
+	// No MinItems validation
+	PtrArrayWithoutMinItems *[]string `json:"ptrArrayWithoutMinItems,omitempty"` // want "field PtrArrayWithoutMinItems underlying type does not need to be a pointer. The pointer should be removed."
+}
+
+type TestPointerToMapWithNonZeroMinAlways struct {
+	// +kubebuilder:validation:MinProperties=1
+	MapPtrWithNonZeroMinProperties *map[string]string `json:"mapPtrWithNonZeroMinProperties,omitempty"` // want "field MapPtrWithNonZeroMinProperties underlying type does not need to be a pointer. The pointer should be removed."
+
+	// No MinProperties validation
+	MapPtrWithoutMinProperties *map[string]string `json:"mapPtrWithoutMinProperties,omitempty"` // want "field MapPtrWithoutMinProperties underlying type does not need to be a pointer. The pointer should be removed."
+}

pkg/analysis/utils/serialization/testdata/src/pointers_always/pointer_to_slice_with_minzero.go.golden

@@ -0,0 +1,34 @@
+package a
+
+type TestPointerToSliceWithMinZeroAlways struct {
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItems *[]string `json:"ptrArrayWithZeroMinItems,omitempty"`
+
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItemsNoOmitEmpty *[]string `json:"ptrArrayWithZeroMinItemsNoOmitEmpty,omitempty"` // want "field PtrArrayWithZeroMinItemsNoOmitEmpty should have the omitempty tag."
+}
+
+type TestPointerToMapWithMinZeroAlways struct {
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinProperties *map[string]string `json:"mapPtrWithZeroMinProperties,omitempty"`
+
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinPropertiesNoOmitEmpty *map[string]string `json:"mapPtrWithZeroMinPropertiesNoOmitEmpty,omitempty"` // want "field MapPtrWithZeroMinPropertiesNoOmitEmpty should have the omitempty tag."
+}
+
+// Test that pointers ARE still flagged when MinItems/MinProperties is NOT zero
+type TestPointerToSliceWithNonZeroMinAlways struct {
+	// +kubebuilder:validation:MinItems=1
+	PtrArrayWithNonZeroMinItems []string `json:"ptrArrayWithNonZeroMinItems,omitempty"` // want "field PtrArrayWithNonZeroMinItems underlying type does not need to be a pointer. The pointer should be removed."
+
+	// No MinItems validation
+	PtrArrayWithoutMinItems []string `json:"ptrArrayWithoutMinItems,omitempty"` // want "field PtrArrayWithoutMinItems underlying type does not need to be a pointer. The pointer should be removed."
+}
+
+type TestPointerToMapWithNonZeroMinAlways struct {
+	// +kubebuilder:validation:MinProperties=1
+	MapPtrWithNonZeroMinProperties map[string]string `json:"mapPtrWithNonZeroMinProperties,omitempty"` // want "field MapPtrWithNonZeroMinProperties underlying type does not need to be a pointer. The pointer should be removed."
+
+	// No MinProperties validation
+	MapPtrWithoutMinProperties map[string]string `json:"mapPtrWithoutMinProperties,omitempty"` // want "field MapPtrWithoutMinProperties underlying type does not need to be a pointer. The pointer should be removed."
+}

pkg/analysis/utils/serialization/testdata/src/pointers_when_required/pointer_to_slice_with_minzero.go

@@ -0,0 +1,34 @@
+package a
+
+type TestPointerToSliceWithMinZero struct {
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItems *[]string `json:"ptrArrayWithZeroMinItems,omitempty"`
+
+	// +kubebuilder:validation:MinItems=0
+	PtrArrayWithZeroMinItemsNoOmitEmpty *[]string `json:"ptrArrayWithZeroMinItemsNoOmitEmpty"` // want "field PtrArrayWithZeroMinItemsNoOmitEmpty should have the omitempty tag."
+}
+
+type TestPointerToMapWithMinZero struct {
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinProperties *map[string]string `json:"mapPtrWithZeroMinProperties,omitempty"`
+
+	// +kubebuilder:validation:MinProperties=0
+	MapPtrWithZeroMinPropertiesNoOmitEmpty *map[string]string `json:"mapPtrWithZeroMinPropertiesNoOmitEmpty"` // want "field MapPtrWithZeroMinPropertiesNoOmitEmpty should have the omitempty tag."
+}
+
+// Test that pointers ARE still flagged when MinItems/MinProperties is NOT zero
+type TestPointerToSliceWithNonZeroMin struct {
+	// +kubebuilder:validation:MinItems=1
+	PtrArrayWithNonZeroMinItems *[]string `json:"ptrArrayWithNonZeroMinItems,omitempty"` // want "field PtrArrayWithNonZeroMinItems does not allow the zero value. The field does not need to be a pointer."
+
+	// No MinItems validation
+	PtrArrayWithoutMinItems *[]string `json:"ptrArrayWithoutMinItems,omitempty"` // want "field PtrArrayWithoutMinItems underlying type does not need to be a pointer. The pointer should be removed."
+}
+
+type TestPointerToMapWithNonZeroMin struct {
+	// +kubebuilder:validation:MinProperties=1
+	MapPtrWithNonZeroMinProperties *map[string]string `json:"mapPtrWithNonZeroMinProperties,omitempty"` // want "field MapPtrWithNonZeroMinProperties does not allow the zero value. The field does not need to be a pointer."
+
+	// No MinProperties validation
+	MapPtrWithoutMinProperties *map[string]string `json:"mapPtrWithoutMinProperties,omitempty"` // want "field MapPtrWithoutMinProperties underlying type does not need to be a pointer. The pointer should be removed."
+}