code review 6459056: text/template: add 'nil' as a keyword in the language

text/template: add 'nil' as a keyword in the language
The keyword reprents an untyped nil and is useful for
passing nil values to methods and functions. The
nil will be promoted to the appropriate type when
used; if a type cannot be assigned, an error results.

[r, 2012-08-07 22:51:50]

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

I'd like you to review this change to
https://code.google.com/p/go/

[dsymonds, 2012-08-08 00:23:12]

LGTM

[rsc, 2012-08-08 15:08:00]

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go
File src/pkg/text/template/exec.go (right):

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:331: s.errorf("cannot assign type to untyped nil")
It looks like evalCommand is called to evaluate one command in a pipeline, and
only for that, in which case the error might be better as

s.errorf("nil is not a command")

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:524: if canBeNil(typ) {
One question is whether it should ever be possible to end up with the zero
(invalid) reflect.Value in the midst of execution.

There are two ways that can happen (that I can see). First, the original
argument to the template's Execute method might be a nil interface value, which
comes down as (interface{})(nil), and then reflect.ValueOf(that) returns the
zero reflect.Value. Second, there is a line above that pulls an element out of
an interface unconditionally, even if it is nil. If a function returns
(interface{})(nil), then the code above, marked '// lovely!', is holding a
non-zero reflect.Value with value.Type() == the type for interface{} and
value.IsNil(), but then the value = reflect.ValueOf(value.Interface()) turns it
into the zero reflect.Value.

This means that if you have these functions:
// func table
"f1": func() interface{} { return nil }
"f2": func(x *int) string { return "f2 ran" }

Right now you can have a template {{f2 .}} and invoke it as t.Execute(w, nil)
and f2 gets passed a (*int)(nil) even though that's not what got passed in.
Similarly, you can have a template {{f1 | f2}} even though the equivalent in Go
would not be allowed.

The code here is enabling those two cases.

I think it would be reasonable to disallow them, by using

if value.Kind() == reflect.Interface && value.Type().NumMethod() == 0 &&
!value.IsNil() {
    value = value.Elem() // more lovely!
}

above, and then calling s.errorf unconditionally here. 

The typed nil node then becomes the only way to generate an 'ideal nil', and
that's handled in only one place, evalArg.

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:651: switch n := n.(type) {
Do you need a NilNode case here?

[r, 2012-08-08 16:53:45]

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

Please take another look.

[r, 2012-08-08 16:56:32]

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go
File src/pkg/text/template/exec.go (right):

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:331: s.errorf("cannot assign type to untyped nil")
On 2012/08/08 15:08:00, rsc wrote:
> It looks like evalCommand is called to evaluate one command in a pipeline, and
> only for that, in which case the error might be better as
> 
> s.errorf("nil is not a command")
> 

Done.

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:524: if canBeNil(typ) {
I tried this and it broke a bunch of tests involving things like map lookups
that fail, since they produce zero reflect.Values that look like nils:


exec_test.go:569: 	.Method3(nil value): unexpected execute error: template:
.Method3(nil value):1: invalid value; expected interface {}
exec_test.go:569: 	if map not unset: unexpected execute error: template: if map
not unset:1: invalid value; expected interface {}

http://codereview.appspot.com/6459056/diff/1/src/pkg/text/template/exec.go#ne...
src/pkg/text/template/exec.go:651: switch n := n.(type) {
On 2012/08/08 15:08:00, rsc wrote:
> Do you need a NilNode case here?

no, but i added one anyway and explained why it's unnecessary.

[rsc, 2012-08-08 18:48:47]

LGTM

[r, 2012-08-09 03:02:31]

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

text/template: add 'nil' as a keyword in the language
The keyword reprents an untyped nil and is useful for
passing nil values to methods and functions. The
nil will be promoted to the appropriate type when
used; if a type cannot be assigned, an error results.

R=rsc, dsymonds
CC=golang-dev
http://codereview.appspot.com/6459056