lint: Enable errcheck, fix failures (#1345)

This enables linting with errcheck on the repository.
Exclusions were added for functions that are known
to never fail, e.g. all Write methods on Zap's buffer.Buffer.

In attempting to enable exclusions for these functions,
I discovered and fixed a typo in the golangci.yml.

<details>
<summary>Complete list of issues fixed</summary>

```
encoder_test.go:44:18: Error return value is not checked (errcheck)
encoder_test.go:55:18: Error return value is not checked (errcheck)
error.go:64:19: Error return value of `arr.AppendObject` is not checked (errcheck)
http_handler.go:83:13: Error return value of `enc.Encode` is not checked (errcheck)
http_handler.go:88:14: Error return value of `enc.Encode` is not checked (errcheck)
http_handler.go:92:13: Error return value of `enc.Encode` is not checked (errcheck)
http_handler.go:95:13: Error return value of `enc.Encode` is not checked (errcheck)
http_handler_test.go:170:24: Error return value of `res.Body.Close` is not checked (errcheck)
logger.go:377:24: Error return value of `log.errorOutput.Sync` is not checked (errcheck)
sink.go:69:17: Error return value of `sr.RegisterSink` is not checked (errcheck)
stacktrace_ext_test.go:178:13: Error return value of `os.MkdirAll` is not checked (errcheck)
writer.go:65:11: Error return value of `c.Close` is not checked (errcheck)
writer_test.go:94:11: Error return value of `os.Remove` is not checked (errcheck)
writer_test.go:258:9: Error return value of `w.Write` is not checked (errcheck)
zapcore/buffered_write_syncer_bench_test.go:43:15: Error return value of `w.Stop` is not checked (errcheck)
zapcore/buffered_write_syncer_bench_test.go:47:12: Error return value of `w.Write` is not checked (errcheck)
zapcore/buffered_write_syncer_test.go:104:11: Error return value of `ws.Write` is not checked (errcheck)
zapcore/core.go:107:9: Error return value of `c.Sync` is not checked (errcheck)
zapcore/core_test.go:151:13: Error return value of `core.Write` is not checked (errcheck)
zapcore/encoder_test.go:288:17: Error return value of `enc.AddArray` is not checked (errcheck)
zapcore/encoder_test.go:316:17: Error return value of `enc.AddArray` is not checked (errcheck)
zapcore/encoder_test.go:723:14: Error return value of `mem.AddArray` is not checked (errcheck)
zapcore/entry.go:245:23: Error return value of `ce.ErrorOutput.Sync` is not checked (errcheck)
zapcore/entry.go:257:22: Error return value of `ce.ErrorOutput.Sync` is not checked (errcheck)
zapcore/error.go:101:19: Error return value of `arr.AppendObject` is not checked (errcheck)
zapcore/json_encoder_bench_test.go:34:16: Error return value of `enc.AddObject` is not checked (errcheck)
zapcore/json_encoder_bench_test.go:98:16: Error return value of `json.Marshal` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:252:16: Error return value of `e.AddObject` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:260:16: Error return value of `e.AddObject` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:268:16: Error return value of `e.AddObject` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:292:13: Error return value of `e.AddArray` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:423:21: Error return value of `arr.AppendObject` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:431:21: Error return value of `arr.AppendObject` is not checked (errcheck)
zapcore/json_encoder_impl_test.go:533:20: Error return value of `arr.AppendObject` is not checked (errcheck)
zapcore/level_test.go:162:16: Error return value of `l.MarshalText` is not checked (errcheck)
zapcore/memory_encoder_test.go:221:16: Error return value of `e.AddObject` is not checked (errcheck)
zapcore/memory_encoder_test.go:235:16: Error return value of `e.AddObject` is not checked (errcheck)
zapcore/memory_encoder_test.go:288:54: Error return value of `e.AppendReflected` is not checked (errcheck)
zapcore/memory_encoder_test.go:294:18: Error return value of `e.AppendArray` is not checked (errcheck)
zapcore/memory_encoder_test.go:305:18: Error return value of `e.AppendArray` is not checked (errcheck)
zapcore/memory_encoder_test.go:306:24: Error return value of `inner.AppendObject` is not checked (errcheck)
zapcore/memory_encoder_test.go:321:18: Error return value of `e.AppendArray` is not checked (errcheck)
zapcore/memory_encoder_test.go:322:24: Error return value of `inner.AppendObject` is not checked (errcheck)
zapcore/tee_test.go:123:13: Error return value of `tee.Write` is not checked (errcheck)
zapcore/write_syncer_bench_test.go:40:12: Error return value of `w.Write` is not checked (errcheck)
zapcore/write_syncer_bench_test.go:54:12: Error return value of `w.Write` is not checked (errcheck)
zapcore/write_syncer_bench_test.go:67:15: Error return value of `w.Stop` is not checked (errcheck)
zapcore/write_syncer_bench_test.go:71:12: Error return value of `w.Write` is not checked (errcheck)
zapcore/write_syncer_bench_test.go:86:12: Error return value of `w.Write` is not checked (errcheck)
zapcore/write_syncer_test.go:73:9: Error return value of `ws.Sync` is not checked (errcheck)
zaptest/observer/observer_test.go:176:15: Error return value of `logger.Write` is not checked (errcheck)
```

</details>

Author: abhinav

.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.