code review 13839045: encoding/json: don't cache value addressability when bu...

encoding/json: don't cache value addressability when building first encoder

newTypeEncoder (called once per type and then cached) was
looking at the first value seen of that type's addressability
and caching the encoder decision.  If the first value seen was
addressable and a future one wasn't, it would panic.

Instead, introduce a new wrapper encoder type that checks
CanAddr at runtime.

Fixes Issue 6458

[bradfitz, 2013-09-23 22:26:20]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2013-09-23 22:59:05]

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:361: // if allowAddr is false, the encoder
returned won't check CanAddr.
// The returned encoder only checks CanAddr when allowAddr is true.

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:373: vx = reflect.New(t).Elem() // ensure vx is
addressable
I am very confused about this allocation of a value you throw away.
If you need to check whether the type implements Marshaler, use t.Implements.

In fact perhaps vx should not be an argument to this function at all.

[bradfitz, 2013-09-23 23:41:08]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-09-23 23:43:27]

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:361: // if allowAddr is false, the encoder
returned won't check CanAddr.
On 2013/09/23 22:59:05, rsc wrote:
> // The returned encoder only checks CanAddr when allowAddr is true.

Done.

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:373: vx = reflect.New(t).Elem() // ensure vx is
addressable
On 2013/09/23 22:59:05, rsc wrote:
> I am very confused about this allocation of a value you throw away.
> If you need to check whether the type implements Marshaler, use t.Implements.
> 
> In fact perhaps vx should not be an argument to this function at all.

Done.

https://codereview.appspot.com/13839045/diff/3001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:620: vxf := fieldByIndex(vx, f.index)
Btw, I tried to simplify this further, to just:

for i, f := range fields {
     se.fieldEncs[i] = typeEncoder(f.typ)
}

But that blows up at runtime.  I haven't figured out yet why yet.

Makes me slightly worried, though.

[bradfitz, 2013-09-24 00:24:59]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-09-24 00:25:40]

PTAL

Simplified newStructEncoder now too.

Please pay the most attention to that part (and its new typeByIndex).



On Mon, Sep 23, 2013 at 5:24 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/13839045/<https://codereview.appspot.com/138...
>

[rsc, 2013-09-24 02:39:59]

looking more closely at the loop at 620...

https://codereview.appspot.com/13839045/diff/4/src/pkg/encoding/json/encode_t...
File src/pkg/encoding/json/encode_test.go (right):

https://codereview.appspot.com/13839045/diff/4/src/pkg/encoding/json/encode_t...
src/pkg/encoding/json/encode_test.go:420: t.Errorf("Got %s; want %s", got, want)
check these separately. and use the usual Encode(x) = %#q, want %#q

[rsc, 2013-09-24 02:42:26]

LGTM

I see you figured out the vxf.IsValid thing.

[bradfitz, 2013-09-24 02:53:50]

Yeah, I'm much happier with the struct encoder now, and happy to see the vx
"example" values gone.

https://codereview.appspot.com/13839045/diff/4/src/pkg/encoding/json/encode_t...
File src/pkg/encoding/json/encode_test.go (right):

https://codereview.appspot.com/13839045/diff/4/src/pkg/encoding/json/encode_t...
src/pkg/encoding/json/encode_test.go:420: t.Errorf("Got %s; want %s", got, want)
On 2013/09/24 02:40:00, rsc wrote:
> check these separately. and use the usual Encode(x) = %#q, want %#q

Done.

[bradfitz, 2013-09-24 02:57:23]

*** Submitted as https://code.google.com/p/go/source/detail?r=411ac795f84e ***

encoding/json: don't cache value addressability when building first encoder

newTypeEncoder (called once per type and then cached) was
looking at the first value seen of that type's addressability
and caching the encoder decision.  If the first value seen was
addressable and a future one wasn't, it would panic.

Instead, introduce a new wrapper encoder type that checks
CanAddr at runtime.

Fixes Issue 6458

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/13839045