Skip to content

Commit

Permalink
lint: Enable errcheck, fix failures
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.
  • Loading branch information
abhinav committed Sep 1, 2023
1 parent 98e9c4f commit 370504b
Show file tree
Hide file tree
Showing 26 changed files with 146 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
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

}
4 changes: 3 additions & 1 deletion http_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,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 Down
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
6 changes: 3 additions & 3 deletions zapcore/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ func (c *ioCore) Write(ent Entry, fields []Field) error {
return err
}
if ent.Level > ErrorLevel {
// Since we may be crashing the program, sync the output. Ignore Sync
// errors, pending a clean solution to issue #370.
c.Sync()
// Since we may be crashing the program, sync the output.
// Ignore Sync errors, pending a clean solution to issue #370.
_ = c.Sync()
}
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion zapcore/core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func TestIOCoreSyncsOutput(t *testing.T) {
DebugLevel,
)

core.Write(tt.entry, nil)
assert.NoError(t, core.Write(tt.entry, nil), "Unexpected error writing entry.")
assert.Equal(t, tt.shouldSync, sink.Called(), "Incorrect Sync behavior.")
}
}
Expand Down
9 changes: 6 additions & 3 deletions zapcore/encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,11 @@ func TestEncoderConfiguration(t *testing.T) {
},
extra: func(enc Encoder) {
enc.AddTime("extra", _epoch)
enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error {
err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error {
enc.AppendTime(_epoch)
return nil
}))
assert.NoError(t, err)
},
expectedJSON: `{"L":"info","T":"1970-01-01 00:00:00 +0000 UTC","N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1970-01-01 00:00:00 +0000 UTC","extras":["1970-01-01 00:00:00 +0000 UTC"],"S":"fake-stack"}` + "\n",
expectedConsole: "1970-01-01 00:00:00 +0000 UTC\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // plain-text preamble
Expand All @@ -313,10 +314,11 @@ func TestEncoderConfiguration(t *testing.T) {
},
extra: func(enc Encoder) {
enc.AddDuration("extra", time.Second)
enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error {
err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error {
enc.AppendDuration(time.Minute)
return nil
}))
assert.NoError(t, err)
},
expectedJSON: `{"L":"info","T":0,"N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1s","extras":["1m0s"],"S":"fake-stack"}` + "\n",
expectedConsole: "0\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // preamble
Expand Down Expand Up @@ -720,10 +722,11 @@ func TestNameEncoders(t *testing.T) {

func assertAppended(t testing.TB, expected interface{}, f func(ArrayEncoder), msgAndArgs ...interface{}) {
mem := NewMapObjectEncoder()
mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error {
err := mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error {
f(arr)
return nil
}))
assert.NoError(t, err, msgAndArgs...)
arr := mem.Fields["k"].([]interface{})
require.Equal(t, 1, len(arr), "Expected to append exactly one element to array.")
assert.Equal(t, expected, arr[0], msgAndArgs...)
Expand Down
Loading

0 comments on commit 370504b

Please sign in to comment.