code review 5527046: text/template: handle panic values that are not errors.

text/template: handle panic values that are not errors.

The recover code assumes that the panic() argument was
an error, but it is usually a simple string.
Fixes issue 2663.

[remyoudompheng, 2012-01-07 08:47:13]

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

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

[r, 2012-01-09 06:20:01]

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

http://codereview.appspot.com/5527046/diff/1002/src/pkg/text/template/exec.go...
src/pkg/text/template/exec.go:78: // level of Parse.
this comment is wrong. my fault. it's Execute.

http://codereview.appspot.com/5527046/diff/1002/src/pkg/text/template/exec.go...
src/pkg/text/template/exec.go:85: switch err := e.(type) {
i believe this code is behaving as i intended, re-panicking for unexpected
things. you must have some case that's misbehaving.

if, for instance, a function called during execution indexes a slice out of
bounds, i believe this package shouldn't hide the panic.

[remyoudompheng, 2012-01-09 07:07:05]

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

http://codereview.appspot.com/5527046/diff/1002/src/pkg/text/template/exec.go...
src/pkg/text/template/exec.go:85: switch err := e.(type) {
On 2012/01/09 06:20:01, r wrote:
> i believe this code is behaving as i intended, re-panicking for unexpected
> things. you must have some case that's misbehaving.
> 
> if, for instance, a function called during execution indexes a slice out of
> bounds, i believe this package shouldn't hide the panic.

Thanks for the explanation. Then I'd like to replace the whole block with

switch err := e.(type) {
case runtime.Error:
  fallthrough
default:
  panic(e)
case error:
  *errp = err
}

otherwise if I try to recover from panic("Call of method on nil interface
value"), I only get the message "interface conversion: string is not error:
missing method Error" instead.

[r2, 2012-01-09 07:12:29]

Please do.

-rob

[remyoudompheng, 2012-01-09 20:10:07]

Hello golang-dev@googlegroups.com, r@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[r, 2012-01-09 20:42:29]

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

http://codereview.appspot.com/5527046/diff/1003/src/pkg/text/template/exec.go...
src/pkg/text/template/exec.go:88: }
switch err := e.(type) {
default:
  fallthrough
case runtime.Error:
  panic(e)
case error:
  *errp = err
}

[remyoudompheng, 2012-01-09 20:45:47]

On 2012/01/09 20:42:29, r wrote:
> http://codereview.appspot.com/5527046/diff/1003/src/pkg/text/template/exec.go
> File src/pkg/text/template/exec.go (right):
> 
>
http://codereview.appspot.com/5527046/diff/1003/src/pkg/text/template/exec.go...
> src/pkg/text/template/exec.go:88: }
> switch err := e.(type) {
> default:
>   fallthrough
> case runtime.Error:
>   panic(e)
> case error:
>   *errp = err
> }

Uh, compiler tells me 
  ./exec.go:83: cannot fallthrough in type switch

I was not aware of that. It doesn't seem to be explicitly written in the spec.
Do you still prefer "default:" to be at the top ? 

Rémy.

[gri, 2012-01-09 20:52:13]

FYI: The spec does restrict the use of "fallthrough" to expression switches:


On Mon, Jan 9, 2012 at 12:45 PM,  <remyoudompheng@gmail.com> wrote:
> I was not aware of that. It doesn't seem to be explicitly written in the
> spec. Do you still prefer "default:" to be at the top ?

[gri, 2012-01-09 20:52:32]

I meant to send the link as well:

http://golang.org/doc/go_spec.html#Fallthrough_statements

- gri

On Mon, Jan 9, 2012 at 12:52 PM, Robert Griesemer <gri@golang.org> wrote:
> FYI: The spec does restrict the use of "fallthrough" to expression switches:
>
>
> On Mon, Jan 9, 2012 at 12:45 PM,  <remyoudompheng@gmail.com> wrote:
>> I was not aware of that. It doesn't seem to be explicitly written in the
>> spec. Do you still prefer "default:" to be at the top ?

[r2, 2012-01-09 20:53:27]

On Jan 9, 2012, at 12:45 PM, remyoudompheng@gmail.com wrote:

> On 2012/01/09 20:42:29, r wrote:
> 
> http://codereview.appspot.com/5527046/diff/1003/src/pkg/text/template/exec.go
>> File src/pkg/text/template/exec.go (right):
> 
> 
>
http://codereview.appspot.com/5527046/diff/1003/src/pkg/text/template/exec.go...
>> src/pkg/text/template/exec.go:88: }
>> switch err := e.(type) {
>> default:
>>   fallthrough
>> case runtime.Error:
>>   panic(e)
>> case error:
>>   *errp = err
>> }
> 
> Uh, compiler tells me
>  ./exec.go:83: cannot fallthrough in type switch
> 
> I was not aware of that. It doesn't seem to be explicitly written in the
> spec. Do you still prefer "default:" to be at the top ?
> 
> Rémy.
> 
> http://codereview.appspot.com/5527046/

no. it's fine. LGTM as is. i didn't try my code.

if it's not in the spec, that's an oversight.

-rob

[r, 2012-01-09 21:04:51]

*** Submitted as 6ff6aceb5f4b ***

text/template: handle panic values that are not errors.

The recover code assumes that the panic() argument was
an error, but it is usually a simple string.
Fixes issue 2663.

R=golang-dev, r, r, gri
CC=golang-dev, remy
http://codereview.appspot.com/5527046

Committer: Rob Pike <r@golang.org>