Skip to content

Commit

Permalink
lint: Enable errcheck, fix failures (#1345)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
abhinav authored Sep 6, 2023
1 parent 9a36792 commit 2b35963
Show file tree
Hide file tree
Showing 29 changed files with 309 additions and 71 deletions.
28 changes: 26 additions & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ linters:
disable-all: true
enable:
# golangci-lint defaults:
# - errcheck TODO: enable errcheck
- errcheck
- gosimple
- govet
- ineffassign
Expand All @@ -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:
Expand All @@ -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
Expand All @@ -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'
18 changes: 9 additions & 9 deletions benchmarks/scenario_bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ func BenchmarkDisabledWithoutFields(b *testing.B) {
}
})
})

}

func BenchmarkDisabledAccumulatedContext(b *testing.B) {
Expand Down Expand Up @@ -183,7 +182,6 @@ func BenchmarkDisabledAccumulatedContext(b *testing.B) {
}
})
})

}

func BenchmarkDisabledAddingFields(b *testing.B) {
Expand Down Expand Up @@ -253,7 +251,6 @@ func BenchmarkDisabledAddingFields(b *testing.B) {
}
})
})

}

func BenchmarkWithoutFields(b *testing.B) {
Expand Down Expand Up @@ -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)
}
}
})
})
Expand Down Expand Up @@ -401,7 +400,6 @@ func BenchmarkWithoutFields(b *testing.B) {
}
})
})

}

func BenchmarkAccumulatedContext(b *testing.B) {
Expand Down Expand Up @@ -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)
}
}
})
})
Expand Down Expand Up @@ -531,7 +531,6 @@ func BenchmarkAccumulatedContext(b *testing.B) {
}
})
})

}

func BenchmarkAddingFields(b *testing.B) {
Expand Down Expand Up @@ -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)
}
}
})
})
Expand Down Expand Up @@ -643,5 +644,4 @@ func BenchmarkAddingFields(b *testing.B) {
}
})
})

}
4 changes: 2 additions & 2 deletions encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
})
}
Expand All @@ -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")
Expand Down
5 changes: 4 additions & 1 deletion error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
36 changes: 36 additions & 0 deletions error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
19 changes: 13 additions & 6 deletions http_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand All @@ -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.",
})
}
Expand Down Expand Up @@ -129,5 +137,4 @@ func decodePutJSON(body io.Reader) (zapcore.Level, error) {
return 0, errors.New("must specify logging level")
}
return *pld.Level, nil

}
29 changes: 28 additions & 1 deletion http_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package zap_test

import (
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"strings"
Expand Down Expand Up @@ -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 {
Expand All @@ -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")
}
2 changes: 1 addition & 1 deletion logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
5 changes: 3 additions & 2 deletions sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions stacktrace_ext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down
4 changes: 2 additions & 2 deletions writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ func TestOpen(t *testing.T) {
}

assert.True(t, fileExists(tempName))
os.Remove(tempName)
}

func TestOpenPathsNotFound(t *testing.T) {
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 6 additions & 2 deletions zapcore/buffered_write_syncer_bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
})
})
Expand Down
3 changes: 2 additions & 1 deletion zapcore/buffered_write_syncer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
})

Expand Down
Loading

0 comments on commit 2b35963

Please sign in to comment.