lint: Enable errcheck, fix failures

.golangci.yml

@@ -9,7 +9,7 @@ linters:
   disable-all: true
   enable:
     # golangci-lint defaults:
-    # - errcheck TODO: enable errcheck
+    - errcheck
     - gosimple
     - govet
     - ineffassign
@@ -21,7 +21,7 @@ linters:
     - nolintlint # lints nolint directives
     - revive
 
-linter-settings:
+linters-settings:
   govet:
     # These govet checks are disabled by default, but they're useful.
     enable:
@@ -30,6 +30,24 @@ linter-settings:
       - sortslice
       - unusedwrite
 
+  errcheck:
+    exclude-functions:
+      # These methods can not fail.
+      # They operate on an in-memory buffer.
+      - (*go.uber.org/zap/buffer.Buffer).Write
+      - (*go.uber.org/zap/buffer.Buffer).WriteByte
+      - (*go.uber.org/zap/buffer.Buffer).WriteString
+
+      - (*go.uber.org/zap/zapio.Writer).Close
+      - (*go.uber.org/zap/zapio.Writer).Sync
+      - (*go.uber.org/zap/zapio.Writer).Write
+      # Write to zapio.Writer cannot fail,
+      # so io.WriteString on it cannot fail.
+      - io.WriteString(*go.uber.org/zap/zapio.Writer)
+
+      # Writing a plain string to a fmt.State cannot fail.
+      - io.WriteString(fmt.State)
+
 issues:
   # Print all issues reported by all linters.
   max-issues-per-linter: 0
@@ -51,3 +69,9 @@ issues:
     #   for foo() { }
     - linters: [revive]
       text: 'empty-block: this block is empty, you can remove it'
+
+    # Ignore logger.Sync() errcheck failures in example_test.go
+    # since those are intended to be uncomplicated examples.
+    - linters: [errcheck]
+      path: example_test.go
+      text: 'Error return value of `logger.Sync` is not checked'

benchmarks/scenario_bench_test.go

@@ -104,7 +104,6 @@ func BenchmarkDisabledWithoutFields(b *testing.B) {
 			}
 		})
 	})
-
 }
 
 func BenchmarkDisabledAccumulatedContext(b *testing.B) {
@@ -183,7 +182,6 @@ func BenchmarkDisabledAccumulatedContext(b *testing.B) {
 			}
 		})
 	})
-
 }
 
 func BenchmarkDisabledAddingFields(b *testing.B) {
@@ -253,7 +251,6 @@ func BenchmarkDisabledAddingFields(b *testing.B) {
 			}
 		})
 	})
-
 }
 
 func BenchmarkWithoutFields(b *testing.B) {
@@ -323,7 +320,9 @@ func BenchmarkWithoutFields(b *testing.B) {
 		b.ResetTimer()
 		b.RunParallel(func(pb *testing.PB) {
 			for pb.Next() {
-				logger.Log(getMessage(0), getMessage(1))
+				if err := logger.Log(getMessage(0), getMessage(1)); err != nil {
+					b.Fatal(err)
+				}
 			}
 		})
 	})
@@ -401,7 +400,6 @@ func BenchmarkWithoutFields(b *testing.B) {
 			}
 		})
 	})
-
 }
 
 func BenchmarkAccumulatedContext(b *testing.B) {
@@ -471,7 +469,9 @@ func BenchmarkAccumulatedContext(b *testing.B) {
 		b.ResetTimer()
 		b.RunParallel(func(pb *testing.PB) {
 			for pb.Next() {
-				logger.Log(getMessage(0), getMessage(1))
+				if err := logger.Log(getMessage(0), getMessage(1)); err != nil {
+					b.Fatal(err)
+				}
 			}
 		})
 	})
@@ -531,7 +531,6 @@ func BenchmarkAccumulatedContext(b *testing.B) {
 			}
 		})
 	})
-
 }
 
 func BenchmarkAddingFields(b *testing.B) {
@@ -592,7 +591,9 @@ func BenchmarkAddingFields(b *testing.B) {
 		b.ResetTimer()
 		b.RunParallel(func(pb *testing.PB) {
 			for pb.Next() {
-				logger.Log(fakeSugarFields()...)
+				if err := logger.Log(fakeSugarFields()...); err != nil {
+					b.Fatal(err)
+				}
 			}
 		})
 	})
@@ -643,5 +644,4 @@ func BenchmarkAddingFields(b *testing.B) {
 			}
 		})
 	})
-
 }

encoder_test.go

