code review 179125: Propagate error to the caller in json.Marshal. Fixes is...

Propagate error to the caller in json.Marshal. Fixes issue 445.

[Ivan Krasin, 2009-12-22 10:12:33]

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

I'd like you to review the following change.

[rsc, 2009-12-22 17:15:55]

Thanks for fixing this.
While you're here, let's allow interface{}
when it contains types that can be marshalled.
There's a one-line fix to struct.go
below, but please add a test for that case.

http://codereview.appspot.com/179125/diff/6/7
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/179125/diff/6/7#newcode406
src/pkg/json/struct.go:406: err = &MarshalError{val.Type()}
err = writeValue(w, v.Elem())

http://codereview.appspot.com/179125/diff/6/8
File src/pkg/json/struct_test.go (right):

http://codereview.appspot.com/179125/diff/6/8#newcode251
src/pkg/json/struct_test.go:251: type Issue445Val1 struct {
Issue445 won't mean anything to future
readers unless they go look up the issue.
it's like calling it type Type12345.
better to use a real name.

much simpler case:

type ChanVal struct {
    C chan int
}

http://codereview.appspot.com/179125/diff/6/8#newcode254
src/pkg/json/struct_test.go:254: type Issue445Val2 struct {
can drop this

http://codereview.appspot.com/179125/diff/6/8#newcode258
src/pkg/json/struct_test.go:258: type Issue445 struct {
no need for this struct.

http://codereview.appspot.com/179125/diff/6/8#newcode262
src/pkg/json/struct_test.go:262: func TestIssue445(t *testing.T) {
// Issue 445
func TestBadStructField(t *testing.T) {

http://codereview.appspot.com/179125/diff/6/8#newcode265
src/pkg/json/struct_test.go:265: obj.lookup = make(map[string]interface{})
much simpler test case:

var c ChanVal
err := Marshal(&buf, c)

http://codereview.appspot.com/179125/diff/6/8#newcode267
src/pkg/json/struct_test.go:267: obj.lookup["b"] = new(Issue445Val2)
can drop this

[Ivan Krasin, 2009-12-23 16:12:55]

http://codereview.appspot.com/179125/diff/6/7
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/179125/diff/6/7#newcode406
src/pkg/json/struct.go:406: err = &MarshalError{val.Type()}
On 2009/12/22 17:15:55, rsc wrote:
> err = writeValue(w, v.Elem())
> 
Done. Also I enabled handling *reflect.PtrValue and tests for it. I don't know
why it was disabled before (please, correct me if I did a mistake)

http://codereview.appspot.com/179125/diff/6/8
File src/pkg/json/struct_test.go (right):

http://codereview.appspot.com/179125/diff/6/8#newcode251
src/pkg/json/struct_test.go:251: type Issue445Val1 struct {
On 2009/12/22 17:15:55, rsc wrote:
> Issue445 won't mean anything to future
> readers unless they go look up the issue.
> it's like calling it type Type12345.
> better to use a real name.
> 
> much simpler case:
> 
> type ChanVal struct {
>     C chan int
> }
> 

Done. Moved the test to marshalErrorTests array

http://codereview.appspot.com/179125/diff/6/8#newcode254
src/pkg/json/struct_test.go:254: type Issue445Val2 struct {
On 2009/12/22 17:15:55, rsc wrote:
> can drop this
> 

Done.

http://codereview.appspot.com/179125/diff/6/8#newcode258
src/pkg/json/struct_test.go:258: type Issue445 struct {
On 2009/12/22 17:15:55, rsc wrote:
> no need for this struct.
> 

Done.

http://codereview.appspot.com/179125/diff/2001/3001
File src/pkg/json/struct.go (right):

http://codereview.appspot.com/179125/diff/2001/3001#newcode408
src/pkg/json/struct.go:408: case *reflect.PtrValue:
Probably, the compiler should be enhanced to able to compile this:
case *reflect.InterfaceValue,
        *reflect.PtrValue:
        if v.IsNil() {
                fmt.Fprint(w, "null")
        } else {
                err = writeValue(w, v.Elem())
        }
Now it suppose that v is reflect.Value and reflect.Value does not have IsNil()
and Elem()

Should I create an issue for this?

[rsc, 2009-12-23 16:48:28]

> Probably, the compiler should be enhanced to able to compile this:
> case *reflect.InterfaceValue,
>        *reflect.PtrValue:
>        if v.IsNil() {
>                fmt.Fprint(w, "null")
>        } else {
>                err = writeValue(w, v.Elem())
>        }
> Now it suppose that v is reflect.Value and reflect.Value does not have
> IsNil() and Elem()
>
> Should I create an issue for this?

Sure.
We've been meaning to talk about doing that but it's
pretty far down on the list.

[imkrasin, 2009-12-24 10:30:09]

On 2009/12/23 16:48:28, rsc wrote:
> > Probably, the compiler should be enhanced to able to compile this:
> > case *reflect.InterfaceValue,
> >        *reflect.PtrValue:
> >        if v.IsNil() {
> >                fmt.Fprint(w, "null")
> >        } else {
> >                err = writeValue(w,
v.Elem())
> >        }
> > Now it suppose that v is reflect.Value and reflect.Value does not have
> > IsNil() and Elem()
> >
> > Should I create an issue for this?
> 
> Sure.
> We've been meaning to talk about doing that but it's
> pretty far down on the list.
I have created an issue (feel free to mark it as Long Term). Anything else with
this CL? or it is ready for submit?

[rsc, 2010-01-05 22:16:37]

LGTM 

but you need to hg sync / hg upload before I can apply it.
One of the changes has already made it in.

[imkrasin, 2010-01-06 15:15:04]

Done. I think you could submit it.

On 2010/01/05 22:16:37, rsc wrote:
> LGTM 
> 
> but you need to hg sync / hg upload before I can apply it.
> One of the changes has already made it in.

[rsc, 2010-01-06 15:25:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=e29d95da0388 ***

Propagate error to the caller in json.Marshal. Fixes issue 445.

R=rsc, imkrasin
CC=golang-dev
http://codereview.appspot.com/179125

Committer: Russ Cox <rsc@golang.org>