rs / zerolog

Zero Allocation JSON Logger
MIT License
10.41k stars 567 forks source link

passing nil time to stringer panics #426

Closed nathanfranke closed 2 years ago

nathanfranke commented 2 years ago
package main

import (
    "os"
    "time"

    "github.com/rs/zerolog"
)

func main() {
    l := zerolog.New(os.Stderr)
    var t *time.Time = nil
    l.Error().Stringer("time", t).Msg("error") // panic
}
rs commented 2 years ago

This is expected.

nathanfranke commented 2 years ago

Should the documentation for Stringer be updated then?

Stringer adds the field key with val.String() (or null if val is nil) to the *Event context.
nathanfranke commented 2 years ago

Also, methods in libraries should not panic. The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

rs commented 2 years ago

It does not use panic(), you are sending a nil pointer, this is a programming error.

nathanfranke commented 2 years ago
Stringer adds the field key with val.String() (or null if val is nil) to the *Event context.
rs commented 2 years ago

If you send nil it won’t panic but that’s not what you are doing. See https://glucn.medium.com/golang-an-interface-holding-a-nil-value-is-not-nil-bb151f472cc7 for more context.