add slowg analyzer to check inappropriate use of log/slog

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

.gitignore

@@ -1 +1 @@
-customvet
+linters

README.md

@@ -16,6 +16,25 @@ file which fills up memory leader to a denial of service attack). Users are
 encouraged to use alternative constructs such as making use of
 [io.LimitReader](https://pkg.go.dev/io#LimitReader).
 
+slowg
+-----
+
+`slowg` is an analyzer that checks for inappropriate use of `Logger.With` from
+the `log/slog` (or `golang.org/x/exp/slog`) package.
+
+`Logger.With()` (and `Logger.WithGroup()` creates a new Logger containing the
+provided attributes. The parent logger is cloned when arguments are supplied,
+which is a relatively expensive operation which should not be used in hot code
+path.
+
+For example, slowg would report the following call:
+
+ log.With("key", val).Info("message")
+
+Which should be replaced with the following call:
+
+ log.Info("message", "key", val)
+
 timeafter
 ---------
 

go.mod

@@ -2,7 +2,10 @@ module github.com/cilium/linters
 
 go 1.19
 
-require golang.org/x/tools v0.11.0
+require (
+ golang.org/x/exp v0.0.0-20230711023510-fffb14384f22
+ golang.org/x/tools v0.11.0
+)
 
 require (
  golang.org/x/mod v0.12.0 // indirect

go.sum

@@ -1,3 +1,5 @@
+golang.org/x/exp v0.0.0-20230711023510-fffb14384f22 h1:FqrVOBQxQ8r/UwwXibI0KMolVhvFiGobSfdE33deHJM=
+golang.org/x/exp v0.0.0-20230711023510-fffb14384f22/go.mod h1:FXUEEKJgO7OQYeo8N01OfiKP8RXMtf6e8aTskBGqWdc=
 golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
 golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
 golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=

ioreadall/io_readall.go

@@ -22,8 +22,9 @@ var ioReadAllPkgs = []string{"io", "ioutil"}
 // Analyzer implements an analysis function that checks for the use of
 // io.ReadAll.
 var Analyzer = &analysis.Analyzer{
- Name: "readall",
+ Name: "ioreadall",
  Doc: `check for "io.ReadAll" instances`,
+ URL: "https://github.com/cilium/linters/ioreadall",
  Requires: []*analysis.Analyzer{inspect.Analyzer},
  Run: run,
 }

main.go

@@ -6,11 +6,16 @@ package main
 
 import (
  "github.com/cilium/linters/ioreadall"
+ "github.com/cilium/linters/slowg"
  "github.com/cilium/linters/timeafter"
 
  "golang.org/x/tools/go/analysis/multichecker"
 )
 
 func main() {
- multichecker.Main(timeafter.Analyzer, ioreadall.Analyzer)
+ multichecker.Main(
+ ioreadall.Analyzer,
+ slowg.Analyzer,
+ timeafter.Analyzer,
+ )
 }

slowg/doc.go

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+// Package slowg defines an Analyzer that checks for inappropriate use of
+// Logger.With() from the log/slog package.
+//
+// # Analyzer slowg
+//
+// slowg: check for inappropriate use of Logger.With().
+//
+// The slowg checker looks for calls to Logger.With() from the log/slog
+// package. Logger.With() constructs a new Logger containing the provided
+// attributes. The parent logger is cloned when arguments are supplied, which
+// is a relatively expensive operation which should not be used in hot code path.
+// For example, slowg would report the following call:
+//
+// log.With("key", val).Info("message")
+//
+// And suggest to replace it with the following call:
+//
+// log.Info("message", "key", val)
+package slowg

slowg/slowg.go

@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package slowg
+
+import (
+ "errors"
+ "go/ast"
+ "go/types"
+
+ _ "golang.org/x/exp/slog" // require the exp module for the unit tests
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+)
+
+// Analyzer implements an analysis function that checks for inappropriate use
+// of Logger.With.
+var Analyzer = &analysis.Analyzer{
+ Name: "slowg",
+ Doc: "check for inappropriate use of Logger.With()",
+ URL: "https://github.com/cilium/linters/slowg",
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+ inspect, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+ if !ok {
+ return nil, errors.New("require analyzer of type *inspector.Inspector")
+ }
+ nodeFilter := []ast.Node{
+ (*ast.SelectorExpr)(nil),
+ }
+ inspect.Preorder(nodeFilter, func(node ast.Node) {
+ sel, ok := node.(*ast.SelectorExpr)
+ if !ok {
+ return
+ }
+
+ if sel.Sel == nil {
+ return
+ }
+ call, ok := sel.X.(*ast.CallExpr)
+ if !ok {
+ return
+ }
+ fn := typeutil.StaticCallee(pass.TypesInfo, call)
+ if fn == nil {
+ // not a static call
+ return
+ }
+ if !isSlogPkg(fn) {
+ // not the log/slog or x/exp/slog package
+ return
+ }
+ if recvName(fn) != "Logger" {
+ // not a receiver of the Logger struct
+ return
+ }
+ switch fn.Name() {
+ case "With", "WithGroup":
+ default:
+ // not one of the call we need to care about
+ return
+ }
+ meth := sel.Sel.Name
+ if !isLogMethod(meth) {
+ // not a logging method (e.g. Info, DebugCtx, ...)
+ return
+ }
+ pass.ReportRangef(call, "call to %s on a newly instantiated Logger", meth)
+ })
+ return nil, nil
+}
+
+func isSlogPkg(fn *types.Func) bool {
+ switch fn.Pkg().Path() {
+ case "log/slog":
+ return true
+ case "golang.org/x/exp/slog":
+ return true
+ }
+ return false
+}
+
+func isLogMethod(s string) bool {
+ switch s {
+ case "Log", "LogAttrs",
+ "Debug", "Info", "Warn", "Error",
+ "DebugCtx", "InfoCtx", "WarnCtx", "ErrorCtx":
+ return true
+ }
+ return false
+}
+
+func recvName(fn *types.Func) string {
+ sig, ok := fn.Type().(*types.Signature)
+ if !ok {
+ return ""
+ }
+ recv := sig.Recv()
+ if recv != nil {
+ t := recv.Type()
+ if pt, ok := t.(*types.Pointer); ok {
+ t = pt.Elem()
+ }
+ if nt, ok := t.(*types.Named); ok {
+ return nt.Obj().Name()
+ }
+ }
+ return ""
+}

slowg/slowg_stdlib_test.go

@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+//go:build go1.21
+
+package slowg
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestStdlib(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, Analyzer, "stdlib")
+}

slowg/slowg_xexp_test.go

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package slowg
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestExp(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, Analyzer, "xexp")
+}

slowg/testdata/src/golang.org

@@ -0,0 +1 @@
+../../../vendor/golang.org

slowg/testdata/src/stdlib/stdlib.go

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+//go:build go1.21
+
+package stdlib
+
+import (
+ "io"
+ "log/slog"
+)
+
+func main() {
+ log := slog.New(slog.NewTextHandler(io.Discard, nil))
+
+ log.Debug("debug")
+ log.Info("info")
+ log.Warn("warn")
+ log.Error("error")
+ log.DebugCtx(nil, "debug")
+ log.InfoCtx(nil, "info")
+ log.WarnCtx(nil, "warn")
+ log.ErrorCtx(nil, "error")
+ log.Log(nil, slog.LevelInfo, "log")
+ log.LogAttrs(nil, slog.LevelInfo, "log")
+
+ // With
+ log.With("key", "val").Debug("debug") // want `call to Debug on a newly instantiated Logger`
+ log.With("key", "val").Info("info") // want `call to Info on a newly instantiated Logger`
+ log.With("key", "val").Warn("warn") // want `call to Warn on a newly instantiated Logger`
+ log.With("key", "val").Error("error") // want `call to Error on a newly instantiated Logger`
+ log.With("key", "val").DebugCtx(nil, "debug") // want `call to DebugCtx on a newly instantiated Logger`
+ log.With("key", "val").InfoCtx(nil, "info") // want `call to InfoCtx on a newly instantiated Logger`
+ log.With("key", "val").WarnCtx(nil, "warn") // want `call to WarnCtx on a newly instantiated Logger`
+ log.With("key", "val").ErrorCtx(nil, "error") // want `call to ErrorCtx on a newly instantiated Logger`
+ log.With("key", "val").Log(nil, slog.LevelInfo, "log") // want `call to Log on a newly instantiated Logger`
+ log.With("key", "val").LogAttrs(nil, slog.LevelInfo, "log") // want `call to LogAttrs on a newly instantiated Logger`
+
+ // WithGroup
+ log.WithGroup("group").Debug("debug") // want `call to Debug on a newly instantiated Logger`
+ log.WithGroup("group").Info("info") // want `call to Info on a newly instantiated Logger`
+ log.WithGroup("group").Warn("warn") // want `call to Warn on a newly instantiated Logger`
+ log.WithGroup("group").Error("error") // want `call to Error on a newly instantiated Logger`
+ log.WithGroup("group").DebugCtx(nil, "debug") // want `call to DebugCtx on a newly instantiated Logger`
+ log.WithGroup("group").InfoCtx(nil, "info") // want `call to InfoCtx on a newly instantiated Logger`
+ log.WithGroup("group").WarnCtx(nil, "warn") // want `call to WarnCtx on a newly instantiated Logger`
+ log.WithGroup("group").ErrorCtx(nil, "error") // want `call to ErrorCtx on a newly instantiated Logger`
+ log.WithGroup("group").Log(nil, slog.LevelInfo, "log") // want `call to Log on a newly instantiated Logger`
+ log.WithGroup("group").LogAttrs(nil, slog.LevelInfo, "log") // want `call to LogAttrs on a newly instantiated Logger`
+}

slowg/testdata/src/xexp/xexp.go

@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package xexp
+
+import (
+ "io"
+
+ "golang.org/x/exp/slog"
+)
+
+func main() {
+ log := slog.New(slog.NewTextHandler(io.Discard, nil))
+
+ log.Debug("debug")
+ log.Info("info")
+ log.Warn("warn")
+ log.Error("error")
+ log.DebugCtx(nil, "debug")
+ log.InfoCtx(nil, "info")
+ log.WarnCtx(nil, "warn")
+ log.ErrorCtx(nil, "error")
+ log.Log(nil, slog.LevelInfo, "log")
+ log.LogAttrs(nil, slog.LevelInfo, "log")
+
+ // With
+ log.With("key", "val").Debug("debug") // want `call to Debug on a newly instantiated Logger`
+ log.With("key", "val").Info("info") // want `call to Info on a newly instantiated Logger`
+ log.With("key", "val").Warn("warn") // want `call to Warn on a newly instantiated Logger`
+ log.With("key", "val").Error("error") // want `call to Error on a newly instantiated Logger`
+ log.With("key", "val").DebugCtx(nil, "debug") // want `call to DebugCtx on a newly instantiated Logger`
+ log.With("key", "val").InfoCtx(nil, "info") // want `call to InfoCtx on a newly instantiated Logger`
+ log.With("key", "val").WarnCtx(nil, "warn") // want `call to WarnCtx on a newly instantiated Logger`
+ log.With("key", "val").ErrorCtx(nil, "error") // want `call to ErrorCtx on a newly instantiated Logger`
+ log.With("key", "val").Log(nil, slog.LevelInfo, "log") // want `call to Log on a newly instantiated Logger`
+ log.With("key", "val").LogAttrs(nil, slog.LevelInfo, "log") // want `call to LogAttrs on a newly instantiated Logger`
+
+ // WithGroup
+ log.WithGroup("group").Debug("debug") // want `call to Debug on a newly instantiated Logger`
+ log.WithGroup("group").Info("info") // want `call to Info on a newly instantiated Logger`
+ log.WithGroup("group").Warn("warn") // want `call to Warn on a newly instantiated Logger`
+ log.WithGroup("group").Error("error") // want `call to Error on a newly instantiated Logger`
+ log.WithGroup("group").DebugCtx(nil, "debug") // want `call to DebugCtx on a newly instantiated Logger`
+ log.WithGroup("group").InfoCtx(nil, "info") // want `call to InfoCtx on a newly instantiated Logger`
+ log.WithGroup("group").WarnCtx(nil, "warn") // want `call to WarnCtx on a newly instantiated Logger`
+ log.WithGroup("group").ErrorCtx(nil, "error") // want `call to ErrorCtx on a newly instantiated Logger`
+ log.WithGroup("group").Log(nil, slog.LevelInfo, "log") // want `call to Log on a newly instantiated Logger`
+ log.WithGroup("group").LogAttrs(nil, slog.LevelInfo, "log") // want `call to LogAttrs on a newly instantiated Logger`
+}

timeafter/time_after.go

@@ -23,6 +23,7 @@ const (
 var Analyzer = &analysis.Analyzer{
  Name: "timeafter",
  Doc: `check for "time.After" instances in loops`,
+ URL: "https://github.com/cilium/linters/timeafter",
  Requires: []*analysis.Analyzer{inspect.Analyzer},
  Run: run,
 }