fmt: surprising behavior of structs with embedded error

[Gunni]

Reported at the request of @FiloSottile after talking to him about it at #gopherconIS.

What version of Go are you using (go version)?

1.10.2 windows/amd64

Does this issue reproduce with the latest release?

I do not know, but i assume so since this is a syntax/behavior that i assume has not been changed.

What operating system and processor architecture are you using (go env)?

Windows
amd64
gopath is set to my configured one, everything else is default

What did you do?

https://play.golang.org/p/0BPsgof_9oI

https://play.golang.org/p/Z3hV6sMz9Ko (as minimal as it can be i think)

What did you expect to see?

I expected my stringer to be called and a string get generated

What did you see instead?

%!v(PANIC=runtime error: invalid memory address or nil pointer dereference)

Extra

Now, i was at a lecture at #gopherconIS while coding this and listening. So my full focus was not on debugging this, which is probably the reason it took me hours to figure out.

I was even using GoLand to debug it, but my breakpoints were useless at the time because my program was going from a fmt.Println(... directly to the Panic! thing... (also the debugger took forever to start, sometimes even giving me a connection refused error, but that's a different problem)

Now i assume most of you noticed my mistake, i did solve the problem eventually, i forgot to name the variables in my struct.

But the problem is that i did not understand why it is so important to name them until i met up with @FiloSottile at #gopherconIS. He explained to me that when i add these "naked" types to a struct, their functions actually join my struct (WTF?!?!?), in this case, Error() was suddenly a member of it, satisfying the only thing that Error interfaces need...

Solution

I don't know how to solve this, i assume "naked" types in structs are a desirable quality for something, but being a beginner at Go i have not been exposed to them.

You also don't do warnings, which is cool, maybe a linter can spot it?

But how many false positives would that have?

Maybe printArg() in $GOROOT/fmt/print.go could add a case in the default inside the giant switch, to not panic?

Or panic with a more descriptive error? (You fit the Error interface but that function is derpy, go fix your struct)

[cznic]

Considering the program does what language specification and the documentation of the fmt package say, I'm not sure what's surprising or magic here.

This snippet exhibits the same "problem" mechanism

type I interface{ m() }

func f() {
        var v I
        v.m() // <-- Compiles fine, but crash guaranteed here at runtime
}

playground

[Gunni]

@cznic the problem isn't the documentation, it's that the error given tells me about a nil pointer dereference, when i wasn't trying to use pointers, i expected it to return the value, and channels to ensure that only Result could be returned, and since i sent a copy of the struct, and not a pointer, the nil pointer dereference error just confused me.

Thing is, being a beginner at Go, this king of thing is suuper confusing, and that's what the Go panel at the conference mentioned being important to hear about.

[cznic]

... the problem isn't the documentation, it's that the error given tells me about a nil pointer dereference, when i wasn't trying to use pointers, ...

The error looks correct to me. Dispatching a method call via an interface value amounts to dereferencing a vtable pointer. So, yes the code does use pointers (a nil one in particular.)

Implementation details are discussed for example here.

I believe the (minimized) example in the report works as intended.

[Gunni]

Why not something like this? https://play.golang.org/p/Uo2pkHCorkK displaying a string form of nil as {<nil>} feels ok in my case.

[FiloSottile]

The code behavior can't be changed as it is indeed working as documented, but the combination of struct embedding and %s prioritizing Error() string is enough indirection to be confusing. How can we make the error message better and save the next developer some time? The first thing I can think of is %!v(PANIC=...) including information about what method it tried to call. Anyone has a better idea?

[martisch]

While adding additional info such as %!v(PANIC="runtime error: invalid memory address or nil pointer dereference", CALL=Result.Error) can give more clues where this happens it does not necessary mean the nil pointer deference was when dereferencing to call that specific error method but could have happened anywhere within the call. Which I think is also the case here since from a quick look the .(*Result).Error is autogenerated and then calls the Error method of the embedded field.

Supplying a call stack might be better for debugging but could be viewed as breaking backwards compatibility as existing tests and code may rely on the documented format of the PANIC string/message.

/cc @robpike

[robpike]

All it means is that fmt caught a panic in a method it called. I worry that trying to be more specific may mislead people as to the nature of the problem. The actual panic is recorded:

https://play.golang.org/p/nkyirMdK1DN

It might not be that the pointer is nil:

https://play.golang.org/p/NVndSFaGpvw

[FiloSottile]

How about %!v(recovered panic "runtime error: invalid memory address or nil pointer dereference" in (*Result).Error) / %!v(recovered panic "whatever" in (*Foo).String)? To me that does not suggest the panic happened in dereferencing to call the method, but just that it happened somewhere in the call. However it gives enough information (Error was called instead of String, Result has an Error method) to debug the issue without knowing the answer already.

(Edited s/caught/recovered)

[cznic]

The Error() method was never called. The nil dereference panic occurs a step earlier. The nil interface value has a nil itable pointer. That pointer is dereferenced to find out the address of the Error method.

[FiloSottile]

It was caused by an attempt to call Error(), the internals details are less important to help a developer figure out why a panic they are not expecting is happening.

[martisch]

Regardless of the final form of a potential new message in the event of a panic: Is more information in the panic string to make behavior with embedded methods more explicit worth breaking existing GO1 compatibility and possible programs that may depend on the documented syntax of %!verb(PANIC=panicstring)?

AFAIK in the past changes to fmt have been avoided to not break existing behavior (uint8 vs byte, length of %#06x vs %#6x ...).

[randall77]

From the fmt package:

If the panic is caused by a nil receiver to an Error or String method, however, the output is the undecorated string, "".

How does this not apply in this situation?

[martisch]

AFAIK (on phone not able to check asm) that only applies to the Error method that fmt calls which is the autogenerated Error for Result. Result is not nil but the embedded field. The call to the Error method of the embedded field than triggers the panic inside the sucessful call to the Result Error. The internal implementation of Result Error is not known to fmt.

I guess the documentation can be clearer to say this only applies to calls that fmt makes directly.

[Gunni]

I think @randall77 is onto something, the https://golang.org/pkg/fmt/ documentation literally states this exact situation!

If the panic is caused by a nil receiver to an Error or String method, 
however, the output is the undecorated string, "<nil>".

Edit: nevermind, that's just for Printf with %!.

[martisch]

I do not think the documentation was meant to imply that any panic due to any nil value anywhere down from the function that fmt called is captured by the statement as it would mean that <nil> is printed even for values that are not nil. e.g. if fmt calls (*Result).Error() where *Result is not nil and then somewhere in what Error does (not related to Result value directly) there is a panic then I would not expect fmt to print that Result formatted is <nil>. And as far as I understand Result is not nil in the example given but an embedded field is. Similar scenario just coded explicitly:
https://play.golang.org/p/0xBoa-PQiyh here Result is not nil but the panic happens to the call of Error of the embedded field.

The documentation could be changed to clarify that only for panics where the value was nil that fmt called an Error or String method on the reported format is <nil>.
Possible new version: "If a panic is caused when fmt calls an Error or String method with a nil value then the output is the undercoated string, <nil>."
(that should fit what the current implementation has been detecting https://play.golang.org/p/8tnL8BUoSBR)

The option that fmt would detect any nil pointer receiver panic down the call stack for other values would need stack traces for the panic and possibly code inspection for fmt to figure out what happend which sounds like a lot of added complexity which to me does not seem worth placing into fmt to solve the issue discussed.

[ianlancetaylor]

I think we can perhaps do a little better along the lines that @FiloSottile suggested above, by mentioning the method without significantly changing the output. CL 153827 changes the original program to print

%!v(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)

I'm certainly open to other approaches, but I think we should keep the %!v(PANIC=...) format.

[gopherbot]

Change https://golang.org/cl/153827 mentions this issue: fmt: include failing method name in panic message