Address review comments

Author: Karthik-K-N

pkg/analysis/optionalfields/analyzer.go

@@ -56,6 +56,7 @@ type analyzer struct {
 	pointerPolicy     OptionalFieldsPointerPolicy
 	pointerPreference OptionalFieldsPointerPreference
 	omitEmptyPolicy   OptionalFieldsOmitEmptyPolicy
+	omitZeroPolicy    OptionalFieldsOmitZeroPolicy
 }
 
 // newAnalyzer creates a new analyzer.
@@ -70,6 +71,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{
@@ -132,6 +134,10 @@ 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) {
@@ -144,25 +150,35 @@ func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, f
 	if a.pointerPreference == OptionalFieldsPointerPreferenceAlways {
 		// The field must always be a pointer, pointers require omitempty, so enforce that too.
 		a.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying)
-		a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct, jsonTags)
+		a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
 
 		return
 	}
 
 	// The pointer preference is now when required.
+	// Validate for omitzero policy.
+	if a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyIgnore || hasOmitZero {
+		// If we require omitzero, or the field has omitzero, we can check the field properties based on it being an omitzero field.
+		a.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, hasValidZeroValue, isPointer, isStruct)
+	} else {
+		// The field does not have omitzero, and does not require it.
+		a.checkFieldPropertiesWithoutOmitZero(pass, field, fieldName, jsonTags, hasValidZeroValue, isPointer, isStruct)
+	}
 
+	// 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, hasOmitZero, hasValidZeroValue, completeValidation, isPointer, isStruct)
 	} else {
 		// The field does not have omitempty, and does not require it.
-		a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct, hasOmitZero)
+		a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct)
 	}
 }
 
 func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasOmitZero, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
 	// In this case, we should always add the omitempty if it isn't present.
-	a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct, jsonTags)
+	a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags)
 
 	switch {
 	case !hasValidZeroValue && isStruct && hasOmitZero:
@@ -181,7 +197,7 @@ func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass
 	}
 }
 
-func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct, hasOmitZero bool) {
+func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool) {
 	switch {
 	case hasValidZeroValue:
 		// The field is not omitempty, and the zero value is valid, the field does not need to be a pointer.
@@ -198,6 +214,39 @@ func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, fie
 	}
 }
 
