[go] encoding/json: add omitzero option
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
encoding/json: add omitzero option
Fixes #45669
Change diff
diff --git a/doc/next/6-stdlib/99-minor/encoding/json/45669.md b/doc/next/6-stdlib/99-minor/encoding/json/45669.md
new file mode 100644
index 0000000..ec18cce
--- /dev/null
+++ b/doc/next/6-stdlib/99-minor/encoding/json/45669.md
@@ -0,0 +1,11 @@
+When marshaling, the `omitzero` option specifies that the struct field should be
+omitted if the field value is zero as determined by the `IsZero() bool` method
+if present, otherwise based on whether the field is the zero Go value (according
+to [reflect.Value.IsZero]).
+
+This option has no effect when unmarshaling. If `omitempty` is specified together
+with `omitzero`, whether a field is omitted is based on the logical OR of the two.
+
+This will mean that `omitzero` of a slice omits a nil slice but emits [] for a
+zero-length non-nil slice (and similar for maps). It will also mean that
+`omitzero` of a [time.Time] omits time.Time{}.
\ No newline at end of file
diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go
index 988de71..671cd3f 100644
--- a/src/encoding/json/encode.go
+++ b/src/encoding/json/encode.go
@@ -318,6 +318,17 @@
return false
}
+type zeroable interface {
+ IsZero() bool
+}
+
+func isZeroValue(v reflect.Value) bool {
+ if z, ok := v.Interface().(zeroable); ok {
+ return z.IsZero()
+ }
+ return v.IsZero()
+}
+
func (e *encodeState) reflectValue(v reflect.Value, opts encOpts) {
valueEncoder(v)(e, v, opts)
}
@@ -701,7 +712,8 @@
fv = fv.Field(i)
}
- if f.omitEmpty && isEmptyValue(fv) {
+ if (f.omitEmpty && isEmptyValue(fv)) ||
+ (f.omitZero && isZeroValue(fv)) {
continue
}
e.WriteByte(next)
@@ -1048,6 +1060,7 @@
index []int
typ reflect.Type
omitEmpty bool
+ omitZero bool
quoted bool
encoder encoderFunc
@@ -1154,6 +1167,7 @@
index: index,
typ: ft,
omitEmpty: opts.Contains("omitempty"),
+ omitZero: opts.Contains("omitzero"),
quoted: quoted,
}
field.nameBytes = []byte(field.name)
diff --git a/src/encoding/json/encode_test.go b/src/encoding/json/encode_test.go
index 23a14d0..388d4dd 100644
--- a/src/encoding/json/encode_test.go
+++ b/src/encoding/json/encode_test.go
@@ -15,9 +15,10 @@
"runtime/debug"
"strconv"
"testing"
+ "time"
)
-type Optionals struct {
+type OptionalsEmpty struct {
Sr string `json:"sr"`
So string `json:"so,omitempty"`
Sw string `json:"-"`
@@ -56,7 +57,7 @@
"str": {},
"sto": {}
}`
- var o Optionals
+ var o OptionalsEmpty
o.Sw = "something"
o.Mr = map[string]any{}
o.Mo = map[string]any{}
@@ -70,6 +71,120 @@
}
}
+type OptionalsZero struct {
+ Sr string `json:"sr"`
+ So string `json:"so,omitzero"`
+ Sw string `json:"-"`
+
+ Ir int `json:"omitempty"` // actually named omitempty, not an option
+ Io int `json:"io,omitzero"`
+
+ Slr []string `json:"slr,random"`
+ Slo []string `json:"slo,omitzero"`
+ SloNonNil []string `json:"slononnil,omitzero"`
+
+ Mr map[string]any `json:"mr"`
+ Mo map[string]any `json:",omitzero"`
+
+ Fr float64 `json:"fr"`
+ Fo float64 `json:"fo,omitzero"`
+
+ Br bool `json:"br"`
+ Bo bool `json:"bo,omitzero"`
+
+ Ur uint `json:"ur"`
+ Uo uint `json:"uo,omitzero"`
+
+ Str struct{} `json:"str"`
+ Sto struct{} `json:"sto,omitzero"`
+
+ MyTime time.Time `json:"mytime,omitzero"`
+}
+
+func TestOmitZero(t *testing.T) {
+ var want = `{
+ "sr": "",
+ "omitempty": 0,
+ "slr": null,
+ "slononnil": [],
+ "mr": {},
+ "Mo": {},
+ "fr": 0,
+ "br": false,
+ "ur": 0,
+ "str": {}
+}`
+ var o OptionalsZero
+ o.Sw = "something"
+ o.SloNonNil = make([]string, 0)
+ o.Mr = map[string]any{}
+ o.Mo = map[string]any{}
+
+ got, err := MarshalIndent(&o, "", " ")
+ if err != nil {
+ t.Fatalf("MarshalIndent error: %v", err)
+ }
+ if got := string(got); got != want {
+ t.Errorf("MarshalIndent:\n\tgot: %s\n\twant: %s\n", indentNewlines(got), indentNewlines(want))
+ }
+}
+
+type OptionalsEmptyZero struct {
+ Sr string `json:"sr"`
+ So string `json:"so,omitempty,omitzero"`
+ Sw string `json:"-"`
+
+ Ir int `json:"omitempty"` // actually named omitempty, not an option
+ Io int `json:"io,omitempty,omitzero"`
+
+ Slr []string `json:"slr,random"`
+ Slo []string `json:"slo,omitempty,omitzero"`
+ SloNonNil []string `json:"slononnil,omitempty,omitzero"`
+
+ Mr map[string]any `json:"mr"`
+ Mo map[string]any `json:",omitempty,omitzero"`
+
+ Fr float64 `json:"fr"`
+ Fo float64 `json:"fo,omitempty,omitzero"`
+
+ Br bool `json:"br"`
+ Bo bool `json:"bo,omitempty,omitzero"`
+
+ Ur uint `json:"ur"`
+ Uo uint `json:"uo,omitempty,omitzero"`
+
+ Str struct{} `json:"str"`
+ Sto struct{} `json:"sto,omitempty,omitzero"`
+
+ MyTime time.Time `json:"mytime,omitempty,omitzero"`
+}
+
+func TestOmitEmptyZero(t *testing.T) {
+ var want = `{
+ "sr": "",
+ "omitempty": 0,
+ "slr": null,
+ "mr": {},
+ "fr": 0,
+ "br": false,
+ "ur": 0,
+ "str": {}
+}`
+ var o OptionalsEmptyZero
+ o.Sw = "something"
+ o.SloNonNil = make([]string, 0)
+ o.Mr = map[string]any{}
+ o.Mo = map[string]any{}
+
+ got, err := MarshalIndent(&o, "", " ")
+ if err != nil {
+ t.Fatalf("MarshalIndent error: %v", err)
+ }
+ if got := string(got); got != want {
+ t.Errorf("MarshalIndent:\n\tgot: %s\n\twant: %s\n", indentNewlines(got), indentNewlines(want))
+ }
+}
+
type StringTag struct {
BoolStr bool `json:",string"`
IntStr int64 `json:",string"`
Change information
- A doc/next/6-stdlib/99-minor/encoding/json/45669.md
- M src/encoding/json/encode.go
- M src/encoding/json/encode_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. The commit message body is very brief. That can be OK if the change is trivial like correcting spelling or fixing a broken link, but usually the description should provide context for the change and explain what it does in complete sentences.
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 5 comments![]( "Open in Gerrit")
omitted if the field value is zero as determined by the `IsZero() bool` methodWhen marshaling, a struct field with the new `omitzero` option in the struct field tag will be omitted if its value is zero. If the field type has an `IsZero() bool` method, that will be used to determine whether the value is zero. Otherwise, the value is zero if it is [the zero value for its type](/ref/spec#The_zero_value).
This option has no effect when unmarshaling. If `omitempty` is specified togetherThe omitempty sentence should be a new paragraph.
with `omitzero`, whether a field is omitted is based on the logical OR of the two.If both omitempty and omitzero are specified, the field will be omitted if the value is either empty or zero (or both).
This will mean that `omitzero` of a slice omits a nil slice but emits [] for aI don't think we need this paragraph in the release notes. The release notes should point out the change; explanatory details belong in the regular documentation and/or examples.
// The "omitempty" option specifies that the field should be omittedThe new omitzero option needs to be documented up here.
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 6 comments![]( "Open in Gerrit")
omitted if the field value is zero as determined by the `IsZero() bool` methodWhen marshaling, a struct field with the new `omitzero` option in the struct field tag will be omitted if its value is zero. If the field type has an `IsZero() bool` method, that will be used to determine whether the value is zero. Otherwise, the value is zero if it is [the zero value for its type](/ref/spec#The_zero_value).
Done
This option has no effect when unmarshaling. If `omitempty` is specified togetherThe omitempty sentence should be a new paragraph.
Done
with `omitzero`, whether a field is omitted is based on the logical OR of the two.If both omitempty and omitzero are specified, the field will be omitted if the value is either empty or zero (or both).
Done
This will mean that `omitzero` of a slice omits a nil slice but emits [] for aI don't think we need this paragraph in the release notes. The release notes should point out the change; explanatory details belong in the regular documentation and/or examples.
Done
// The "omitempty" option specifies that the field should be omittedThe new omitzero option needs to be documented up here.
Done
// Examples of struct field tags and their meanings:These examples don't add much. They just repeat the omitempty examples. Let's omit them.
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Commit-Queue | +1 |
1 comment
// Examples of struct field tags and their meanings:These examples don't add much. They just repeat the omitempty examples. Let's omit them.
Done
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Auto-Submit | +1 |
| Code-Review | +2 |
| Commit-Queue | +1 |
Thanks.
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Tsai (Gerrit)
Joe Tsai added 1 comment![]( "Open in Gerrit")
func isZeroValue(v reflect.Value) bool {
if z, ok := v.Interface().(interface {
IsZero() bool
}); ok {
return z.IsZero()
}
return v.IsZero()
}
While this is nice, concise, and simple, it unfortunately doesn't handle a few edge cases in particular with nil interfaces, nil pointers, and addressability.
The exact implementation that the "github.com/go-json-experiment/json" uses is here:
https://github.com/go-json-experiment/json/blob/ebd3a8989ca1eadb7a68e02a93448ecbbab5900c/fields.go#L167-L184
It's open to debate whether we should call IsZero on nil pointers, but we decided not to do so to avoid a possible nil pointer panic, which could be fatal in JSON marshaling/unmarshaling. I can't imagine a use-case where an IsZero method reports false on a nil-pointer receiver. However, there exist implementations of IsZero defined on a pointer receiver that also forgot to check whether it is in.
Also, we should check whether the pointer version of T also implements IsZero. This would be similar to how MarshalJSON and MarshalText defined on (*T) is inconsistently called because they have struct field that is of type T. This is not people expect and a source of many bugs.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted![]( "Open in Gerrit")
| Auto-Submit | +0 |
| Code-Review | +0 |
| Commit-Queue | +1 |
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Tsai (Gerrit)
Joe Tsai added 1 comment![]( "Open in Gerrit")
func isZeroValue(v reflect.Value) bool {
if z, ok := v.Interface().(interface {
IsZero() bool
}); ok {
return z.IsZero()
}
return v.IsZero()
}
While this is nice, concise, and simple, it unfortunately doesn't handle a few edge cases in particular with nil interfaces, nil pointers, and addressability.
The exact implementation that the "github.com/go-json-experiment/json" uses is here:
https://github.com/go-json-experiment/json/blob/ebd3a8989ca1eadb7a68e02a93448ecbbab5900c/fields.go#L167-L184It's open to debate whether we should call IsZero on nil pointers, but we decided not to do so to avoid a possible nil pointer panic, which could be fatal in JSON marshaling/unmarshaling. I can't imagine a use-case where an IsZero method reports false on a nil-pointer receiver. However, there exist implementations of IsZero defined on a pointer receiver that also forgot to check whether it is in.
Also, we should check whether the pointer version of T also implements IsZero. This would be similar to how MarshalJSON and MarshalText defined on (*T) is inconsistently called because they have struct field that is of type T. This is not people expect and a source of many bugs.
I just did a static analysis of all open-source Go code of all `func (*T) IsZero() bool` methods and unfortunately it seems ~85% of them would panic if called on a nil *T, which is worse than I suspected, so that lends credibility to special-casing *T.
bcd a (Gerrit)
bcd a added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. The commit message body is very brief. That can be OK if the change is trivial like correcting spelling or fixing a broken link, but usually the description should provide context for the change and explain what it does in complete sentences.The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Acknowledged
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted Commit-Queue+1![]( "Open in Gerrit")
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted Commit-Queue+1![]( "Open in Gerrit")
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
bcd a (Gerrit)
bcd a added 1 comment![]( "Open in Gerrit")
func isZeroValue(v reflect.Value) bool {
if z, ok := v.Interface().(interface {
IsZero() bool
}); ok {
return z.IsZero()
}
return v.IsZero()
}
Joe TsaiWhile this is nice, concise, and simple, it unfortunately doesn't handle a few edge cases in particular with nil interfaces, nil pointers, and addressability.
The exact implementation that the "github.com/go-json-experiment/json" uses is here:
https://github.com/go-json-experiment/json/blob/ebd3a8989ca1eadb7a68e02a93448ecbbab5900c/fields.go#L167-L184It's open to debate whether we should call IsZero on nil pointers, but we decided not to do so to avoid a possible nil pointer panic, which could be fatal in JSON marshaling/unmarshaling. I can't imagine a use-case where an IsZero method reports false on a nil-pointer receiver. However, there exist implementations of IsZero defined on a pointer receiver that also forgot to check whether it is in.
Also, we should check whether the pointer version of T also implements IsZero. This would be similar to how MarshalJSON and MarshalText defined on (*T) is inconsistently called because they have struct field that is of type T. This is not people expect and a source of many bugs.
I just did a static analysis of all open-source Go code of all `func (*T) IsZero() bool` methods and unfortunately it seems ~85% of them would panic if called on a nil *T, which is worse than I suspected, so that lends credibility to special-casing *T.
Looks like the new patchset includes all these edge cases, could you please take a look? Thanks.
Related details
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ali Bayati
💐🙏
--
You received this message because you are subscribed to the Google Groups "golang-codereviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-coderevi...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-codereviews/d2307d2191fad7223037044e50b9823427989992-EmailReviewComments-HTML%40go-review.googlesource.com.
Joe Tsai (Gerrit)
Joe Tsai added 6 comments![]( "Open in Gerrit")
func isZeroValue(v reflect.Value) bool {We should move this logic into `typeFields`, which is executed exactly once for each type. Otherwise, we would be wasting runtime cycles checking type properties that can be derived ahead of time.
isZero = func() bool { return v.Addr().Interface().(isZeroer).IsZero() }There's an additional edge case here, we need to do something like:
```
if !v.CanAddr() {
// Temporarily box v so we can take the address.
v2 := reflect.New(v.Type()).Elem()
v2.Set(v)
v = v2
}
```
before we can blindly call `reflect.Value.Addr`.
The `go-json-experiment` module avoids the problem because it strictly enforces that all `reflect.Value` passed around are addressable at an architectural level. We can't benefit from that architectural invariant here, so we might need to call `reflect.New` if the value is not addressable.
// Do not remove or change the type signature.Oh yuck...
Aren't we technically in violation of this?
The `structFields` type references the `field` type, so we are indirectly changing the signature.
return nps.Int == 0Perhaps invert this? otherwise we can't tell if this was omitted because this is the zero value of `NoPanicStruct` or whether this method was called.
Could also test float64 with `-0` (and also a `[2]float64{+0, -0}`), which would be omitted since it is zero according to the Go language spec.
}Should test a `map[string]OptionalsZero` and we expect `NoPanicStruct3.IsZero` to be called even though `IsZero` is on a pointer receiver.
Related details
- Ian Lance Taylor
- Russ Cox
Joe Tsai (Gerrit)
Joe Tsai added 1 comment![]( "Open in Gerrit")
omitZero boolWe should also store an `isZero func(reflect.Value) bool` value.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
bcd a (Gerrit)
bcd a added 9 comments![]( "Open in Gerrit")
Thanks for reviewing, it would be great to know your thoughts on the new patchset.
We should move this logic into `typeFields`, which is executed exactly once for each type. Otherwise, we would be wasting runtime cycles checking type properties that can be derived ahead of time.
Done
func isZeroValue(v reflect.Value) bool {
if z, ok := v.Interface().(interface {
IsZero() bool
}); ok {
return z.IsZero()
}
return v.IsZero()
}
Joe TsaiWhile this is nice, concise, and simple, it unfortunately doesn't handle a few edge cases in particular with nil interfaces, nil pointers, and addressability.
The exact implementation that the "github.com/go-json-experiment/json" uses is here:
https://github.com/go-json-experiment/json/blob/ebd3a8989ca1eadb7a68e02a93448ecbbab5900c/fields.go#L167-L184It's open to debate whether we should call IsZero on nil pointers, but we decided not to do so to avoid a possible nil pointer panic, which could be fatal in JSON marshaling/unmarshaling. I can't imagine a use-case where an IsZero method reports false on a nil-pointer receiver. However, there exist implementations of IsZero defined on a pointer receiver that also forgot to check whether it is in.
Also, we should check whether the pointer version of T also implements IsZero. This would be similar to how MarshalJSON and MarshalText defined on (*T) is inconsistently called because they have struct field that is of type T. This is not people expect and a source of many bugs.
bcd aI just did a static analysis of all open-source Go code of all `func (*T) IsZero() bool` methods and unfortunately it seems ~85% of them would panic if called on a nil *T, which is worse than I suspected, so that lends credibility to special-casing *T.
Looks like the new patchset includes all these edge cases, could you please take a look? Thanks.
Done
isZero = func() bool { return v.Addr().Interface().(isZeroer).IsZero() }There's an additional edge case here, we need to do something like:
```
if !v.CanAddr() {
// Temporarily box v so we can take the address.
v2 := reflect.New(v.Type()).Elem()
v2.Set(v)
v = v2
}
```
before we can blindly call `reflect.Value.Addr`.The `go-json-experiment` module avoids the problem because it strictly enforces that all `reflect.Value` passed around are addressable at an architectural level. We can't benefit from that architectural invariant here, so we might need to call `reflect.New` if the value is not addressable.
Done
We should also store an `isZero func(reflect.Value) bool` value.
Done
Oh yuck...
Aren't we technically in violation of this?
The `structFields` type references the `field` type, so we are indirectly changing the signature.
Done
Perhaps invert this? otherwise we can't tell if this was omitted because this is the zero value of `NoPanicStruct` or whether this method was called.
Done
Could also test float64 with `-0` (and also a `[2]float64{+0, -0}`), which would be omitted since it is zero according to the Go language spec.
Done
Should test a `map[string]OptionalsZero` and we expect `NoPanicStruct3.IsZero` to be called even though `IsZero` is on a pointer receiver.
- Ian Lance Taylor
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
bcd a (Gerrit)
bcd a added 1 comment![]( "Open in Gerrit")
// Temporarily box v so we can take the address.To match this, I created this test, will mail later if it's the right approach.
```
type MyInt int
func (mi *MyInt) IsZero() bool {
return *mi != 0
}type OptionalsZero struct {
...
Map map[string]OptionalsZero `json:"map,omitzero"`
Mi MyInt `json:"Mi,omitzero"`
}
...o.Map = map[string]OptionalsZero{"foo": {}}
```
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
bcd a (Gerrit)
bcd a added 1 comment![]( "Open in Gerrit")
// Temporarily box v so we can take the address.To match this, I created this test, will mail later if it's the right approach.
```
type MyInt intfunc (mi *MyInt) IsZero() bool {
return *mi != 0
}type OptionalsZero struct {
...
Map map[string]OptionalsZero `json:"map,omitzero"`
Mi MyInt `json:"Mi,omitzero"`
}
...o.Map = map[string]OptionalsZero{"foo": {}}
```
Sorry, forgot`TestOmitZeroMap` covered this.
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
Joe Tsai (Gerrit)
Joe Tsai added 5 comments![]( "Open in Gerrit")
Thank you for working on this.
Code looks good. Just a few more possible test cases.
Mo map[string]any `json:",omitzero"`Should test a `map[string]any` with `omitzero` that is actually zero.
Time time.Time `json:"time,omitzero"`Should also test with `time.Time{}.Local()`, which is usually not equal to `time.Time{}`.
NoPanicStruct1 isZeroer `json:"nps1,omitzero"` // non-nil interface with non-nil pointerShould also test non-nil interface with nil pointer.
NoPanicStruct2 *NoPanicStruct `json:"nps2,omitzero"` // nil pointerShould also test with non-nil pointer.
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Ian Lance Taylor
- Ian Lance Taylor
- Joseph Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zxilly Chou (Gerrit)
Zxilly Chou added 3 comments![]( "Open in Gerrit")
field.isZero = func(v reflect.Value) bool {Maybe the function passed to isZero could be declared globally in advance to avoid duplicate creation?
field.isZero = func(v reflect.Value) bool {same here
field.isZero = func(v reflect.Value) bool {same here
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Joseph Tsai
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joseph Tsai
| Code-Review | +2 |
1 comment
field.isZero = func(v reflect.Value) bool {Maybe the function passed to isZero could be declared globally in advance to avoid duplicate creation?
If the anonymous function has no closed over variables, I'm fairly certain the compiler functionally treats this as no different than a global function. Assuming the compiler does what I think it is doing, it's cleaner to keep this here for a simple function since we can more readily see the condition triggering the function along with the function implementation in the same place.
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Russ Cox
- Zxilly Chou
Jes Cok (Gerrit)
Jes Cok added 4 comments![]( "Open in Gerrit")
Should test a `map[string]any` with `omitzero` that is actually zero.
Done
Should also test with `time.Time{}.Local()`, which is usually not equal to `time.Time{}`.
Done
NoPanicStruct1 isZeroer `json:"nps1,omitzero"` // non-nil interface with non-nil pointerShould also test non-nil interface with nil pointer.
Done
NoPanicStruct2 *NoPanicStruct `json:"nps2,omitzero"` // nil pointerShould also test with non-nil pointer.
- Ian Lance Taylor
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
- Zxilly Chou
Jes Cok (Gerrit)
Jes Cok voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Joseph Tsai (Gerrit)
Joseph Tsai added 1 comment![]( "Open in Gerrit")
Jes Cok (Gerrit)
Jes Cok added 3 comments![]( "Open in Gerrit")
field.isZero = func(v reflect.Value) bool {Joseph TsaiMaybe the function passed to isZero could be declared globally in advance to avoid duplicate creation?
If the anonymous function has no closed over variables, I'm fairly certain the compiler functionally treats this as no different than a global function. Assuming the compiler does what I think it is doing, it's cleaner to keep this here for a simple function since we can more readily see the condition triggering the function along with the function implementation in the same place.
Agree with Joe, let's leave this as is. If it's a performance issue, please provide something like benchmarks, thanks.
field.isZero = func(v reflect.Value) bool {Jes Coksame here
Acknowledged
field.isZero = func(v reflect.Value) bool {Jes Coksame here
Acknowledged
Related details
- Ian Lance Taylor
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
- Zxilly Chou
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Auto-Submit | +1 |
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
- Ian Lance Taylor
Michael Knyszek (Gerrit)
Michael Knyszek voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ian Lance Taylor
- Joe Tsai
- Russ Cox
- Zxilly Chou
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
encoding/json: add omitzero option
Fixes #45669
- A doc/next/6-stdlib/99-minor/encoding/json/45669.md
- M src/encoding/json/encode.go
- M src/encoding/json/encode_test.go
Code-Review: +2 by Joseph Tsai, +1 by Michael Knyszek, +1 by Ian Lance Taylor
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
bcd a (Gerrit)
bcd a added 1 comment![]( "Open in Gerrit")
#69857 is `xml` version of this option. Any chance to get that through before the freeze?
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
#69857 is `xml` version of this option. Any chance to get that through before the freeze?
It's on the list for the proposal committee, but it may not make it.