code review 4640043: fmt: catch panics from calls to String etc.

fmt: catch panics from calls to String etc.

This change causes Print et al. to catch panics generated by
calls to String, GoString, and Format.  The panic is formatted
into the output stream as an error, but the program continues.
As a special case, if the argument was a nil pointer, the
result is just "<nil>", because that's almost certainly enough
information and handles the very common case of String
methods that don't guard against nil.

Scan does not want this change. Input must work; output can
be for debugging and it's nice to get output even when you
make a mistake.

[r, 2011-06-17 04:24:14]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2011-06-17 04:39:24]

LGTM

Works for me.

[r2, 2011-06-17 10:20:57]

On 17/06/2011, at 2:39 PM, David Symonds wrote:

> LGTM
> 
> Works for me.

You're not a stern enough critic, but I'm starting to warm to this myself, which
may mean I'm not a stern enough critic.

But as a UI (not API), this feels right to me after a day mulling it over.

-rob

[adg, 2011-06-17 13:25:22]

On 17 June 2011 20:20, Rob 'Commander' Pike <r@google.com> wrote:
> You're not a stern enough critic, but I'm starting to warm to this myself,
which may mean I'm not a stern enough critic.
>
> But as a UI (not API), this feels right to me after a day mulling it over.

At first I thought you were going to do some reflection on every
Stringer, which I thought would be crazy overhead. Now that I see it's
done by catching the panic, it looks pretty nice to me.

I'm curious to see what the performance impact of defer is, here.

Andrew

[gri, 2011-06-17 16:24:34]

I think this looks good, pending tests and documentation updates (if necessary).

So if I understand this correctly, a fmt call should never crash anymore: either
the arguments are not proper and we get an error print, or fmt invokes a
Stringer.String() which may crash, which gets caught and then also leads to an
error print. Is that correct?

- Robert

http://codereview.appspot.com/4640043/diff/1/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/4640043/diff/1/src/pkg/fmt/print.go#newcode584
src/pkg/fmt/print.go:584: p.buf.WriteByte('=')
the '=' could go into the panicBytes
(this asymmetry has precedent, irparenBytes contains a closing ')' for instance)

[rsc, 2011-06-17 16:44:12]

LGTM

s/v.Type().Kind()/v.Kind()/

[gri, 2011-06-17 17:00:05]

I tried this out with a slightly modified example (division by 0 in
String()), for the output:

&{pending %v(PANIC=runtime error: integer divide by zero)}

which seems pretty nice; however one looses the detailed stack trace.
I have some "UI" questions here:

1) Is it ok to lose that stack trace in these cases? (Most of the
time, the respective print call is clear, but if there's many, it
might be tedious to locate which one it is. Also, if the culprit
String() method is complex, one may need to call it explicitly (to get
the trace) and find the offensive statement inside that method.

2) Would it make sense to re-throw the panic upon exit from fmt? Or
perhaps have a special printing format that does not catch internal
panics?

We may need to gather experience with this code first.
- Robert


On Fri, Jun 17, 2011 at 9:24 AM,  <gri@golang.org> wrote:
> I think this looks good, pending tests and documentation updates (if
> necessary).
>
> So if I understand this correctly, a fmt call should never crash
> anymore: either the arguments are not proper and we get an error print,
> or fmt invokes a Stringer.String() which may crash, which gets caught
> and then also leads to an error print. Is that correct?
>
> - Robert
>
>
> http://codereview.appspot.com/4640043/diff/1/src/pkg/fmt/print.go
> File src/pkg/fmt/print.go (right):
>
> http://codereview.appspot.com/4640043/diff/1/src/pkg/fmt/print.go#newcode584
> src/pkg/fmt/print.go:584: p.buf.WriteByte('=')
> the '=' could go into the panicBytes
> (this asymmetry has precedent, irparenBytes contains a closing ')' for
> instance)
>
> http://codereview.appspot.com/4640043/
>

[rsc, 2011-06-17 17:23:05]

fmt's job is to print.
We already squash all kinds of other errors too.
I think squashing the panic is in line with that.

Russ

[r2, 2011-06-17 22:03:41]

right. that's what i decided after mulling it over yesterday. the UI
is that fmt will not crash if you make a mistake in your String
function. the output won't be pretty but it will be informative (if
not quite so much) and debugging can continue.  if you want to see the
traceback, call String() yourself with the value.

now, i don't print the value. i could, by falling through and not
using the String method, but i'd like to try this simpler thing first.
i'm pretty sure that it'll feel like a huge improvement.

i'll finish this up today and send it out for proper review.

-rob

[r, 2011-06-17 23:04:12]

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

Please take another look.

[r2, 2011-06-20 21:27:18]

can someone review this please?

-rob

[gri1, 2011-06-20 21:42:23]

I'm ok with the idea.

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/fmt_test.go
File src/pkg/fmt/fmt_test.go (right):

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/fmt_test.go#newc...
src/pkg/fmt/fmt_test.go:692: var _ Stringer = Panic{}
I assume this is just to verify you got the types right? Please add comment
accordingly, this is confusing at first.

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/fmt_test.go#newc...
src/pkg/fmt/fmt_test.go:710: var _ Formatter = PanicF{}
same here, short comment, please

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/fmt_test.go#newc...
src/pkg/fmt/fmt_test.go:723: {"%d", (*Panic)(nil), "<nil>"}, // nil pointer
special case
shouldn't these three be %s ?

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/4640043/diff/14001/src/pkg/fmt/print.go#newcode584
src/pkg/fmt/print.go:584: p.buf.WriteByte('=')
append = to panicBytes and you don't need this WriteByte call here (there's
precedent for this, see mapBytes contains the opening '[')

[r, 2011-06-20 22:02:33]

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

Please take another look.

[rsc, 2011-06-20 22:14:41]

LGTM

i think as is if you fmt.Println(d, d) it will only catch the first panic

http://codereview.appspot.com/4640043/diff/15003/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/4640043/diff/15003/src/pkg/fmt/print.go#newcode592
src/pkg/fmt/print.go:592: p.buf.WriteByte(')')
a
p.panicking = false
?

[r, 2011-06-20 22:31:11]

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

fmt: catch panics from calls to String etc.

This change causes Print et al. to catch panics generated by
calls to String, GoString, and Format.  The panic is formatted
into the output stream as an error, but the program continues.
As a special case, if the argument was a nil pointer, the
result is just "<nil>", because that's almost certainly enough
information and handles the very common case of String
methods that don't guard against nil.

Scan does not want this change. Input must work; output can
be for debugging and it's nice to get output even when you
make a mistake.

R=dsymonds, r, adg, gri, rsc, gri
CC=golang-dev
http://codereview.appspot.com/4640043