code review 4876046: exp/template: don't panic on range of nil interface

exp/template: don't panic on range of nil interface

This avoids a non-obvious panic when range is used on a
nil interface, and fixes it by behaving as if the range
was empty.

The new behavior is equivalent to the outcome of iterating
on a nil map or slice, and is useful because it allows
generic structures such as used in json (map[string]interface{})
to behave correctly if a key generally set to a list or map
isn't present.

[niemeyer, 2011-08-14 00:11:01]

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

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

[r, 2011-08-14 02:33:15]

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:237: break // Can't tell what the value might be if
set
better is to drop this check ....

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:239: s.errorf("range can't iterate over value of
type %T", val.Interface())
and change this to
s.errorf("range can't iterate over %v", val)
the print will be informative

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:863: if iv.kind == 0 {
surely iv.kind == Invalid

however, hold off on this because rsc is working on a related fix due to another
issue that arose.

[niemeyer, 2011-08-14 03:13:15]

Thanks for the review.  Some follow ups:

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:239: s.errorf("range can't iterate over value of
type %T", val.Interface())
This will yield a different result. The goal as stated in the CL description is
to make {{range $v}} not blow up if $v resolves to an interface that at the
moment is set to nil, because the interface may actually be set to a reasonable
map or slice in other instances.

That's a real case and not rare if the data is coming from a dynamic map with
optional keys (json, bson, yaml, ...).  Given that the proposed logic still
validates the value when the interface is indeed set, this should still be safe.

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:863: if iv.kind == 0 {
> surely iv.kind == Invalid

I've followed the local convention there.

> however, hold off on this because rsc is working

Ok.. please see how you feel about the rest. I'll be happy to separate that out
in another CL to avoid blocking the exp/template change.

[gustavo_niemeyer.net, 2011-08-14 03:32:18]

> This will yield a different result. The goal as stated in the CL
> description is to make {{range $v}} not blow up if $v resolves to an
> interface that at the moment is set to nil, because the interface may
> actually be set to a reasonable map or slice in other instances.
>
> That's a real case and not rare if the data is coming from a dynamic map
> with optional keys (json, bson, yaml, ...).  Given that the proposed
> logic still validates the value when the interface is indeed set, this
> should still be safe.

Just to help the conversation, here is a real case:

        {{range .tabs}}
        <li{{if .current}} id="current"{{end}}><a
href="{{.link}}">{{.name}}</a></li>
        {{else}}
        <li id="current"><a href="{{.link}}">{{.title}}</a></li>
        {{end}}

This is part of a template in the small server that backs
labix.org/{mgo,gobson,etc}.  If .tabs isn't set for a page, it should
fall to the else.  Instead, it blows up without this fix.  The type of
dot is map[string]interface{}, because it comes from an external
source.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-08-14 08:09:26]

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go
File src/pkg/exp/template/exec.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:237: break // Can't tell what the value might be if
set
// An invalid value is likely a nil map, etc. and acts like an empty map.

http://codereview.appspot.com/4876046/diff/7001/src/pkg/exp/template/exec.go#...
src/pkg/exp/template/exec.go:239: s.errorf("range can't iterate over value of
type %T", val.Interface())
yes i realized this while i was out. background thinking.
but please make the suggested change to the error message anyway.

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/4876046/diff/7001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:865: }
drop this change, at least for now. it's separable and doesn't affect the
functionality of the other.
also, as i said, rsc is in here.

[niemeyer, 2011-08-14 19:31:50]

> drop this change, at least for now. it's separable and doesn't affect the
> functionality of the other.
> also, as i said, rsc is in here.

Sounds good.  All tweaks made, and backed this out. I'll resend
this minor to rsc in a separate CL.

[niemeyer, 2011-08-14 19:32:17]

Hello golang-dev@googlegroups.com, r@golang.org, gustavo@niemeyer.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-08-15 00:52:56]

LGTM but update the CL description before submitting. it describes the reflect
change no longer here.

[niemeyer, 2011-08-15 03:22:44]

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

exp/template: don't panic on range of nil interface

This avoids a non-obvious panic when range is used on a
nil interface, and fixes it by behaving as if the range
was empty.

The new behavior is equivalent to the outcome of iterating
on a nil map or slice, and is useful because it allows
generic structures such as used in json (map[string]interface{})
to behave correctly if a key generally set to a list or map
isn't present.

R=golang-dev, r, gustavo
CC=golang-dev
http://codereview.appspot.com/4876046