Merge pull request #115 from Karthik-K-N/optionalfields

Support omitzero tags in optionalfields linter

Author: k8s-ci-robot

docs/linters.md

@@ -180,10 +180,13 @@ The `nophase` linter checks that the fields in the API types don't contain a 'Ph
 
 ## OptionalFields
 
-The `optionalfields` linter checks that all fields marked as optional adhere to being pointers and having the `omitempty` value in their `json` tag where appropriate.
+The `optionalfields` linter checks that all fields marked as optional adhere to being pointers and having either the `omitempty` or `omitzero` value in their `json` tag where appropriate.
+Currently `omitzero` is handled only for fields with struct type.
 
 If you prefer to avoid pointers where possible, the linter can be configured with the `WhenRequired` preference to determine, based on the serialization and valid values for the field, whether the field should be a pointer or not.
-For example, an optional string with a non-zero minimum length does not need to be a pointer, as the zero value is not valid, and it is safe for the Go marshaller to omit the empty value.
+For example
+- an optional string with a non-zero minimum length does not need to be a pointer, as the zero value is not valid, and it is safe for the Go marshaller to omit the empty value.
+- an optional struct having omitzero json tag with a non-zero minimum properties does not need to be a pointer, as the zero value is not valid, and it is safe for the Go marshaller to omit the empty value.
 
 In certain use cases, it can be desirable to not omit optional fields from the serialized form of the object.
 In this case, the `omitempty` policy can be set to `Ignore`, and the linter will ensure that the zero value of the object is an acceptable value for the field.
@@ -198,19 +201,23 @@ lintersConfig:
       policy: SuggestFix | Warn # The policy for pointers in optional fields. Defaults to `SuggestFix`.
     omitempty:
         policy: SuggestFix | Warn | Ignore # The policy for omitempty in optional fields. Defaults to `SuggestFix`.
