code review 5486076: fmt: don't recur if String method (etc.) misbehaves

fmt: don't recur if String method (etc.) misbehaves

Fixes issue 2555.

[r, 2011-12-14 23:07:11]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2011-12-14 23:50:14]

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

http://codereview.appspot.com/5486076/diff/1/src/pkg/fmt/fmt_test.go#newcode830
src/pkg/fmt/fmt_test.go:830: return Sprintf("recur@%p value: %d", r, r.i)
am I missing something? How does this recur? I didn't think %p would call
String().

[r2, 2011-12-15 00:04:45]

On Dec 14, 2011, at 3:50 PM, dsymonds@golang.org wrote:

> 
> http://codereview.appspot.com/5486076/diff/1/src/pkg/fmt/fmt_test.go
> File src/pkg/fmt/fmt_test.go (right):
> 
>
http://codereview.appspot.com/5486076/diff/1/src/pkg/fmt/fmt_test.go#newcode830
> src/pkg/fmt/fmt_test.go:830: return Sprintf("recur@%p value: %d", r,
> r.i)
> am I missing something? How does this recur? I didn't think %p would
> call String().

It is subtle.

The %p verb is not legal for a non-pointer, so it tries to print %!p(value). To
print value, it in effect does Print(value), which causes the String method to
be called, which uses %p, and we're back at the beginning. The fix is to avoid
the user's methods if we're in the error state.

-rob

[r2, 2011-12-15 00:09:52]

P.S. A test failure before the fix generated:

recur@%!p(fmt_test.Recur=recur@%!p(fmt_test.Recur=recur@%!p(fmt_test.Recur=re...
stack too large)) value: 3) value: 3) value: 3) value: 3) value: 3) value: 3)
value: 3) value: 3) value: 3) value: 3), value: 3


-rob

[dsymonds, 2011-12-15 00:26:00]

LGTM

On Thu, Dec 15, 2011 at 11:04 AM, Rob 'Commander' Pike <r@google.com> wrote:

> On Dec 14, 2011, at 3:50 PM, dsymonds@golang.org wrote:
>
>>
>> http://codereview.appspot.com/5486076/diff/1/src/pkg/fmt/fmt_test.go
>> File src/pkg/fmt/fmt_test.go (right):
>>
>>
http://codereview.appspot.com/5486076/diff/1/src/pkg/fmt/fmt_test.go#newcode830
>> src/pkg/fmt/fmt_test.go:830: return Sprintf("recur@%p value: %d", r,
>> r.i)
>> am I missing something? How does this recur? I didn't think %p would
>> call String().
>
> It is subtle.
>
> The %p verb is not legal for a non-pointer, so it tries to print %!p(value).
To print value, it in effect does Print(value), which causes the String method
to be called, which uses %p, and we're back at the beginning. The fix is to
avoid the user's methods if we're in the error state.

Aah, okay. Makes sense, but it probably deserves a comment in the test.

[r, 2011-12-15 00:32:42]

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

Please take another look.

[dsymonds, 2011-12-15 00:35:50]

LGTM

[r, 2011-12-15 00:37:58]

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

fmt: don't recur if String method (etc.) misbehaves

Fixes issue 2555.

R=golang-dev, dsymonds, r
CC=golang-dev
http://codereview.appspot.com/5486076