[go] encoding/json, encoding/xml: 'omitempty' for empty structs

[Joe Shaw (Gerrit)]

Joe Shaw uploaded a change:
https://go-review.googlesource.com/13914

encoding/json, encoding/xml: 'omitempty' for empty structs

Empty structs (such as time.Time) that are marked omitempty are omitted
when marshaling to JSON and XML.

For #11939
For #4357
Fixes #10648

Change-Id: Ife16dda4ed808df4e927914369646b7ef37154da
---
M src/encoding/json/encode.go
M src/encoding/json/encode_test.go
M src/encoding/xml/marshal.go
M src/encoding/xml/marshal_test.go
4 files changed, 44 insertions(+), 24 deletions(-)



diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go
index 90782de..00c3ad7 100644
--- a/src/encoding/json/encode.go
+++ b/src/encoding/json/encode.go
@@ -54,7 +54,7 @@
// becomes a member of the object unless
// - the field's tag is "-", or
// - the field is empty and its tag specifies the "omitempty" option.
-// The empty values are false, 0, any
+// The empty values are false, 0, an empty struct, any
// nil pointer or interface value, and any array, slice, map, or string of
// length zero. The object's default key string is the struct field name
// but can be specified in the struct field's tag value. The "json" key in
@@ -289,6 +289,8 @@
return v.Float() == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
+ case reflect.Struct:
+ return reflect.Zero(v.Type()).Interface() == v.Interface()
}
return false
}
diff --git a/src/encoding/json/encode_test.go
b/src/encoding/json/encode_test.go
index 7abfa85..1f9ecdb 100644
--- a/src/encoding/json/encode_test.go
+++ b/src/encoding/json/encode_test.go
@@ -37,6 +37,14 @@

Str struct{} `json:"str"`
Sto struct{} `json:"sto,omitempty"`
+
+ // non-nil pointers
+ Psr *struct{} `json:"psr"`
+ Pso *struct{} `json:"pso,omitempty"`
+
+ // nil pointers
+ Nsr *struct{} `json:"nsr"`
+ Nso *struct{} `json:"nso,omitempty"`
}

var optionalsExpected = `{
@@ -48,7 +56,9 @@
"br": false,
"ur": 0,
"str": {},
- "sto": {}
+ "psr": {},
+ "pso": {},
+ "nsr": null
}`

func TestOmitEmpty(t *testing.T) {
@@ -56,6 +66,8 @@
o.Sw = "something"
o.Mr = map[string]interface{}{}
o.Mo = map[string]interface{}{}
+ o.Psr = new(struct{})
+ o.Pso = new(struct{})

got, err := MarshalIndent(&o, "", " ")
if err != nil {
diff --git a/src/encoding/xml/marshal.go b/src/encoding/xml/marshal.go
index 86d1422..d66761d 100644
--- a/src/encoding/xml/marshal.go
+++ b/src/encoding/xml/marshal.go
@@ -54,7 +54,8 @@
// subject to the usual marshalling procedure. It must not contain
// the "--" string within it.
// - a field with a tag including the "omitempty" option is omitted
-// if the field value is empty. The empty values are false, 0, any
+// if the field value is empty. The empty values are false, 0, an
+// empty struct, any
// nil pointer or interface value, and any array, slice, map, or
// string of length zero.
// - an anonymous struct field is handled as if the fields of its
@@ -757,13 +758,14 @@
if finfo.flags&fAttr != 0 {
continue
}
- vf := finfo.value(val)
+ ovf := finfo.value(val)

// Dereference or skip nil pointer, interface values.
- switch vf.Kind() {
+ vf := ovf
+ switch ovf.Kind() {
case reflect.Ptr, reflect.Interface:
- if !vf.IsNil() {
- vf = vf.Elem()
+ if !ovf.IsNil() {
+ vf = ovf.Elem()
}
}

@@ -880,7 +882,7 @@
}
}
}
- if err := p.marshalValue(vf, finfo, nil); err != nil {
+ if err := p.marshalValue(ovf, finfo, nil); err != nil {
return err
}
}
@@ -984,6 +986,8 @@
return v.Float() == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
+ case reflect.Struct:
+ return reflect.Zero(v.Type()).Interface() == v.Interface()
}
return false
}
diff --git a/src/encoding/xml/marshal_test.go
b/src/encoding/xml/marshal_test.go
index 66675d7..9b66a53 100644
--- a/src/encoding/xml/marshal_test.go
+++ b/src/encoding/xml/marshal_test.go
@@ -204,14 +204,15 @@
}

