Repository: go-logr/logr (go)

Issue:
Make the zero value useful
While updating [go-tlsconfig](https://github.com/PowerDNS/go-tlsconfig) to logr v1, I encountered an issue with how zero values of Logger are handled. These currently cause a panic when a method is called on them.

In go-tlsconfig a logr.Logger is an option that could be passed like this (pre-v1 code):

```go
type Options struct {
        ...
	Logr     logr.Logger
}

func New(ctx context.Context, opt Options) (*Foo, error) {
        if log == nil {
		log = SomeDefaultLogger()
	}
        ...
}
```

The caller can omit the Logr option if it is not interested in logging or has no preference for a logger, depending on the component.

Since v1 made the Logger a concrete struct instead of an interface, it is no longer possible to set or compare it to `nil`. If the caller omits the Logr, any log call will cause a panic.

The component also has no clean way to check if it received a zero value. It is possible to check  if `log.GetSink() == nil`, but this is a bit messy and easy to forget.

### Proposed solution

These problems can be avoided by making the zero value of a Logger useful, which is considered a good practise for Go values anyway. This makes the Logger safer and more convenient to use. Calling methods on a zero value would be equivalent to calling them on a Discard logger.

Additionally, a new `IsZero()` function would allow testing for the zero value so that a default logger can be assigned, if desired. This is similar to `Time.IsZero()` in the standard library.

The Options struct in the example above would remain the same and the New function would become:

```
func New(ctx context.Context, opt Options) (*Foo, error) {
        if log.IsZero() {
		log = SomeDefaultLogger()
	}
        ...
}
```

If the component is not interested in logging when a zero Logger is provided, it can still safely use that zero value and no longer needs to explicitly assign a Discard logger.

The Discard logger would remain as is and not be converted to a zero value. This allows a caller to explicitly disable logging without it would being overridden with some component-specific default logger.

I posted a PR with this change (#153).

### Alternatives

Changing the option to `*logr.Logger` is also undesirable, because this would require:

- the caller to explicitly take the address of a logger like `&log` to pass it as an option.
- the component to dereference the pointer like `*log` to use and pass it as a value.







Pull request:
Make zero value useful & add .IsZero()
To accommodate optional loggers, make the zero value of a logger useful and not trigger panics when its methods are called.

Additionally, this adds `Logger.IsZero()` to check for zero values.

Closes #152 


Patch:
diff --git a/funcr/funcr.go b/funcr/funcr.go
index 7accdb0..99dfe42 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -37,6 +37,7 @@ package funcr
 import (
 	"bytes"
 	"encoding"
+	"encoding/json"
 	"fmt"
 	"path/filepath"
 	"reflect"
@@ -500,6 +501,20 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s
 		}
 		return buf.String()
 	case reflect.Slice, reflect.Array:
+		// If this is outputing as JSON make sure this isn't really a json.RawMessage.
+		// If so just emit "as-is" and don't pretty it as that will just print
+		// it as [X,Y,Z,...] which isn't terribly useful vs the string form you really want.
+		if f.outputFormat == outputJSON {
+			if rm, ok := value.(json.RawMessage); ok {
+				// If it's empty make sure we emit an empty value as the array style would below.
+				if len(rm) > 0 {
+					buf.Write(rm)
+				} else {
+					buf.WriteString("null")
+				}
+				return buf.String()
+			}
+		}
 		buf.WriteByte('[')
 		for i := 0; i < v.Len(); i++ {
 			if i > 0 {
diff --git a/logr.go b/logr.go
index 25c2884..f720386 100644
--- a/logr.go
+++ b/logr.go
@@ -244,7 +244,7 @@ type Logger struct {
 // Enabled tests whether this Logger is enabled.  For example, commandline
 // flags might be used to set the logging verbosity and disable some info logs.
 func (l Logger) Enabled() bool {
-	return l.sink.Enabled(l.level)
+	return l.sink != nil && l.sink.Enabled(l.level)
 }
 
 // Info logs a non-error message with the given key/value pairs as context.
@@ -273,6 +273,9 @@ func (l Logger) Info(msg string, keysAndValues ...interface{}) {
 // triggered this log line, if present. The err parameter is optional
 // and nil may be passed instead of an error instance.
 func (l Logger) Error(err error, msg string, keysAndValues ...interface{}) {
+	if l.sink == nil {
+		return
+	}
 	if withHelper, ok := l.sink.(CallStackHelperLogSink); ok {
 		withHelper.GetCallStackHelper()()
 	}
@@ -294,6 +297,9 @@ func (l Logger) V(level int) Logger {
 // WithValues returns a new Logger instance with additional key/value pairs.
 // See Info for documentation on how key/value pairs work.
 func (l Logger) WithValues(keysAndValues ...interface{}) Logger {
+	if l.sink == nil {
+		return l
+	}
 	l.setSink(l.sink.WithValues(keysAndValues...))
 	return l
 }
@@ -304,6 +310,9 @@ func (l Logger) WithValues(keysAndValues ...interface{}) Logger {
 // contain only letters, digits, and hyphens (see the package documentation for
 // more information).
 func (l Logger) WithName(name string) Logger {
+	if l.sink == nil {
+		return l
+	}
 	l.setSink(l.sink.WithName(name))
 	return l
 }
@@ -357,6 +366,11 @@ func (l Logger) WithCallStackHelper() (func(), Logger) {
 	return helper, l
 }
 
+// IsZero returns true if this logger is an uninitialized zero value
+func (l Logger) IsZero() bool {
+	return l.sink == nil
+}
+
 // contextKey is how we find Loggers in a context.Context.
 type contextKey struct{}
 


Test patch:
diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go
index b888fe4..20b26a5 100644
--- a/funcr/funcr_test.go
+++ b/funcr/funcr_test.go
@@ -230,6 +230,10 @@ type Tembedjsontags struct {
 	Tinner6 `json:"inner6,omitempty"`
 }
 
+type Trawjson struct {
+	Message json.RawMessage `json:"message"`
+}
+
 func TestPretty(t *testing.T) {
 	// used below
 	newStr := func(s string) *string {
@@ -669,6 +673,15 @@ func makeKV(args ...interface{}) []interface{} {
 }
 
 func TestRender(t *testing.T) {
+	// used below
+	raw := &Trawjson{}
+	marshal := &TjsontagsInt{}
+	var err error
+	raw.Message, err = json.Marshal(marshal)
+	if err != nil {
+		t.Fatalf("json.Marshal error: %v", err)
+	}
+
 	testCases := []struct {
 		name       string
 		builtins   []interface{}
@@ -738,6 +751,21 @@ func TestRender(t *testing.T) {
 		}{"arg", 789}, "val"),
 		expectKV:   `"<non-string-key: {"F1":"builtin",>"="val" "<non-string-key: {\"F1\":\"value\",\"F>"="val" "<non-string-key: {\"F1\":\"arg\",\"F2\">"="val"`,
 		expectJSON: `{"<non-string-key: {"F1":"builtin",>":"val","<non-string-key: {\"F1\":\"value\",\"F>":"val","<non-string-key: {\"F1\":\"arg\",\"F2\">":"val"}`,
+	}, {
+		name:       "json rendering with json.RawMessage",
+		args:       makeKV("key", raw),
+		expectKV:   `"key"={"message":[123,34,105,110,116,49,34,58,48,44,34,45,34,58,48,44,34,73,110,116,53,34,58,48,125]}`,
+		expectJSON: `{"key":{"message":{"int1":0,"-":0,"Int5":0}}}`,
+	}, {
+		name:       "byte array not json.RawMessage",
+		args:       makeKV("key", []byte{1, 2, 3, 4}),
+		expectKV:   `"key"=[1,2,3,4]`,
+		expectJSON: `{"key":[1,2,3,4]}`,
+	}, {
+		name:       "json rendering with empty json.RawMessage",
+		args:       makeKV("key", &Trawjson{}),
+		expectKV:   `"key"={"message":[]}`,
+		expectJSON: `{"key":{"message":null}}`,
 	}}
 
 	for _, tc := range testCases {
diff --git a/logr_test.go b/logr_test.go
index 3f63818..3ca0c7e 100644
--- a/logr_test.go
+++ b/logr_test.go
@@ -18,6 +18,7 @@ package logr
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
@@ -400,3 +401,42 @@ func TestContext(t *testing.T) {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }
+
+func TestIsZero(t *testing.T) {
+	var l Logger
+	if !l.IsZero() {
+		t.Errorf("expected IsZero")
+	}
+	sink := &testLogSink{}
+	l = New(sink)
+	if l.IsZero() {
+		t.Errorf("expected not IsZero")
+	}
+	// Discard is not considered a zero unset logger, but an intentional choice
+	// to ignore messages that should not be overridden by a component.
+	l = Discard()
+	if l.IsZero() {
+		t.Errorf("expected not IsZero")
+	}
+}
+
+func TestZeroValue(t *testing.T) {
+	// Make sure that the zero value is useful and equivalent to a Discard logger.
+	var l Logger
+	if l.Enabled() {
+		t.Errorf("expected not Enabled")
+	}
+	if !l.IsZero() {
+		t.Errorf("expected IsZero")
+	}
+	// Make sure that none of these methods cause a crash
+	l.Info("foo")
+	l.Error(errors.New("bar"), "some error")
+	if l.GetSink() != nil {
+		t.Errorf("expected nil from GetSink")
+	}
+	l2 := l.WithName("some-name").V(2).WithValues("foo", 1).WithCallDepth(1)
+	l2.Info("foo")
+	l2.Error(errors.New("bar"), "some error")
+	_, _ = l.WithCallStackHelper()
+}