Skip to content

feat: Handle omitzero in structfield linter #3881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gmlewis merged 9 commits into from
Dec 19, 2025
Merged

feat: Handle omitzero in structfield linter #3881

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7454100
add omitzero check in structfield linter
Not-Dhananjay-Mishra Dec 15, 2025
9be5c0b
add test cases
Not-Dhananjay-Mishra Dec 16, 2025
c3eb5ce
allowing omitzero only for slices, maps, and structs
Not-Dhananjay-Mishra Dec 16, 2025
e6ec5c6
add more test
Not-Dhananjay-Mishra Dec 16, 2025
4000936
simplified code
Not-Dhananjay-Mishra Dec 17, 2025
0ac2a9b
change msg for primitive type
Not-Dhananjay-Mishra Dec 18, 2025
21a8006
change msg for pointer primitive types
Not-Dhananjay-Mishra Dec 18, 2025
9e93909
fix linter error
Not-Dhananjay-Mishra Dec 18, 2025
2624e99
feedback
Not-Dhananjay-Mishra Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
allowing omitzero only for slices, maps, and structs
  • Loading branch information
@Not-Dhananjay-Mishra
Not-Dhananjay-Mishra committed Dec 16, 2025
commit c3eb5ce288a6e01990451748ede870d39d5b5b84
129 changes: 110 additions & 19 deletions tools/structfield/structfield.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,30 +149,101 @@ func processTag(structName string, goField *ast.Ident, field *ast.Field, structT
hasOmitEmpty := strings.Contains(tagName, ",omitempty")
hasOmitZero := strings.Contains(tagName, ",omitzero")

if hasOmitEmpty && hasOmitZero {
omitTag := "omitempty and omitzero"
checkGoFieldType(structName, goField.Name, field, field.Type.Pos(), pass, allowedTagTypes, omitTag)
switch {
// Both omitempty and omitzero
case hasOmitEmpty && hasOmitZero:
if tagType == "url" {
const msg = "the %q field in struct %q uses unsupported omitzero tag for URL tags"
pass.Reportf(field.Pos(), msg, goField.Name, structName)
} else {
checkGoFieldType(structName, goField.Name, field, field.Pos(), pass, allowedTagTypes, hasOmitEmpty, hasOmitZero)
}
tagName = strings.ReplaceAll(tagName, ",omitempty", "")
tagName = strings.ReplaceAll(tagName, ",omitzero", "")
} else if hasOmitEmpty || hasOmitZero {
omitTag := "omitempty"
if hasOmitZero {
omitTag = "omitzero"
}

checkGoFieldType(structName, goField.Name, field, field.Type.Pos(), pass, allowedTagTypes, omitTag)

// Only omitempty
case hasOmitEmpty:
checkGoFieldType(structName, goField.Name, field, field.Pos(), pass, allowedTagTypes, hasOmitEmpty, hasOmitZero)
tagName = strings.ReplaceAll(tagName, ",omitempty", "")
tagName = strings.ReplaceAll(tagName, ",omitzero", "")
}

if tagType == "url" {
tagName = strings.ReplaceAll(tagName, ",comma", "")
if tagType == "url" {
tagName = strings.ReplaceAll(tagName, ",comma", "")
}

// Only omitzero
case hasOmitZero:
if tagType == "url" {
const msg = "the %q field in struct %q uses unsupported omitzero tag for URL tags"
pass.Reportf(field.Pos(), msg, goField.Name, structName)
} else {
checkGoFieldType(structName, goField.Name, field, field.Pos(), pass, allowedTagTypes, hasOmitEmpty, hasOmitZero)
}
tagName = strings.ReplaceAll(tagName, ",omitzero", "")
}

checkGoFieldName(structName, goField.Name, tagName, goField.Pos(), pass, allowedTagNames)
}

func checkAndReportInvalidTypesForOmitzero(structName, goFieldName string, fieldType ast.Expr, tokenPos token.Pos, pass *analysis.Pass) bool {
switch ft := fieldType.(type) {
case *ast.StarExpr:
// Check for *Struct
if ident, ok := ft.X.(*ast.Ident); ok {
if obj := pass.TypesInfo.ObjectOf(ident); obj != nil {
if _, ok := obj.Type().Underlying().(*types.Struct); ok {
return true
}
}
}
// Check for *[]T where T is builtin - should be []T
if arrType, ok := ft.X.(*ast.ArrayType); ok {
// For *[]Struct
if ident, ok := arrType.Elt.(*ast.Ident); ok {
if obj := pass.TypesInfo.ObjectOf(ident); obj != nil {
if _, ok := obj.Type().Underlying().(*types.Struct); ok {
const msg = "change the %q field type to %q in the struct %q"
pass.Reportf(tokenPos, msg, goFieldName, "[]*"+ident.Name, structName)
return true
}
}
}
// For *[]*Struct
if starExpr, ok := arrType.Elt.(*ast.StarExpr); ok {
if ident, ok := starExpr.X.(*ast.Ident); ok {
const msg = "change the %q field type to %q in the struct %q"
pass.Reportf(tokenPos, msg, goFieldName, "[]*"+ident.Name, structName)
return true
}
}
// For *[]builtin
if ident, ok := arrType.Elt.(*ast.Ident); ok && isBuiltinType(ident.Name) {
const msg = "change the %q field type to %q in the struct %q"
pass.Reportf(tokenPos, msg, goFieldName, "[]"+ident.Name, structName)
return true
}
}
if _, ok := ft.X.(*ast.MapType); ok {
return true
}
// Slice
case *ast.ArrayType:
return true

// Map
case *ast.MapType:
return true

// Struct
case *ast.Ident:
if obj := pass.TypesInfo.ObjectOf(ft); obj != nil {
if _, ok := obj.Type().Underlying().(*types.Struct); ok {
return true
}
}
}
return false
}

func checkGoFieldName(structName, goFieldName, tagName string, tokenPos token.Pos, pass *analysis.Pass, allowedNames map[string]bool) {
fullName := structName + "." + goFieldName
if allowedNames[fullName] {
Expand All @@ -186,16 +257,36 @@ func checkGoFieldName(structName, goFieldName, tagName string, tokenPos token.Po
}
}

func checkGoFieldType(structName, goFieldName string, field *ast.Field, tokenPos token.Pos, pass *analysis.Pass, allowedTypes map[string]bool, omitTag string) {
func checkGoFieldType(structName, goFieldName string, field *ast.Field, tokenPos token.Pos, pass *analysis.Pass, allowedTypes map[string]bool, omitempty, omitzero bool) {
if allowedTypes[structName+"."+goFieldName] {
return
}
switch {
case omitempty && omitzero:
skipOmitzero := checkAndReportInvalidTypesForOmitzero(structName, goFieldName, field.Type, tokenPos, pass)
skipOmitempty := checkAndReportInvalidTypes(structName, goFieldName, field.Type, tokenPos, pass)
if !skipOmitzero {
const msg = `change the %q field type to a slice, map, or struct in the struct %q because its tag uses "omitzero"`
pass.Reportf(tokenPos, msg, goFieldName, structName)
}
if !skipOmitempty {
const msg = `change the %q field type to %q in the struct %q because its tag uses "omitempty"`
pass.Reportf(tokenPos, msg, goFieldName, "*"+exprToString(field.Type), structName)
}

skipOmitempty := checkAndReportInvalidTypes(structName, goFieldName, field.Type, tokenPos, pass)
case omitzero:
skipOmitzero := checkAndReportInvalidTypesForOmitzero(structName, goFieldName, field.Type, tokenPos, pass)
if !skipOmitzero {
const msg = `change the %q field type to a slice, map, or struct in the struct %q because its tag uses "omitzero"`
pass.Reportf(tokenPos, msg, goFieldName, structName)
}

if !skipOmitempty {
const msg = `change the %q field type to %q in the struct %q because its tag uses "%v"`
pass.Reportf(tokenPos, msg, goFieldName, "*"+exprToString(field.Type), structName, omitTag)
case omitempty:
skipOmitempty := checkAndReportInvalidTypes(structName, goFieldName, field.Type, tokenPos, pass)
if !skipOmitempty {
const msg = `change the %q field type to %q in the struct %q because its tag uses "omitempty"`
pass.Reportf(tokenPos, msg, goFieldName, "*"+exprToString(field.Type), structName)
}
}
}

Expand Down
24 changes: 16 additions & 8 deletions tools/structfield/testdata/src/has-warnings/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,16 @@ type JSONFieldType struct {
PointerToSliceOfStructs *[]Struct `json:"pointer_to_slice_of_structs,omitempty"` // want `change the "PointerToSliceOfStructs" field type to "\[\]\*Struct" in the struct "JSONFieldType"`
PointerToSliceOfPointerStructs *[]*Struct `json:"pointer_to_slice_of_pointer_structs,omitempty"` // want `change the "PointerToSliceOfPointerStructs" field type to "\[\]\*Struct" in the struct "JSONFieldType"`
PointerToMap *map[string]string `json:"pointer_to_map,omitempty"` // want `change the "PointerToMap" field type to "map\[string\]string" in the struct "JSONFieldType"`
SliceOfInts []*int `json:"slice_of_ints,omitempty"` // want `change the "SliceOfInts" field type to "\[\]int" in the struct "JSONFieldType"`
SliceOfZeroInts []*int `json:"slice_of_zero_ints,omitzero"` // want `change the "SliceOfZeroInts" field type to "\[\]int" in the struct "JSONFieldType"`
SliceOfBothOmitTags []*int `json:"slice_of_both_omit_tags,omitempty,omitzero"` // want `change the "SliceOfBothOmitTags" field type to "\[\]int" in the struct "JSONFieldType"`

Count int `json:"count,omitzero"` // want `change the "Count" field type to a slice, map, or struct in the struct "JSONFieldType" because its tag uses "omitzero"`
Size *int `json:"size,omitzero"` // want `change the "Size" field type to a slice, map, or struct in the struct "JSONFieldType" because its tag uses "omitzero"`
PointerToSliceOfStringsZero *[]string `json:"pointer_to_slice_of_strings_zero,omitzero"` // want `change the "PointerToSliceOfStringsZero" field type to "\[\]string" in the struct "JSONFieldType"`
PointerToSliceOfStructsZero *[]Struct `json:"pointer_to_slice_of_structs_zero,omitzero"` // want `change the "PointerToSliceOfStructsZero" field type to "\[\]\*Struct" in the struct "JSONFieldType"`
PointerToSliceOfPointerStructsZero *[]*Struct `json:"pointer_to_slice_of_pointer_structs_zero,omitzero"` // want `change the "PointerToSliceOfPointerStructsZero" field type to "\[\]\*Struct" in the struct "JSONFieldType"`
PointerSliceInt *[]int `json:"pointer_slice_int,omitempty"` // want `change the "PointerSliceInt" field type to "\[\]int" in the struct "JSONFieldType"`

PointerStructBoth Struct `json:"pointer_struct_both,omitempty,omitzero"` // want `change the "PointerStructBoth" field type to "\*Struct" in the struct "JSONFieldType" because its tag uses "omitempty"`
StringBoth *string `json:"string_both,omitempty,omitzero"` // want `change the "StringBoth" field type to a slice, map, or struct in the struct "JSONFieldType" because its tag uses "omitzero"`
}

type Struct struct{}
Expand All @@ -33,9 +40,10 @@ type URLFieldName struct {
}

type URLFieldType struct {
Page string `url:"page,omitempty"` // want `change the "Page" field type to "\*string" in the struct "URLFieldType" because its tag uses "omitempty"`
PerPage int `url:"per_page,omitempty"` // want `change the "PerPage" field type to "\*int" in the struct "URLFieldType" because its tag uses "omitempty"`
Participating bool `url:"participating,omitempty"` // want `change the "Participating" field type to "\*bool" in the struct "URLFieldType" because its tag uses "omitempty"`
PerPageZeros int `url:"per_page_zeros,omitzero"` // want `change the "PerPageZeros" field type to "\*int" in the struct "URLFieldType" because its tag uses "omitzero"`
PerPageBoth int `url:"per_page_both,omitempty,omitzero"` // want `change the "PerPageBoth" field type to "\*int" in the struct "URLFieldType" because its tag uses "omitempty and omitzero"`
Page string `url:"page,omitempty"` // want `change the "Page" field type to "\*string" in the struct "URLFieldType" because its tag uses "omitempty"`
PerPage int `url:"per_page,omitempty"` // want `change the "PerPage" field type to "\*int" in the struct "URLFieldType" because its tag uses "omitempty"`
Participating bool `url:"participating,omitempty"` // want `change the "Participating" field type to "\*bool" in the struct "URLFieldType" because its tag uses "omitempty"`

PerPageZeros []int `url:"per_page_zeros,omitzero"` // want `the "PerPageZeros" field in struct "URLFieldType" uses unsupported omitzero tag for URL tags`
PerPageBoth *int `url:"per_page_both,omitempty,omitzero"` // want `the "PerPageBoth" field in struct "URLFieldType" uses unsupported omitzero tag for URL tags`
}
16 changes: 14 additions & 2 deletions tools/structfield/testdata/src/no-warnings/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,20 @@ type JSONFieldType struct {
Exception string `json:"exception,omitempty"`
Value any `json:"value,omitempty"`
SliceOfPointerStructs []*Struct `json:"slice_of_pointer_structs,omitempty"`
SliceOfZerosStructs []*Struct `json:"slice_of_zeros_structs,omitzero"`
SliceOfBothOmitTags []*Struct `json:"slice_of_both_omit_tags,omitempty,omitzero"`

SliceOfStrings []string `json:"slice_of_strings,omitzero"`
SliceOfPointerInts []*int `json:"slice_of_pointer_ints,omitzero"`
MapOfStringToInt map[string]int `json:"map_of_string_to_int,omitzero"`
MapOfPointerStringToInt *map[string]int `json:"map_of_pointer_string_to_int,omitzero"`
StructField Struct `json:"struct_field,omitzero"`
Copy link
Collaborator

@gmlewis gmlewis Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having troubles with:

  • *map[...]...
  • Struct
  • []Struct

I still think these should not be allowed, but instead use:

  • map[...]...
  • *Struct
  • []*Struct

But maybe I'm just not getting the whole thing?

cc: @stevehipwell - @alexandear - @zyfy29 - please help me make sense of how we should handle omitzero in this repo, as I'm really struggling with this one.

Copy link
Contributor Author

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I go ahead with this :

Allowing only - map[...]... , *Struct , []*Struct and []buildin as suggested.

I tried a few small experiments with different field shapes. From what I observed, even when the data is “empty”, these values are still considered non-zero and therefore are not omitted during marshaling.

I tested with the following example.

type DummyDataForOmitZero struct {
	OmitZeroWithoutPointerSlice []int  `json:"omit_zero_without_pointer_slice,omitzero"`
	OmitZeroWithPointerSlice    []*int `json:"omit_zero_with_pointer_slice,omitzero"`

	OmitZeroWithoutPointerStruct Struct  `json:"omit_zero_without_pointer_struct,omitzero"`
	OmitZeroWithPointerStruct    *Struct `json:"omit_zero_with_pointer_struct,omitzero"`

	OmitZeroWithoutPointerSliceofStruct []Struct  `json:"omit_zero_without_pointer_sliceofstruct,omitzero"`
	OmitZeroWithPointerSliceofStruct    []*Struct `json:"omit_zero_with_pointer_sliceofstruct,omitzero"`

	OmitZeroWithoutPointerMap map[string]string  `json:"omit_zero_without_pointer_map,omitzero"`
	OmitZeroWithPointerMap    *map[string]string `json:"omit_zero_with_pointer_map,omitzero"`
}

Empty input:

		data := DummyDataForOmitZero{
			OmitZeroWithoutPointerSlice:         []int{},
			OmitZeroWithPointerSlice:            []*int{},
			OmitZeroWithoutPointerSliceofStruct: []Struct{},
			OmitZeroWithPointerSliceofStruct:    []*Struct{},
			OmitZeroWithoutPointerStruct:        Struct{},
			OmitZeroWithPointerStruct:           &Struct{},
			OmitZeroWithoutPointerMap:           map[string]string{},
			OmitZeroWithPointerMap:              &map[string]string{},
		}

Marshaled output:

{
  "omit_zero_without_pointer_slice": [],
  "omit_zero_with_pointer_slice": [],
  "omit_zero_with_pointer_struct": {},
  "omit_zero_without_pointer_sliceofstruct": [],
  "omit_zero_with_pointer_sliceofstruct": [],
  "omit_zero_without_pointer_map": {},
  "omit_zero_with_pointer_map": {}
}

or should non-pointer values be allowed as well?
any opinion?

Copy link
Collaborator

@gmlewis gmlewis Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome... thank you for the excellent experiment... that is useful to see the results.

Here's my opinion, and I am totally happy to be corrected or discuss:

I have some ideas of what should never be used in this repo (regardless the JSON tags) unless there is a REALLY GOOD reason (which, granted, does indeed sometimes happen):

  • pointers to slices or maps (which are already reference types)
  • slices of pointers to primitives
  • slices of structs without the pointers (for iteration)

As for the use of omitzero, I don't have a good enough "feel" for them yet to really understand their use.

Looking carefully at the above experiment, only one case was removed by omitzero:

  • OmitZeroWithoutPointerStruct Struct json:"omit_zero_without_pointer_struct,omitzero"

But I'm struggling to figure out if that is a really useful case in this repo. It seems like a very targeted and specific use case... one that we might indeed find useful, but honestly I don't know where.

So in my opinion, if we still don't allow my list above, then I think we may be able to proceed.

Copy link
Contributor Author

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense.
I’ll proceed with -

Allowing only

  • map[...]...
  • *Struct
  • []*Struct
  • []builtin (e.g. []int, []string .... )

We can adjust the linter later if necessary.

PointerStructField *Struct `json:"pointer_struct_field,omitzero"`
SliceOfPointerStructsZero []*Struct `json:"slice_of_pointer_structs_zero,omitzero"`
SliceOfNonPointerStructsZero []Struct `json:"slice_of_non_pointer_structs_zero,omitzero"`

SliceOfStringsBoth []string `json:"slice_of_strings_both,omitzero,omitempty"`
MapOfStringToIntBoth map[string]int `json:"map_of_string_to_int_both,omitzero,omitempty"`
SliceOfPointerStructsBoth []*Struct `json:"slice_of_pointer_structs_both,omitzero,omitempty"`
StructFieldBoth *Struct `json:"struct_field_both,omitzero,omitempty"`
}

type URLFieldName struct {
Expand Down