type OmitFieldTest struct {
- Int int `xml:",omitempty"`
- Named int `xml:"int,omitempty"`
- Float float64 `xml:",omitempty"`
- Uint8 uint8 `xml:",omitempty"`
- Bool bool `xml:",omitempty"`
- Str string `xml:",omitempty"`
- Bytes []byte `xml:",omitempty"`
- Ptr *PresenceTest `xml:",omitempty"`
+ Int int `xml:",omitempty"`
+ Named int `xml:"int,omitempty"`
+ Float float64 `xml:",omitempty"`
+ Uint8 uint8 `xml:",omitempty"`
+ Bool bool `xml:",omitempty"`
+ Str string `xml:",omitempty"`
+ Bytes []byte `xml:",omitempty"`
+ Ptr *PresenceTest `xml:",omitempty"`
+ Struct PresenceTest `xml:",omitempty"`
}

type AnyTest struct {
@@ -838,14 +839,15 @@
// omitempty on fields
{
Value: &OmitFieldTest{
- Int: 8,
- Named: 9,
- Float: 23.5,
- Uint8: 255,
- Bool: true,
- Str: "str",
- Bytes: []byte("byt"),
- Ptr: &PresenceTest{},
+ Int: 8,
+ Named: 9,
+ Float: 23.5,
+ Uint8: 255,
+ Bool: true,
+ Str: "str",
+ Bytes: []byte("byt"),
+ Ptr: &PresenceTest{},
+ Struct: PresenceTest{},
},
ExpectXML: `<OmitFieldTest>` +
`<Int>8</Int>` +

--
https://go-review.googlesource.com/13914

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

R=rsc

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-HasComments: No

[David Symonds (Gerrit)]

David Symonds has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

I'd much rather see this check for an IsZero method and delegate to that,
which works for time.Time but also permits any other type to work with
this, not just structs.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: David Symonds <dsym...@golang.org>

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

> I'd much rather see this check for an IsZero method and delegate to
> that, which works for time.Time but also permits any other type to
> work with this, not just structs.

That's the second part of the proposal in #11939, but I haven't submitted a
CL for that yet. I'll do that tomorrow. There is some backward
compatibility considerations with that, but I'm not sure if it's a
problem. I was hoping for some opinions in the issue from the core
committers about that.

Worst case we could do that only in the case of structs, which up to this
point have not supported omitempty at all, and don't have backward
compatibility concerns.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>

Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

> > I'd much rather see this check for an IsZero method and delegate
> to
> > that, which works for time.Time but also permits any other type
> to
> > work with this, not just structs.
>
> That's the second part of the proposal in #11939, but I haven't
> submitted a CL for that yet. I'll do that tomorrow. There is some
> backward compatibility considerations with that, but I'm not sure
> if it's a problem. I was hoping for some opinions in the issue
> from the core committers about that.
>
> Worst case we could do that only in the case of structs, which up
> to this point have not supported omitempty at all, and don't have
> backward compatibility concerns.

https://go-review.googlesource.com/13977 introduces the encoding.IsZeroer
interface.

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

(3 comments)

https://go-review.googlesource.com/#/c/13914/1/src/encoding/json/encode.go
File src/encoding/json/encode.go:

Line 57: // The empty values are false, 0, an empty struct, any
"an empty comparable struct" I think?


Line 293: return reflect.Zero(v.Type()).Interface() == v.Interface()
this explodes at runtime if the struct contains a map or slice, though?


https://go-review.googlesource.com/#/c/13914/1/src/encoding/json/encode_test.go
File src/encoding/json/encode_test.go:

Line 70: o.Pso = new(struct{})
add a test somewhere (can be its own Test) that omitempty doesn't explode
on a struct with a slice or map in it (some non-comparable type)

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>
Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/13914/1/src/encoding/json/encode.go
File src/encoding/json/encode.go:

Line 293: return reflect.Zero(v.Type()).Interface() == v.Interface()
> this explodes at runtime if the struct contains a map or slice, though?

It does, thanks.

I'll update it to use reflect.DeepEqual() instead. This is what
encoding/asn1 does.

[Joe Shaw (Gerrit)]

Joe Shaw uploaded a new patch set:

https://go-review.googlesource.com/13914

encoding/json, encoding/xml: 'omitempty' for empty structs

Empty structs (such as time.Time) that are marked omitempty are omitted
when marshaling to JSON and XML.

For #11939
For #4357
Fixes #10648

Change-Id: Ife16dda4ed808df4e927914369646b7ef37154da
---
M src/encoding/json/encode.go
M src/encoding/json/encode_test.go
M src/encoding/xml/marshal.go
M src/encoding/xml/marshal_test.go

4 files changed, 73 insertions(+), 24 deletions(-)

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 2:

I'd really like to stop adding to these packages. I think we need to leave
well enough alone at some point.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>
Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 2:

This is a pretty big usability improvement, in my opinion. time.Time is
used a lot with encoding/json and many programs do not handle the
serialized zero values well. Every program either needs to define a new
type, or use pointers as a workaround, making the code uglier.

This bug keeps getting filed over and over again (see #4357 and its many
dups over the years) because it doesn't match user expectation or match up
with other types.

I'm fine with abandoning the IsZeroer thing (#13977), though.

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 2:

CL 13977, I mean.

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for empty structs

Patch Set 2:

(1 comment)

I appreciate the difficulty with zeroed structs and json/xml, especially
around not being able to mark a time.Time omitempty.

The problem is that there is also difficulty in defining which structs
should be omitted and which should not. The mistake in the description
identified below (zero vs empty) is exactly the point.

Is it really just zeroed structs that should be omitted? Or all structs
containing nothing but empty fields? Or only those structs that - due to
unexported fields or themselves having omitempty or ignore on fields of
their own - would JSON-format as {}? Or something even more subtle? I don't
know the answer here.

I kind of like the elegance and directness of "omitempty for structs means
don't emit it if it would encode as {}". The implementation is easy: encode
it and then backspace over it if it comes out empty. But that doesn't solve
the problem for types implementing json.Marshaler, like time.Time.

https://go-review.googlesource.com/#/c/13914/2/src/encoding/json/encode.go
File src/encoding/json/encode.go:

Line 60: // The empty values are false, 0, an empty struct, any
The terminology here is wrong.
The empty struct is a struct with no fields (empty of fields) aka struct{}.
What this CL implements is "a zeroed struct".

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>
Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Joe Shaw (Gerrit)]

Joe Shaw uploaded a new patch set:

https://go-review.googlesource.com/13914

encoding/json, encoding/xml: 'omitempty' for zeroed structs

Zeroed structs (such as time.Time) that are marked omitempty are omitted

when marshaling to JSON and XML.

For #11939
For #4357
Fixes #10648

Change-Id: Ife16dda4ed808df4e927914369646b7ef37154da
---
M src/encoding/json/encode.go
M src/encoding/json/encode_test.go
M src/encoding/xml/marshal.go
M src/encoding/xml/marshal_test.go

4 files changed, 72 insertions(+), 23 deletions(-)

[Joe Shaw (Gerrit)]

Joe Shaw has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for zeroed structs

Patch Set 2:

(1 comment)

New patchset fixes up the terminology.

It's true that this doesn't cover the case of unexported fields. I
initially felt that the tradeoff of the simplicity of the implementation
outweighed the fact that it didn't cover all cases, but maybe that's not
the right thing to do.

Perhaps a better way is to recursively call isEmptyValue() on each
exported, non-ignored field in the struct. That wouldn't take omitempty
into account for structs deeper in the hierarchy though.

I like the idea of omitting things that encode as "{}" but not covering
time.Time is an issue. MarshalJSON could accept an io.EOF-like error value
indicating if a value is omittable and time.Time's implementation would
return it if IsZero() returned true, but that feels gross to me.

https://go-review.googlesource.com/#/c/13914/2/src/encoding/json/encode.go
File src/encoding/json/encode.go:

Line 60: // The empty values are false, 0, an empty struct, any
> The terminology here is wrong.

I fixed this up in the following patchset.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>
Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for zeroed structs

Patch Set 3:

R=go1.7

I'm sorry but we didn't get to this in time for Go 1.6. Stability in the
runtime took more time than expected. Postponing to Go 1.7.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>
Gerrit-Reviewer: Joe Shaw <j...@joeshaw.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Caleb Spare (Gerrit)]

Caleb Spare has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for zeroed structs

Patch Set 3:

> R=go1.7
>
> I'm sorry but we didn't get to this in time for Go 1.6. Stability
> in the runtime took more time than expected. Postponing to Go 1.7.

Ping

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Caleb Spare <ces...@gmail.com>

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

encoding/json, encoding/xml: 'omitempty' for zeroed structs

Patch Set 3:

R=go1.8

Sorry, looks like this got dropped again. It's a semantic change so I
don't think we can put it into 1.7 at this point.

--
https://go-review.googlesource.com/13914
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Caleb Spare <ces...@gmail.com>
Gerrit-Reviewer: David Symonds <dsym...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>