Skip to content

Commit e1b2381

Browse files
adonovanadonovan
committed
go/analysis/passes/modernize: omitzero: suppress on kubebuilder
This CL causes the omitzero pass to be silent if the file mentions "+kubebuilder" in a comment, since that system has its own interpretation of what "omitzero" means in a json tag. This a slightly sleazy heuristic; if it proves inadequate in practice then we should (sadly) disable this modernizer. Fixes golang/go#76649 Change-Id: I0683dd8ea99c777bf0fd1ce89b23f20284c15b00 Reviewed-on: https://go-review.googlesource.com/c/tools/+/725681 Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Madeline Kalil <mkalil@google.com>
1 parent d1bcb8b commit e1b2381

File tree

7 files changed

+118
-75
lines changed

7 files changed

+118
-75
lines changed

‎go/analysis/passes/modernize/doc.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,12 +199,19 @@ are often used to express optionality.
199199
200200
omitzero: suggest replacing omitempty with omitzero for struct fields
201201
202-
The omitzero analyzer identifies uses of the `omitempty` JSON struct tag on
203-
fields that are themselves structs. The `omitempty` tag has no effect on
204-
struct-typed fields. The analyzer offers two suggestions: either remove the
202+
The omitzero analyzer identifies uses of the `omitempty` JSON struct
203+
tag on fields that are themselves structs. For struct-typed fields,
204+
the `omitempty` tag has no effect on the behavior of json.Marshal and
205+
json.Unmarshal. The analyzer offers two suggestions: either remove the
205206
tag, or replace it with `omitzero` (added in Go 1.24), which correctly
206207
omits the field if the struct value is zero.
207208
209+
However, some other serialization packages (notably kubebuilder, see
210+
https://book.kubebuilder.io/reference/markers.html) may have their own
211+
interpretation of the `json:",omitzero"` tag, so removing it may affect
212+
program behavior. For this reason, the omitzero modernizer will not
213+
make changes in any package that contains +kubebuilder annotations.
214+
208215
Replacing `omitempty` with `omitzero` is a change in behavior. The
209216
original code would always encode the struct field, whereas the
210217
modified code will omit it if it is a zero-value.

‎go/analysis/passes/modernize/modernize_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func TestNewExpr(t *testing.T) {
5555
}
5656

5757
func TestOmitZero(t *testing.T) {
58-
RunWithSuggestedFixes(t, TestData(), modernize.OmitZeroAnalyzer, "omitzero")
58+
RunWithSuggestedFixes(t, TestData(), modernize.OmitZeroAnalyzer, "omitzero/...")
5959
}
6060

6161
func TestRangeInt(t *testing.T) {

‎go/analysis/passes/modernize/omitzero.go‎

Lines changed: 89 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"go/types"
1010
"reflect"
1111
"strconv"
12+
"strings"
13+
"sync"
1214

1315
"golang.org/x/tools/go/analysis"
1416
"golang.org/x/tools/go/analysis/passes/inspect"
@@ -25,82 +27,106 @@ var OmitZeroAnalyzer = &analysis.Analyzer{
2527
URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/modernize#omitzero",
2628
}
2729

28-
func checkOmitEmptyField(pass *analysis.Pass, info *types.Info, curField *ast.Field) {
29-
typ := info.TypeOf(curField.Type)
30-
_, ok := typ.Underlying().(*types.Struct)
31-
if !ok {
32-
// Not a struct
33-
return
34-
}
35-
tag := curField.Tag
36-
if tag == nil {
37-
// No tag to check
38-
return
39-
}
40-
// The omitempty tag may be used by other packages besides json, but we should only modify its use with json
41-
tagconv, _ := strconv.Unquote(tag.Value)
42-
match := omitemptyRegex.FindStringSubmatchIndex(tagconv)
43-
if match == nil {
44-
// No omitempty in json tag
45-
return
46-
}
47-
omitEmpty, err := astutil.RangeInStringLiteral(curField.Tag, match[2], match[3])
48-
if err != nil {
49-
return
50-
}
51-
var remove analysis.Range = omitEmpty
30+
// The omitzero pass searches for instances of "omitempty" in a json field tag on a
31+
// struct. Since "omitfilesUsingGoVersions not have any effect when applied to a struct field,
32+
// it suggests either deleting "omitempty" or replacing it with "omitzero", which
33+
// correctly excludes structs from a json encoding.
34+
func omitzero(pass *analysis.Pass) (any, error) {
35+
// usesKubebuilder reports whether "+kubebuilder:" appears in
36+
// any comment in the package, since it has its own
37+
// interpretation of what omitzero means; see go.dev/issue/76649.
38+
// It is computed once, on demand.
39+
usesKubebuilder := sync.OnceValue[bool](func() bool {
40+
for _, file := range pass.Files {
41+
for _, comment := range file.Comments {
42+
if strings.Contains(comment.Text(), "+kubebuilder:") {
43+
return true
44+
}
45+
}
46+
}
47+
return false
48+
})
49+
50+
checkField := func(field *ast.Field) {
51+
typ := pass.TypesInfo.TypeOf(field.Type)
52+
_, ok := typ.Underlying().(*types.Struct)
53+
if !ok {
54+
// Not a struct
55+
return
56+
}
57+
tag := field.Tag
58+
if tag == nil {
59+
// No tag to check
60+
return
61+
}
62+
// The omitempty tag may be used by other packages besides json, but we should only modify its use with json
63+
tagconv, _ := strconv.Unquote(tag.Value)
64+
match := omitemptyRegex.FindStringSubmatchIndex(tagconv)
65+
if match == nil {
66+
// No omitempty in json tag
67+
return
68+
}
69+
omitEmpty, err := astutil.RangeInStringLiteral(field.Tag, match[2], match[3])
70+
if err != nil {
71+
return
72+
}
73+
var remove analysis.Range = omitEmpty
5274

53-
jsonTag := reflect.StructTag(tagconv).Get("json")
54-
if jsonTag == ",omitempty" {
55-
// Remove the entire struct tag if json is the only package used
56-
if match[1]-match[0] == len(tagconv) {
57-
remove = curField.Tag
58-
} else {
59-
// Remove the json tag if omitempty is the only field
60-
remove, err = astutil.RangeInStringLiteral(curField.Tag, match[0], match[1])
61-
if err != nil {
62-
return
75+
jsonTag := reflect.StructTag(tagconv).Get("json")
76+
if jsonTag == ",omitempty" {
77+
// Remove the entire struct tag if json is the only package used
78+
if match[1]-match[0] == len(tagconv) {
79+
remove = field.Tag
80+
} else {
81+
// Remove the json tag if omitempty is the only field
82+
remove, err = astutil.RangeInStringLiteral(field.Tag, match[0], match[1])
83+
if err != nil {
84+
return
85+
}
6386
}
6487
}
65-
}
66-
pass.Report(analysis.Diagnostic{
67-
Pos: curField.Tag.Pos(),
68-
End: curField.Tag.End(),
69-
Message: "Omitempty has no effect on nested struct fields",
70-
SuggestedFixes: []analysis.SuggestedFix{
71-
{
72-
Message: "Remove redundant omitempty tag",
73-
TextEdits: []analysis.TextEdit{
74-
{
75-
Pos: remove.Pos(),
76-
End: remove.End(),
88+
89+
// Don't offer a fix if the package seems to use kubebuilder,
90+
// as it has its own intepretation of "omitzero" tags.
91+
// https://book.kubebuilder.io/reference/markers.html
92+
if usesKubebuilder() {
93+
return
94+
}
95+
96+
pass.Report(analysis.Diagnostic{
97+
Pos: field.Tag.Pos(),
98+
End: field.Tag.End(),
99+
Message: "Omitempty has no effect on nested struct fields",
100+
SuggestedFixes: []analysis.SuggestedFix{
101+
{
102+
Message: "Remove redundant omitempty tag",
103+
TextEdits: []analysis.TextEdit{
104+
{
105+
Pos: remove.Pos(),
106+
End: remove.End(),
107+
},
77108
},
78109
},
79-
},
80-
{
81-
Message: "Replace omitempty with omitzero (behavior change)",
82-
TextEdits: []analysis.TextEdit{
83-
{
84-
Pos: omitEmpty.Pos(),
85-
End: omitEmpty.End(),
86-
NewText: []byte(",omitzero"),
110+
{
111+
Message: "Replace omitempty with omitzero (behavior change)",
112+
TextEdits: []analysis.TextEdit{
113+
{
114+
Pos: omitEmpty.Pos(),
115+
End: omitEmpty.End(),
116+
NewText: []byte(",omitzero"),
117+
},
87118
},
88119
},
89-
},
90-
}})
91-
}
120+
}})
121+
}
92122

93-
// The omitzero pass searches for instances of "omitempty" in a json field tag on a
94-
// struct. Since "omitfilesUsingGoVersions not have any effect when applied to a struct field,
95-
// it suggests either deleting "omitempty" or replacing it with "omitzero", which
96-
// correctly excludes structs from a json encoding.
97-
func omitzero(pass *analysis.Pass) (any, error) {
98123
for curFile := range filesUsingGoVersion(pass, versions.Go1_24) {
99124
for curStruct := range curFile.Preorder((*ast.StructType)(nil)) {
100125
for _, curField := range curStruct.Node().(*ast.StructType).Fields.List {
101-
checkOmitEmptyField(pass, pass.TypesInfo, curField)
126+
checkField(curField)
102127
}
103128
}
104129
}
130+
105131
return nil, nil
106132
}

‎go/analysis/passes/modernize/testdata/src/omitzero/kube/kube.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package kube
2+
3+
type Foo struct {
4+
EmptyStruct struct{} `json:",omitempty"` // nope: the comment below mentions the k-word
5+
}
6+
7+
// +kubebuilder:validation:Optional
8+
type Other struct {
9+
}

‎gopls/doc/analyzers.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3596,7 +3596,9 @@ Package documentation: [noresultvalues](https://pkg.go.dev/golang.org/x/tools/go
35963596
<a id='omitzero'></a>
35973597
## `omitzero`: suggest replacing omitempty with omitzero for struct fields
35983598

3599-
The omitzero analyzer identifies uses of the \`omitempty\` JSON struct tag on fields that are themselves structs. The \`omitempty\` tag has no effect on struct-typed fields. The analyzer offers two suggestions: either remove the tag, or replace it with \`omitzero\` (added in Go 1.24), which correctly omits the field if the struct value is zero.
3599+
The omitzero analyzer identifies uses of the \`omitempty\` JSON struct tag on fields that are themselves structs. For struct-typed fields, the \`omitempty\` tag has no effect on the behavior of json.Marshal and json.Unmarshal. The analyzer offers two suggestions: either remove the tag, or replace it with \`omitzero\` (added in Go 1.24), which correctly omits the field if the struct value is zero.
3600+
3601+
However, some other serialization packages (notably kubebuilder, see [https://book.kubebuilder.io/reference/markers.html](https://book.kubebuilder.io/reference/markers.html)) may have their own interpretation of the \`json:",omitzero"\` tag, so removing it may affect program behavior. For this reason, the omitzero modernizer will not make changes in any package that contains +kubebuilder annotations.
36003602

36013603
Replacing \`omitempty\` with \`omitzero\` is a change in behavior. The original code would always encode the struct field, whereas the modified code will omit it if it is a zero-value.
36023604

‎gopls/internal/doc/api.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1576,7 +1576,7 @@
15761576
},
15771577
{
15781578
"Name": "\"omitzero\"",
1579-
"Doc": "suggest replacing omitempty with omitzero for struct fields\n\nThe omitzero analyzer identifies uses of the `omitempty` JSON struct tag on\nfields that are themselves structs. The `omitempty` tag has no effect on\nstruct-typed fields. The analyzer offers two suggestions: either remove the\ntag, or replace it with `omitzero` (added in Go 1.24), which correctly\nomits the field if the struct value is zero.\n\nReplacing `omitempty` with `omitzero` is a change in behavior. The\noriginal code would always encode the struct field, whereas the\nmodified code will omit it if it is a zero-value.",
1579+
"Doc": "suggest replacing omitempty with omitzero for struct fields\n\nThe omitzero analyzer identifies uses of the `omitempty` JSON struct\ntag on fields that are themselves structs. For struct-typed fields,\nthe `omitempty` tag has no effect on the behavior of json.Marshal and\njson.Unmarshal. The analyzer offers two suggestions: either remove the\ntag, or replace it with `omitzero` (added in Go 1.24), which correctly\nomits the field if the struct value is zero.\n\nHowever, some other serialization packages (notably kubebuilder, see\nhttps://book.kubebuilder.io/reference/markers.html) may have their own\ninterpretation of the `json:\",omitzero\"` tag, so removing it may affect\nprogram behavior. For this reason, the omitzero modernizer will not\nmake changes in any package that contains +kubebuilder annotations.\n\nReplacing `omitempty` with `omitzero` is a change in behavior. The\noriginal code would always encode the struct field, whereas the\nmodified code will omit it if it is a zero-value.",
15801580
"Default": "true",
15811581
"Status": ""
15821582
},
@@ -3479,7 +3479,7 @@
34793479
},
34803480
{
34813481
"Name": "omitzero",
3482-
"Doc": "suggest replacing omitempty with omitzero for struct fields\n\nThe omitzero analyzer identifies uses of the `omitempty` JSON struct tag on\nfields that are themselves structs. The `omitempty` tag has no effect on\nstruct-typed fields. The analyzer offers two suggestions: either remove the\ntag, or replace it with `omitzero` (added in Go 1.24), which correctly\nomits the field if the struct value is zero.\n\nReplacing `omitempty` with `omitzero` is a change in behavior. The\noriginal code would always encode the struct field, whereas the\nmodified code will omit it if it is a zero-value.",
3482+
"Doc": "suggest replacing omitempty with omitzero for struct fields\n\nThe omitzero analyzer identifies uses of the `omitempty` JSON struct\ntag on fields that are themselves structs. For struct-typed fields,\nthe `omitempty` tag has no effect on the behavior of json.Marshal and\njson.Unmarshal. The analyzer offers two suggestions: either remove the\ntag, or replace it with `omitzero` (added in Go 1.24), which correctly\nomits the field if the struct value is zero.\n\nHowever, some other serialization packages (notably kubebuilder, see\nhttps://book.kubebuilder.io/reference/markers.html) may have their own\ninterpretation of the `json:\",omitzero\"` tag, so removing it may affect\nprogram behavior. For this reason, the omitzero modernizer will not\nmake changes in any package that contains +kubebuilder annotations.\n\nReplacing `omitempty` with `omitzero` is a change in behavior. The\noriginal code would always encode the struct field, whereas the\nmodified code will omit it if it is a zero-value.",
34833483
"URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/modernize#omitzero",
34843484
"Default": true
34853485
},

‎gopls/internal/protocol/tsserver.go‎

Lines changed: 4 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)