+func (a *analyzer) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, hasValidZeroValue, isPointer, isStruct bool) {
+	// In this case, we should always add the omitzero for struct fields if it isn't present.
+	a.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, isStruct, jsonTags)
+
+	if !hasValidZeroValue && isStruct {
+		// The struct field need not be pointer if it does not have a valid zero value.
+		a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not allow the zero value. The field does not need to be a pointer.")
+	}
+}
+
+func (a *analyzer) checkFieldPropertiesWithoutOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasValidZeroValue, isPointer, isStruct bool) {
+	if !isStruct {
+		// Handle omitzero only for struct fields.
+		return
+	}
+
+	switch {
+	case hasValidZeroValue:
+		// The field is not omitzero, and the zero value is valid, the field does not need to be a pointer.
+		a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional, without omitzero and allows the zero value. The field does not need to be a pointer.")
+	case !hasValidZeroValue && isStruct:
+		// The zero value would not be accepted, so the struct field needs to have omitzero.
+		// Force the omitzero policy to suggest a fix. We can only get to this function when the omitzero policy is configured to Ignore.
+		// Since we absolutely have to add the omitzero tag, we can report it as a suggestion.
+		reportShouldAddOmitZero(pass, field, OptionalFieldsOmitZeroPolicySuggestFix, fieldName, "field %s is struct and optional and does not allow the zero value. It must have the omitzero tag.", jsonTags)
+
+		// Once it has the omitzero tag, it will also need to be a pointer in some cases.
+		// Now handle it as if it had the omitzero already.
+		// We already handle the omitzero tag above, so force the `hasOmitZero` to true.
+		a.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, true, hasValidZeroValue, isPointer, isStruct)
+	}
+}
+
 func (a *analyzer) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
 	if isPointerType(pass, underlying) {
 		if isPointer {
@@ -232,17 +281,20 @@ func (a *analyzer) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast
 	reportShouldRemovePointer(pass, field, a.pointerPolicy, fieldName, message)
 }
 
-func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty, hasOmitZero, hasValidZeroValue, isStruct bool, jsonTags extractjsontags.FieldTagInfo) {
+func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty bool, jsonTags extractjsontags.FieldTagInfo) {
 	if hasOmitEmpty {
 		return
 	}
 
-	if !hasValidZeroValue && isStruct && hasOmitZero {
-		// The struct field need not have omitempty tag if it does not have a valid zero value and has omitzero tag.
+	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, isStruct bool, jsonTags extractjsontags.FieldTagInfo) {
+	if hasOmitZero || !isStruct {
 		return
 	}
-
-	reportShouldAddOmitEmpty(pass, field, a.omitEmptyPolicy, fieldName, "field %s is optional and should have the omitempty tag", jsonTags)
+	// Currently, add omitzero tags to only struct fields.
+	reportShouldAddOmitZero(pass, field, a.omitZeroPolicy, fieldName, "field %s is optional and should have the omitzero tag", jsonTags)
 }
 
 func (a *analyzer) handleIncompleteFieldValidation(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) {
@@ -312,9 +364,15 @@ func getStructZeroValue(pass *analysis.Pass, structType *ast.StructType) string
 	for _, field := range structType.Fields.List {
 		fieldTagInfo := jsonTagInfo.FieldTags(field)
 
-		if fieldTagInfo.OmitEmpty || fieldTagInfo.OmitZero {
+		isPointer, _ := isStarExpr(field.Type)
+		if !isPointer && fieldTagInfo.OmitZero {
+			// for non-pointer field if it has omitzero, we can use a zero value.
+			continue
+		}
+
+		if fieldTagInfo.OmitEmpty {
 			// If the field is omitted, we can use a zero value.
-			// For structs, if they aren't a pointer another error will be raised.
+			// For structs, if they aren't a pointer, another error will be raised.
 			continue
 		}
 

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.
+	OmitZero OptionalFieldsOmitZero `json:"omitzero"`
 }
 
 // OptionalFieldsPointers is the configuration for pointers in optional fields.
@@ -57,6 +62,17 @@ type OptionalFieldsOmitEmpty struct {
 	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" and "Ignore".
+	// 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 "Ignore", and optional field missing the `omitzero` tag will be ignored.
+	// Note, when set to "Ignore", and a field does not have the `omitzero` tag, this may affect whether the field should be a pointer or not.
+	Policy OptionalFieldsOmitZeroPolicy `json:"policy"`
+}
+
 // OptionalFieldsPointerPreference is the preference for pointers in optional fields.
 type OptionalFieldsPointerPreference string
 
@@ -92,3 +108,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"
+
+	// OptionalFieldsOmitZeroPolicyIgnore indicates that the linter will ignore any field missing the omitzero tag.
+	OptionalFieldsOmitZeroPolicyIgnore OptionalFieldsOmitZeroPolicy = "Ignore"
+)

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

@@ -179,7 +179,7 @@ type A struct {
 	// structWithOnlyOmitZeroTag is a struct field with a minimum number of properties and has only omitzero tag.
 	// +kubebuilder:validation:MinProperties=1
 	// +optional
-	StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"`
+	StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"` // want "field StructWithOnlyOmitZeroTag is optional and should have the omitempty tag"
 
 	// structWithMinPropertiesOnStruct is a struct field with a minimum number of properties on the struct.
 	// +optional

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

@@ -179,7 +179,7 @@ type A struct {
 	// structWithOnlyOmitZeroTag is a struct field with a minimum number of properties and has only omitzero tag.
 	// +kubebuilder:validation:MinProperties=1
 	// +optional
-	StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"`
+    StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitempty,omitzero"`  // want "field StructWithOnlyOmitZeroTag is optional and should have the omitempty tag"
 
 	// structWithMinPropertiesOnStruct is a struct field with a minimum number of properties on the struct.
 	// +optional

pkg/analysis/optionalfields/util.go

@@ -149,6 +149,34 @@ func reportShouldAddOmitEmpty(pass *analysis.Pass, field *ast.Field, omitEmptyPo
 	}
 }
 
+// reportShouldAddOmitZero adds an analysis diagnostic that explains that an omitzero tag should be added.
+func reportShouldAddOmitZero(pass *analysis.Pass, field *ast.Field, omitZeroPolicy OptionalFieldsOmitZeroPolicy, fieldName, messageFmt string, fieldTagInfo extractjsontags.FieldTagInfo) {
+	switch omitZeroPolicy {
+	case OptionalFieldsOmitZeroPolicySuggestFix:
+		pass.Report(analysis.Diagnostic{
+			Pos:     field.Pos(),
+			Message: fmt.Sprintf(messageFmt, fieldName),
+			SuggestedFixes: []analysis.SuggestedFix{
+				{
+					Message: fmt.Sprintf("should add 'omitzero' to the field tag for field %s", fieldName),
+					TextEdits: []analysis.TextEdit{
+						{
+							Pos:     fieldTagInfo.Pos + token.Pos(len(fieldTagInfo.Name)),
+							NewText: []byte(",omitzero"),
+						},
+					},
+				},
+			},
+		})
+	case OptionalFieldsOmitZeroPolicyWarn:
+		pass.Reportf(field.Pos(), messageFmt, fieldName)
+	case OptionalFieldsOmitZeroPolicyIgnore:
+		// Do nothing, as the policy is to ignore the missing omitzero tag.
+	default:
+		panic(fmt.Sprintf("unknown omit zero policy: %s", omitZeroPolicy))
+	}
+}
+
 // isZeroValueValid determines whether the zero value of the field is valid per the validation markers.
 // For example, if the string has a minimum length greater than 0, the zero value is not valid.
 // Or if the minimum value of an integer field is greater than 0, the zero value is not valid.
@@ -223,7 +251,13 @@ func areStructFieldZeroValuesValid(pass *analysis.Pass, structType *ast.StructTy
 	for _, field := range structType.Fields.List {
 		fieldTagInfo := jsonTagInfo.FieldTags(field)
 
-		if fieldTagInfo.OmitEmpty || fieldTagInfo.OmitZero {
+		isPointer, _ := isStarExpr(field.Type)
+		if !isPointer && fieldTagInfo.OmitZero {
+			// for non-pointer field if it has omitzero, we can use a zero value.
+			continue
+		}
+
+		if fieldTagInfo.OmitEmpty {
 			// If the field is omitted, we can use a zero value.
 			// For structs, if they aren't a pointer another error will be raised.
 			continue