fmt: formatting of reflect.Value items inconsistent with documentation

[nerdatmath]

Please answer these questions before submitting your issue. Thanks!

1. What version of Go are you using (go version)?
   1.6.2
2. What operating system and processor architecture are you using (go env)?
   playground
3. What did you do?
   If possible, provide a recipe for reproducing the error.
   A complete runnable program is good.
   A link on play.golang.org is best.

From sbkim in https://groups.google.com/d/topic/golang-nuts/3DofSQIIF2w/discussion:

https://golang.org/pkg/fmt/ says:

1. If the operand is a reflect.Value, the operand is replaced by the concrete value that it holds, and printing continues with the next rule.
2. If an operand implements method String() string, that method will be invoked to convert the object to a string, which will then be formatted as required by the verb (if any).

So, in the following program (https://play.golang.org/p/V4H-vkjoE5):

package main

import (
 "fmt"
 "reflect"
)

type SpecialInt int

func (i SpecialInt) String() string { return fmt.Sprintf("SpecialInt(%d)", int(i)) }

func main() {
 v := reflect.ValueOf(SpecialInt(73))
 fmt.Printf("%+v\n", v)
 fmt.Printf("%+v\n", v.Interface())
}

I expect the first fmt.Printf call to substitute the concrete value of v, which is SpecialInt(73), invoke the String() method on it, and print "SpecialInt(73)". However, it just prints "73". On the contrary, the second fmt.Printf call (correctly) prints "SpecialInt(73)".

Is there anything I'm missing here, or maybe something wrong with the Go documentation or implementation?

1. What did you expect to see?
   SpecialInt(73)
   SpecialInt(73)
2. What did you see instead?
   73
   SpecialInt(73)

[robpike]

There is a trivial fix to this that solves the test case but won't work in general. The code says

p.printValue(f, verb, 0)

We could instead call

  p.PrintArg(f.Interface(), verb)

which would extract the interface and print that. This works but only if the value being printed was not acquired by using Value.Field of an unexported field.

We could write

if f.CanInterface() {
        p.printValue(f, verb, 0)
} else {
        p.printArg(f, verb)
}

and maybe that's the best we can do. Another possibility, maybe the right one, is to tweak the if at the top of printValue to know that the handle-methods check hasn't been done yet.

Needs thought.

[adonovan]

I think the depth > 0 condition in printValue is a relic of a previous design, and was perhaps just an optimization. It causes the handleMethods check for a reflect.Value argument to Printf to be skipped. Removing the condition fixes the problem (and the fmt tests pass).

 func (p *pp) printValue(value reflect.Value, verb rune, depth int) {
        // Handle values with special methods if not already handled by printArg (depth == 0).
        if depth > 0 && value.IsValid() && value.CanInterface() {

[martisch]

Added the depth check when refactoring reflect value printing https://go-review.googlesource.com/#/c/20923/.

As far as i recall currently:
It preserves behavior and performance of 1.6 in 1.7 and is an optimization to call handlemethods in printArg at depth 0 instead of in printValue always.

If only the depth check is removed handleMethods is called twice for the default branch of printArg.
If the handleMethods call in printArg is removed to prevent the above there is a hit
in performance for e.g. simple Stringer printing (did a quick benchmark to reconfirm my old testing):

name               old time/op  new time/op  delta
SprintfStringer-4   281ns ±10%   311ns ±20%  +10.98%  (p=0.000 n=18+19)

So the depth check addresses the CL comment about "slows down a very common operation" and to not slow down printValue for depth 0 either.

I will take a look for go 1.8 how we can ideally keep the performance for Stringer and still call handleMethods consistently once for reflect values.

[quentinmit]

@martisch Have you had a chance to look at this?

[martisch]

Thanks for bringing this back to my attention. Was on vacation in september and sidetracked by some other patches. (I also was not able to assign myself to golang issues) I will do some measurements again and send a proposal CL with tests within a week so we have this fixed for 1.8.

[gopherbot]

CL https://golang.org/cl/30753 mentions this issue.