+    omitzero:
+        policy: SuggestFix | Warn | Forbid # The policy for omitzero in optional fields. Defaults to `SuggestFix`.
 ```
 
 ### Fixes
 
-The `optionalfields` linter can automatically fix fields that are marked as optional, that are either not pointers or do not have the `omitempty` value in their `json` tag.
-It will suggest to add the pointer to the field, and update the `json` tag to include the `omitempty` value.
+The `optionalfields` linter can automatically fix fields that are marked as optional, that are either not pointers or do not have the `omitempty` or `omitzero` value in their `json` tag.
+It will suggest to add the pointer to the field, and update the `json` tag to include the `omitempty` value or, for struct fields specifically, it will suggest to remove the pointer to the field, and update the `json` tag to include the `omitzero` value.
 
 If you prefer not to suggest fixes for pointers in optional fields, you can change the `pointers.policy` to `Warn`.
 
 If you prefer not to suggest fixes for `omitempty` in optional fields, you can change the `omitempty.policy` to `Warn` or `Ignore`.
+If you prefer not to suggest fixes for `omitzero` in optional fields, you can change the `omitzero.policy` to `Warn` and also not to consider `omitzero` policy at all, it can be set to `Forbid`.
 
 When the `pointers.preference` is set to `WhenRequired`, the linter will suggest to add the pointer to the field only when the field zero value is a valid value for the field.
 When the field zero value is not a valid value for the field, the linter will suggest to remove the pointer from the field.
+When the field zero value is not a valid value for the field of type struct, the linter will suggest to add `omitzero` json tag and to remove the pointer from the field.
 
 When the `pointers.preference` is set to `Always`, the linter will always suggest to add the pointer to the field, regardless of the validity of the zero value of the field.
 

pkg/analysis/helpers/extractjsontags/analyzer.go

@@ -25,10 +25,15 @@ import (
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
-
+	"k8s.io/utils/set"
 	kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
 )
 
+const (
+	omitEmpty = "omitempty"
+	omitZero  = "omitzero"
+)
+
 // StructFieldTags is used to find information about
 // json tags on fields within struct.
 type StructFieldTags interface {
@@ -137,10 +142,12 @@ func extractTagInfo(tag *ast.BasicLit) FieldTagInfo {
 	}
 
 	tagName := tagValues[0]
+	tagSet := set.New[string](tagValues...)
 
 	return FieldTagInfo{
 		Name:      tagName,
-		OmitEmpty: len(tagValues) == 2 && tagValues[1] == "omitempty",
+		OmitEmpty: tagSet.Has(omitEmpty),
+		OmitZero:  tagSet.Has(omitZero),
 		RawValue:  tagValue,
 		Pos:       pos,
 		End:       end,
@@ -159,6 +166,9 @@ type FieldTagInfo struct {
 	// OmitEmpty is true if the field has the omitempty option in the json tag.
 	OmitEmpty bool
 
+	// OmitZero is true if the field has the omitzero option in the json tag.
+	OmitZero bool
+
 	// Inline is true if the field has the inline option in the json tag.
 	Inline bool
 

pkg/analysis/optionalfields/analyzer.go

@@ -49,6 +49,7 @@ type analyzer struct {
 	pointerPolicy     OptionalFieldsPointerPolicy
 	pointerPreference OptionalFieldsPointerPreference
 	omitEmptyPolicy   OptionalFieldsOmitEmptyPolicy
+	omitZeroPolicy    OptionalFieldsOmitZeroPolicy
 }
 
 // newAnalyzer creates a new analyzer.
@@ -63,6 +64,7 @@ func newAnalyzer(cfg *OptionalFieldsConfig) *analysis.Analyzer {
 		pointerPolicy:     cfg.Pointers.Policy,
 		pointerPreference: cfg.Pointers.Preference,
 		omitEmptyPolicy:   cfg.OmitEmpty.Policy,
+		omitZeroPolicy:    cfg.OmitZero.Policy,
 	}
 
 	return &analysis.Analyzer{
@@ -125,11 +127,16 @@ func defaultConfig(cfg *OptionalFieldsConfig) {
 	if cfg.OmitEmpty.Policy == "" {
 		cfg.OmitEmpty.Policy = OptionalFieldsOmitEmptyPolicySuggestFix
 	}
+
+	if cfg.OmitZero.Policy == "" {
+		cfg.OmitZero.Policy = OptionalFieldsOmitZeroPolicySuggestFix
+	}
 }
 
 func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, fieldName string, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) {
-	hasValidZeroValue, completeValidation := utils.IsZeroValueValid(pass, field, field.Type, markersAccess)
+	hasValidZeroValue, completeValidation := utils.IsZeroValueValid(pass, field, field.Type, markersAccess, a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid)
 	hasOmitEmpty := jsonTags.OmitEmpty
+	hasOmitZero := jsonTags.OmitZero
 	isPointer, underlying := isStarExpr(field.Type)
 	isStruct := utils.IsStructType(pass, field.Type)
 
@@ -142,7 +149,17 @@ func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, f
 	}
 
 	// The pointer preference is now when required.
+	// Validate for omitzero policy.
+	if a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid {
+		// If we require omitzero, we can check the field properties based on it being an omitzero field.
+		a.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, isPointer, isStruct, hasValidZeroValue)
+	} else {
+		// when the omitzero policy is set to forbid, we need to report removing omitzero if set on the struct fields.
+		a.checkFieldPropertiesWithOmitZeroForbidPolicy(pass, field, fieldName, isStruct, hasOmitZero, jsonTags)
+	}
 
+	// The pointer preference is now when required.
+	// Validate for omitempty policy.
 	if a.omitEmptyPolicy != OptionalFieldsOmitEmptyPolicyIgnore || hasOmitEmpty {
 		// If we require omitempty, or the field has omitempty, we can check the field properties based on it being an omitempty field.
 		a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct)
@@ -157,6 +174,9 @@ func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass
 	a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
 
 	switch {
+	case isStruct && !hasValidZeroValue && a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid:
+		// The struct field need not be pointer if it does not have a valid zero value.
+		return
 	case hasValidZeroValue && !completeValidation:
 		a.handleIncompleteFieldValidation(pass, field, fieldName, isPointer, underlying)
 		fallthrough // Since it's a valid zero value, we should still enforce the pointer.
@@ -183,8 +203,26 @@ func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, fie
 		// 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.
-		a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, hasValidZeroValue, completeValidation, isPointer, isStruct)
+		a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, false, hasValidZeroValue, completeValidation, isPointer, isStruct)
+	}
+}
+
+func (a *analyzer) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool) {
+	if !isStruct || hasValidZeroValue {
+		return
 	}
+
+	a.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, jsonTags)
+	a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not have a valid zero value. The field does not need to be a pointer.")
+}
+
+func (a *analyzer) checkFieldPropertiesWithOmitZeroForbidPolicy(pass *analysis.Pass, field *ast.Field, fieldName string, isStruct, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) {
+	if !isStruct || !hasOmitZero {
+		// Handle omitzero only for struct field having omitZero tag.
+		return
+	}
+
+	reportShouldRemoveOmitZero(pass, field, fieldName, jsonTags)
 }
 
 func (a *analyzer) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
@@ -229,6 +267,14 @@ func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *as
 	reportShouldAddOmitEmpty(pass, field, a.omitEmptyPolicy, fieldName, "field %s is optional and should have the omitempty tag", jsonTags)
 }
 
+func (a *analyzer) handleFieldShouldHaveOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) {
+	if hasOmitZero {
+		return
+	}
+	// Currently, add omitzero tags to only struct fields.
+	reportShouldAddOmitZero(pass, field, a.omitZeroPolicy, fieldName, "field %s is optional and does not allow the zero value. It must have the omitzero tag.", jsonTags)
+}
+
 func (a *analyzer) handleIncompleteFieldValidation(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
 	if isPointer || isPointerType(pass, underlying) {
 		// Don't warn them if the field is already a pointer.

pkg/analysis/optionalfields/analyzer_test.go

@@ -40,6 +40,9 @@ func TestWhenRequiredPreferenceConfiguration(t *testing.T) {
 		Pointers: optionalfields.OptionalFieldsPointers{
 			Preference: optionalfields.OptionalFieldsPointerPreferenceWhenRequired,
 		},
+		OmitZero: optionalfields.OptionalFieldsOmitZero{
+			Policy: optionalfields.OptionalFieldsOmitZeroPolicyForbid,
+		},
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -58,10 +61,35 @@ func TestWhenRequiredWithOmitEmptyIgnorePreferenceConfiguration(t *testing.T) {
 		OmitEmpty: optionalfields.OptionalFieldsOmitEmpty{
 			Policy: optionalfields.OptionalFieldsOmitEmptyPolicyIgnore,
 		},
+		OmitZero: optionalfields.OptionalFieldsOmitZero{
+			Policy: optionalfields.OptionalFieldsOmitZeroPolicyForbid,
+		},
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	analysistest.RunWithSuggestedFixes(t, testdata, a, "c")
 }
+
+func TestWithSuggestFixForOmitZero(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	a, err := optionalfields.Initializer().Init(&optionalfields.OptionalFieldsConfig{
+		Pointers: optionalfields.OptionalFieldsPointers{
+			Preference: optionalfields.OptionalFieldsPointerPreferenceWhenRequired,
+			Policy:     optionalfields.OptionalFieldsPointerPolicySuggestFix,
+		},
+		OmitEmpty: optionalfields.OptionalFieldsOmitEmpty{
+			Policy: optionalfields.OptionalFieldsOmitEmptyPolicySuggestFix,
+		},
+		OmitZero: optionalfields.OptionalFieldsOmitZero{
+			Policy: optionalfields.OptionalFieldsOmitZeroPolicySuggestFix,
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	analysistest.RunWithSuggestedFixes(t, testdata, a, "d")
+}

pkg/analysis/optionalfields/config.go

@@ -26,6 +26,11 @@ type OptionalFieldsConfig struct {
 	// This defines how the linter should handle optional fields, and whether they should have the omitempty tag or not.
 	// By default, all fields will be expected to have the `omitempty` tag.
 	OmitEmpty OptionalFieldsOmitEmpty `json:"omitempty"`
+
+	// omitzero is the policy for the `omitzero` tag within the json tag for fields.
+	// This defines how the linter should handle optional fields, and whether they should have the omitzero tag or not.
+	// By default, all the struct fields will be expected to have the `omitzero` tag when their zero value is not an acceptable user choice.
+	OmitZero OptionalFieldsOmitZero `json:"omitzero"`
 }
 
 // OptionalFieldsPointers is the configuration for pointers in optional fields.
@@ -49,14 +54,27 @@ type OptionalFieldsPointers struct {
 // OptionalFieldsOmitEmpty is the configuration for the `omitempty` tag on optional fields.
 type OptionalFieldsOmitEmpty struct {
 	// policy determines whether the linter should require omitempty for all optional fields.
-	// Valid values are "SuggestFix" and "Ignore".
+	// Valid values are "SuggestFix", "Warn" and "Ignore".
 	// When set to "SuggestFix", the linter will suggest adding the `omitempty` tag when an optional field does not have it.
 	// When set to "Warn", the linter will emit a warning if the field does not have the `omitempty` tag.
 	// When set to "Ignore", and optional field missing the `omitempty` tag will be ignored.
 	// Note, when set to "Ignore", and a field does not have the `omitempty` tag, this may affect whether the field should be a pointer or not.
 	Policy OptionalFieldsOmitEmptyPolicy `json:"policy"`
 }
 
+// OptionalFieldsOmitZero is the configuration for the `omitzero` tag on optional fields.
+type OptionalFieldsOmitZero struct {
+	// policy determines whether the linter should require omitzero for all optional `struct` fields.
+	// Valid values are "SuggestFix", "Warn" and "Forbid".
+	// When set to "SuggestFix", the linter will suggest adding the `omitzero` tag when an optional field does not have it.
+	// When set to "Warn", the linter will emit a warning if the field does not have the `omitzero` tag.
+	// When set to "Forbid", 'omitzero' tags wont be considered.
+	// Note, when set to "Forbid", and a field have the `omitzero` tag, the linter will suggest to remove the `omitzero` tag.
+	// Note, `omitzero` tag is supported in go version starting from go 1.24.
+	// Note, Configure omitzero policy to 'Forbid', if using with go version less than go 1.24.
+	Policy OptionalFieldsOmitZeroPolicy `json:"policy"`
+}
+
 // OptionalFieldsPointerPreference is the preference for pointers in optional fields.
 type OptionalFieldsPointerPreference string
 
@@ -92,3 +110,17 @@ const (
 	// OptionalFieldsOmitEmptyPolicyIgnore indicates that the linter will ignore any field missing the omitempty tag.
 	OptionalFieldsOmitEmptyPolicyIgnore OptionalFieldsOmitEmptyPolicy = "Ignore"
 )
+
+// OptionalFieldsOmitZeroPolicy is the policy for the omitzero tag on optional fields.
+type OptionalFieldsOmitZeroPolicy string
+
+const (
+	// OptionalFieldsOmitZeroPolicySuggestFix indicates that the linter will emit a warning if the field does not have omitzero, and suggest a fix.
+	OptionalFieldsOmitZeroPolicySuggestFix OptionalFieldsOmitZeroPolicy = "SuggestFix"
+
+	// OptionalFieldsOmitZeroPolicyWarn indicates that the linter will emit a warning if the field does not have omitzero.
+	OptionalFieldsOmitZeroPolicyWarn OptionalFieldsOmitZeroPolicy = "Warn"
+
+	// OptionalFieldsOmitZeroPolicyForbid indicates that the linter will forbid using omitzero tag.
+	OptionalFieldsOmitZeroPolicyForbid OptionalFieldsOmitZeroPolicy = "Forbid"
+)

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

@@ -288,6 +288,22 @@ type A struct {
 	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
 	// +optional
 	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
+
+	// structWithValidOmitZero is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithValidOmitZero *D `json:"structWithValidOmitZero,omitempty,omitzero"` // want "field StructWithValidOmitZero has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed."
+
+	// structWithOnlyOmitZero is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithOnlyOmitZero *D `json:"structWithOnlyOmitZero,omitzero"` // want "field StructWithOnlyOmitZero has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithOnlyOmitZero is optional and should have the omitempty tag"
+
+	// structWithValidOmitZeroWithoutPointer is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithValidOmitZeroWithoutPointer D `json:"structWithValidOmitZeroWithoutPointer,omitempty,omitzero"` // want "field StructWithValidOmitZeroWithoutPointer has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithValidOmitZeroWithoutPointer is optional and should be a pointer"
+
+	// structWithOnlyOmitZeroWithoutPointer is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithOnlyOmitZeroWithoutPointer D `json:"structWithOnlyOmitZeroWithoutPointer,omitzero"` // want "field StructWithOnlyOmitZeroWithoutPointer has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithOnlyOmitZeroWithoutPointer is optional and should have the omitempty tag" "field StructWithOnlyOmitZeroWithoutPointer is optional and should be a pointer"
 }
 
 type B struct {

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

@@ -288,6 +288,22 @@ type A struct {
 	// StringAliasWithEnumEmptyValue is a string alias field with enum validation and empty value.
 	// +optional
 	StringAliasWithEnumEmptyValue *StringAliasWithEnumEmptyValue `json:"stringAliasWithEnumEmptyValue,omitempty"`
+
+	// structWithValidOmitZero is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithValidOmitZero *D `json:"structWithValidOmitZero,omitempty"` // want "field StructWithValidOmitZero has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed."
+
+	// structWithOnlyOmitZero is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithOnlyOmitZero *D `json:"structWithOnlyOmitZero,omitempty"` // want "field StructWithOnlyOmitZero has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithOnlyOmitZero is optional and should have the omitempty tag"
+
+	// structWithValidOmitZeroWithoutPointer is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithValidOmitZeroWithoutPointer *D `json:"structWithValidOmitZeroWithoutPointer,omitempty"` // want "field StructWithValidOmitZeroWithoutPointer has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithValidOmitZeroWithoutPointer is optional and should be a pointer"
+
+	// structWithOnlyOmitZeroWithoutPointer is a struct field with a minimum number of properties on the struct so not a valid zero value.
+	// +optional
+	StructWithOnlyOmitZeroWithoutPointer *D `json:"structWithOnlyOmitZeroWithoutPointer,omitempty"` // want "field StructWithOnlyOmitZeroWithoutPointer has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed." "field StructWithOnlyOmitZeroWithoutPointer is optional and should have the omitempty tag" "field StructWithOnlyOmitZeroWithoutPointer is optional and should be a pointer"
 }
 
 type B struct {