Typed nil pointer in context results in panic about log15 HOT 11 CLOSED

My opinion is that this is a bug in the implementation of the String method on the value. The String method should return a valid string for a nil pointer. Calling a method on a nil pointer is perfectly acceptable in Go. A contrived example:

http://play.golang.org/p/hQS__z15cT

package main

import "fmt"

func main() {
    m1 := new(mutable)
    *m1 = 0

    for i := 0; i < 5; i++ {
        m1.add(2)
        fmt.Println(m1)
    }

    var m2 *mutable

    fmt.Println(m2)
}

type mutable int

func (m *mutable) add(v int) {
    if m != nil {
        *m += mutable(v)
    }
}

func (m *mutable) String() string {
    if m == nil {
        return "<nil>"
    }
    return fmt.Sprint(*m)
}

So I think the real question is: Should log15 take steps to not trigger these types of bugs in client code? I am not ready to answer that question yet.

from log15.

So for my String(), I've typically been using a value based receiver, primarily because all the snippets I've seen does this. So when I receive the panic, my String would be something like:

func (m mutable) String() string {
    return fmt.Sprint(m)
}

I did find that if I used a pointer based receiver AND I handled the nil case, it would work. I'm more than happy to switch over to do that. As a matter of curiosity, when it comes to String, is it better to do pointer v. value? Minus the reason of size. In the end it probably is better I use pointer based, since some of the structs are a little large.

from log15.

There is not one right answer for when to use a pointer or value based receiver. This question comes up often and the answer depends on the specifics of your code. Typically String() string should not be modifying the value, and typically the cost of formatting the data into a string will significantly outweigh the cost of copying the value. So in general I would favor a value receiver for String() string methods unless I had hard evidence that a pointer receiver was required.

from log15.

Getting back to your original problem. Having re-read your initial post more carefully I did not fully understand the issue. I see now that you have a type with a value based String() string method, but you are passing a nil pointer to such a value in the context.

I was curious what the fmt package does in this case: http://play.golang.org/p/SINT-YfkrK. It prints <nil>. But I have reproduced your report that log15 panics. I think we should do better than that and we should fix this in log15.

from log15.

I did a some research into how the fmt package handles this situation and prints <nil>. It recover()s the panic and only then uses the reflect package to check if the value is a nil pointer.

See https://github.com/golang/go/blob/go1.4.1/src/fmt/print.go#L704 and https://github.com/golang/go/blob/go1.4.1/src/fmt/print.go#L624 for the code.

from log15.

Good find Chris. It seems to me that if fmt.Printf supports that, we should support it as well.

from log15.

Cool, thanks for the info. Before I had submitted the issue I had found:

http://stackoverflow.com/questions/13476349/check-for-nil-and-nil-interface-in-go

And here is a thread with more on interfaces with nil value but type.

http://grokbase.com/t/gg/golang-nuts/1317818wx4/go-nuts-how-to-test-whether-an-interface-contains-a-nil-value-but-with-type

from log15.

@mobileben Thanks for the links, both are helpful.

from log15.

I've just pushed a fix for this issue.

@mobileben & @inconshreveable please review.

from log15.

LGTM

from log15.

Yep, looks good. I'll also undo my work around change in the code I found this issue with and confirm it there.

from log15.