@@ -41,7 +41,7 @@ func TestRegisterEncoder(t *testing.T) {
 
 func TestDuplicateRegisterEncoder(t *testing.T) {
 	testEncoders(func() {
-		RegisterEncoder("foo", newNilEncoder)
+		assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo")
 		assert.Error(t, RegisterEncoder("foo", newNilEncoder), "expected an error when registering an encoder with the same name twice")
 	})
 }
@@ -52,7 +52,7 @@ func TestRegisterEncoderNoName(t *testing.T) {
 
 func TestNewEncoder(t *testing.T) {
 	testEncoders(func() {
-		RegisterEncoder("foo", newNilEncoder)
+		assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo")
 		encoder, err := newEncoder("foo", zapcore.EncoderConfig{})
 		assert.NoError(t, err, "could not create an encoder for the registered name foo")
 		assert.Nil(t, encoder, "the encoder from newNilEncoder is not nil")

error.go

@@ -61,9 +61,12 @@ func (errs errArray) MarshalLogArray(arr zapcore.ArrayEncoder) error {
 		// allocating, pool the wrapper type.
 		elem := _errArrayElemPool.Get()
 		elem.error = errs[i]
-		arr.AppendObject(elem)
+		err := arr.AppendObject(elem)
 		elem.error = nil
 		_errArrayElemPool.Put(elem)
+		if err != nil {
+			return err
+		}
 	}
 	return nil
 }

error_test.go

@@ -95,3 +95,39 @@ func TestErrorsArraysHandleRichErrors(t *testing.T) {
 	require.True(t, ok, "Expected serialized error to be a map, got %T.", serialized)
 	assert.Equal(t, "egad", errMap["error"], "Unexpected standard error string.")
 }
+
+func TestErrArrayBrokenEncoder(t *testing.T) {
+	t.Parallel()
+
+	failWith := errors.New("great sadness")
+	err := (brokenArrayObjectEncoder{
+		Err:           failWith,
+		ObjectEncoder: zapcore.NewMapObjectEncoder(),
+	}).AddArray("errors", errArray{
+		errors.New("foo"),
+		errors.New("bar"),
+	})
+	require.Error(t, err, "Expected error from broken encoder.")
+	assert.ErrorIs(t, err, failWith, "Unexpected error.")
+}
+
+// brokenArrayObjectEncoder is an ObjectEncoder
+// that builds a broken ArrayEncoder.
+type brokenArrayObjectEncoder struct {
+	zapcore.ObjectEncoder
+	zapcore.ArrayEncoder
+
+	Err error // error to return
+}
+
+func (enc brokenArrayObjectEncoder) AddArray(key string, marshaler zapcore.ArrayMarshaler) error {
+	return enc.ObjectEncoder.AddArray(key,
+		zapcore.ArrayMarshalerFunc(func(ae zapcore.ArrayEncoder) error {
+			enc.ArrayEncoder = ae
+			return marshaler.MarshalLogArray(enc)
+		}))
+}
+
+func (enc brokenArrayObjectEncoder) AppendObject(zapcore.ObjectMarshaler) error {
+	return enc.Err
+}

http_handler.go

@@ -69,6 +69,13 @@ import (
 //
 //	curl -X PUT localhost:8080/log/level -H "Content-Type: application/json" -d '{"level":"debug"}'
 func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if err := lvl.serveHTTP(w, r); err != nil {
+		w.WriteHeader(http.StatusInternalServerError)
+		fmt.Fprintf(w, "internal error: %v", err)
+	}
+}
+
+func (lvl AtomicLevel) serveHTTP(w http.ResponseWriter, r *http.Request) error {
 	type errorResponse struct {
 		Error string `json:"error"`
 	}
@@ -80,19 +87,20 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	switch r.Method {
 	case http.MethodGet:
-		enc.Encode(payload{Level: lvl.Level()})
+		return enc.Encode(payload{Level: lvl.Level()})
+
 	case http.MethodPut:
 		requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r)
 		if err != nil {
 			w.WriteHeader(http.StatusBadRequest)
-			enc.Encode(errorResponse{Error: err.Error()})
-			return
+			return enc.Encode(errorResponse{Error: err.Error()})
 		}
 		lvl.SetLevel(requestedLvl)
-		enc.Encode(payload{Level: lvl.Level()})
+		return enc.Encode(payload{Level: lvl.Level()})
+
 	default:
 		w.WriteHeader(http.StatusMethodNotAllowed)
-		enc.Encode(errorResponse{
+		return enc.Encode(errorResponse{
 			Error: "Only GET and PUT are supported.",
 		})
 	}
@@ -129,5 +137,4 @@ func decodePutJSON(body io.Reader) (zapcore.Level, error) {
 		return 0, errors.New("must specify logging level")
 	}
 	return *pld.Level, nil
-
 }

http_handler_test.go

@@ -22,6 +22,7 @@ package zap_test
 
 import (
 	"encoding/json"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -167,7 +168,9 @@ func TestAtomicLevelServeHTTP(t *testing.T) {
 
 			res, err := http.DefaultClient.Do(req)
 			require.NoError(t, err, "Error making %s request.", req.Method)
-			defer res.Body.Close()
+			defer func() {
+				assert.NoError(t, res.Body.Close(), "Error closing response body.")
+			}()
 
 			require.Equal(t, tt.expectedCode, res.StatusCode, "Unexpected status code.")
 			if tt.expectedCode != http.StatusOK {
@@ -188,3 +191,27 @@ func TestAtomicLevelServeHTTP(t *testing.T) {
 		})
 	}
 }
+
+func TestAtomicLevelServeHTTPBrokenWriter(t *testing.T) {
+	t.Parallel()
+
+	lvl := zap.NewAtomicLevel()
+
+	request, err := http.NewRequest(http.MethodGet, "http://localhost:1234/log/level", nil)
+	require.NoError(t, err, "Error constructing request.")
+
+	recorder := httptest.NewRecorder()
+	lvl.ServeHTTP(&brokenHTTPResponseWriter{
+		ResponseWriter: recorder,
+	}, request)
+
+	assert.Equal(t, http.StatusInternalServerError, recorder.Code, "Unexpected status code.")
+}
+
+type brokenHTTPResponseWriter struct {
+	http.ResponseWriter
+}
+
+func (w *brokenHTTPResponseWriter) Write([]byte) (int, error) {
+	return 0, errors.New("great sadness")
+}

logger.go

@@ -374,7 +374,7 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
 	if stack.Count() == 0 {
 		if log.addCaller {
 			fmt.Fprintf(log.errorOutput, "%v Logger.check error: failed to get caller\n", ent.Time.UTC())
-			log.errorOutput.Sync()
+			_ = log.errorOutput.Sync()
 		}
 		return ce
 	}

sink.go

@@ -66,7 +66,8 @@ func newSinkRegistry() *sinkRegistry {
 		factories: make(map[string]func(*url.URL) (Sink, error)),
 		openFile:  os.OpenFile,
 	}
-	sr.RegisterSink(schemeFile, sr.newFileSinkFromURL)
+	// Infallible operation: the registry is empty, so we can't have a conflict.
+	_ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL)
 	return sr
 }
 
@@ -154,7 +155,7 @@ func (sr *sinkRegistry) newFileSinkFromPath(path string) (Sink, error) {
 	case "stderr":
 		return nopCloserSink{os.Stderr}, nil
 	}
-	return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0666)
+	return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
 }
 
 func normalizeScheme(s string) (string, error) {

stacktrace_ext_test.go

@@ -97,7 +97,7 @@ func TestStacktraceFiltersVendorZap(t *testing.T) {
 
 		testDir := filepath.Join(goPath, "src/go.uber.org/zap_test/")
 		vendorDir := filepath.Join(testDir, "vendor")
-		require.NoError(t, os.MkdirAll(testDir, 0777), "Failed to create source director")
+		require.NoError(t, os.MkdirAll(testDir, 0o777), "Failed to create source director")
 
 		curFile := getSelfFilename(t)
 		setupSymlink(t, curFile, filepath.Join(testDir, curFile))
@@ -175,7 +175,7 @@ func getSelfFilename(t *testing.T) string {
 
 func setupSymlink(t *testing.T, src, dst string) {
 	// Make sure the destination directory exists.
-	os.MkdirAll(filepath.Dir(dst), 0777)
+	require.NoError(t, os.MkdirAll(filepath.Dir(dst), 0o777))
 
 	// Get absolute path of the source for the symlink, otherwise we can create a symlink
 	// that uses relative paths.

writer.go

@@ -62,7 +62,7 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 	closers := make([]io.Closer, 0, len(paths))
 	closeAll := func() {
 		for _, c := range closers {
-			c.Close()
+			_ = c.Close()
 		}
 	}
 

writer_test.go

@@ -91,7 +91,6 @@ func TestOpen(t *testing.T) {
 	}
 
 	assert.True(t, fileExists(tempName))
-	os.Remove(tempName)
 }
 
 func TestOpenPathsNotFound(t *testing.T) {
@@ -255,7 +254,8 @@ func TestOpenWithErroringSinkFactory(t *testing.T) {
 func TestCombineWriteSyncers(t *testing.T) {
 	tw := &testWriter{"test", t}
 	w := CombineWriteSyncers(tw)
-	w.Write([]byte("test"))
+	_, err := w.Write([]byte("test"))
+	assert.NoError(t, err, "Unexpected write error.")
 }
 
 func fileExists(name string) bool {

zapcore/buffered_write_syncer_bench_test.go

@@ -40,11 +40,15 @@ func BenchmarkBufferedWriteSyncer(b *testing.B) {
 		w := &BufferedWriteSyncer{
 			WS: AddSync(file),
 		}
-		defer w.Stop()
+		defer func() {
+			assert.NoError(b, w.Stop(), "failed to stop buffered write syncer")
+		}()
 		b.ResetTimer()
 		b.RunParallel(func(pb *testing.PB) {
 			for pb.Next() {
-				w.Write([]byte("foobarbazbabble"))
+				if _, err := w.Write([]byte("foobarbazbabble")); err != nil {
+					b.Fatal(err)
+				}
 			}
 		})
 	})

zapcore/buffered_write_syncer_test.go

@@ -101,7 +101,8 @@ func TestBufferWriter(t *testing.T) {
 		n, err := ws.Write([]byte("foo"))
 		require.NoError(t, err, "Unexpected error writing to WriteSyncer.")
 		require.Equal(t, 3, n, "Wrote an unexpected number of bytes.")
-		ws.Write([]byte("foo"))
+		_, err = ws.Write([]byte("foo"))
+		assert.Error(t, err, "Expected error writing to WriteSyncer.")
 		assert.Error(t, ws.Stop(), "Expected stop to fail.")
 	})