[tools] go/analysis/passes/slog: static checks for slog k-v pairs
369 views
Skip to first unread message
Jonathan Amsterdam (Gerrit)
Apr 21, 2023, 1:08:49 PM4/21/23
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, kokoro, Gopher Robot, Alan Donovan, golang-co...@googlegroups.com
Jonathan Amsterdam submitted this change.
Approvals:
Alan Donovan: Looks good to me, approved
Jonathan Amsterdam: Run TryBots
Gopher Robot: TryBots succeeded
kokoro: gopls CI succeeded
go/analysis/passes/slog: static checks for slog k-v pairs
Add an analyzer for slog calls that take a ...any representing
alternating key-value pairs, or slog.Attrs.
The analyzer checks that a value is a key position is either
a string or a slog.Attr. It also checks that the final key
in the list is followed by a value.
Updates #59407.
Change-Id: I9a09cc4329338dc8ab6db03af60fc09e8cb6066c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/484737
TryBot-Result: Gopher Robot <go...@golang.org>
gopls-CI: kokoro <noreply...@google.com>
Reviewed-by: Alan Donovan <adon...@google.com>
Run-TryBot: Jonathan Amsterdam <j...@google.com>
---
A go/analysis/passes/slog/doc.go
A go/analysis/passes/slog/slog.go
A go/analysis/passes/slog/slog_test.go
A go/analysis/passes/slog/testdata/src/a/a.go
4 files changed, 337 insertions(+), 0 deletions(-)
diff --git a/go/analysis/passes/slog/doc.go b/go/analysis/passes/slog/doc.go
new file mode 100644
index 0000000..ecb10e0
--- /dev/null
+++ b/go/analysis/passes/slog/doc.go
@@ -0,0 +1,23 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package slog defines an Analyzer that checks for
+// mismatched key-value pairs in log/slog calls.
+//
+// # Analyzer slog
+//
+// slog: check for invalid structured logging calls
+//
+// The slog checker looks for calls to functions from the log/slog
+// package that take alternating key-value pairs. It reports calls
+// where an argument in a key position is neither a string nor a
+// slog.Attr, and where a final key is missing its value.
+// For example,it would report
+//
+// slog.Warn("message", 11, "k") // slog.Warn arg "11" should be a string or a slog.Attr
+//
+// and
+//
+// slog.Info("message", "k1", v1, "k2") // call to slog.Info missing a final value
+package slog
diff --git a/go/analysis/passes/slog/slog.go b/go/analysis/passes/slog/slog.go
new file mode 100644
index 0000000..c5fcfec
--- /dev/null
+++ b/go/analysis/passes/slog/slog.go
@@ -0,0 +1,218 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// TODO(jba) deduce which functions wrap the log/slog functions, and use the
+// fact mechanism to propagate this information, so we can provide diagnostics
+// for user-supplied wrappers.
+
+package slog
+
+import (
+ _ "embed"
+ "fmt"
+ "go/ast"
+ "go/token"
+ "go/types"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/analysis/passes/internal/analysisutil"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+)
+
+//go:embed doc.go
+var doc string
+
+var Analyzer = &analysis.Analyzer{
+ Name: "slog",
+ Doc: analysisutil.MustExtractDoc(doc, "slog"),
+ URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/slog",
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+}
+
+var stringType = types.Universe.Lookup("string").Type()
+
+// A position describes what is expected to appear in an argument position.
+type position int
+
+const (
+ // key is an argument position that should hold a string key or an Attr.
+ key position = iota
+ // value is an argument position that should hold a value.
+ value
+ // unknown represents that we do not know if position should hold a key or a value.
+ unknown
+)
+
+func run(pass *analysis.Pass) (any, error) {
+ inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+ nodeFilter := []ast.Node{
+ (*ast.CallExpr)(nil),
+ }
+ inspect.Preorder(nodeFilter, func(node ast.Node) {
+ call := node.(*ast.CallExpr)
+ fn := typeutil.StaticCallee(pass.TypesInfo, call)
+ if fn == nil {
+ return // not a static call
+ }
+ if call.Ellipsis != token.NoPos {
+ return // skip calls with "..." args
+ }
+ skipArgs, ok := kvFuncSkipArgs(fn)
+ if !ok {
+ // Not a slog function that takes key-value pairs.
+ return
+ }
+ if len(call.Args) <= skipArgs {
+ // Too few args; perhaps there are no k-v pairs.
+ return
+ }
+
+ // Check this call.
+ // The first position should hold a key or Attr.
+ pos := key
+ sawUnknown := false
+ for _, arg := range call.Args[skipArgs:] {
+ t := pass.TypesInfo.Types[arg].Type
+ switch pos {
+ case key:
+ // Expect a string or Attr.
+ switch {
+ case t == stringType:
+ pos = value
+ case isAttr(t):
+ pos = key
+ case types.IsInterface(t):
+ // We don't know if this arg is a string or an Attr, so we don't know what to expect next.
+ // (We could see if one of interface's methods isn't a method of Attr, and thus know
+ // for sure that this type is definitely not a string or Attr, but it doesn't seem
+ // worth the effort for such an unlikely case.)
+ pos = unknown
+ default:
+ // Definitely not a key.
+ pass.ReportRangef(call, "%s arg %q should be a string or a slog.Attr (possible missing key or value)",
+ shortName(fn), analysisutil.Format(pass.Fset, arg))
+ // Assume this was supposed to be a value, and expect a key next.
+ pos = key
+ }
+
+ case value:
+ // Anything can appear in this position.
+ // The next position should be a key.
+ pos = key
+
+ case unknown:
+ // We don't know anything about this position, but all hope is not lost.
+ if t != stringType && !isAttr(t) && !types.IsInterface(t) {
+ // This argument is definitely not a key.
+ //
+ // The previous argument could have been a key, in which case this is the
+ // corresponding value, and the next position should hold another key.
+ // We will assume that.
+ pos = key
+ // Another possibility: the previous argument was an Attr, and this is
+ // a value incorrectly placed in a key position.
+ // If we assumed this case instead, we might produce a false positive
+ // (since the first case might actually hold).
+
+ // Once we encounter an unknown position, we can never be
+ // sure if a problem at the end of the call is due to a
+ // missing final value, or a non-key in key position.
+ sawUnknown = true
+ }
+ }
+ }
+ if pos == value {
+ if sawUnknown {
+ pass.ReportRangef(call, "call to %s has a missing or misplaced value", shortName(fn))
+ } else {
+ pass.ReportRangef(call, "call to %s missing a final value", shortName(fn))
+ }
+ }
+ })
+ return nil, nil
+}
+
+func isAttr(t types.Type) bool {
+ return t.String() == "log/slog.Attr"
+}
+
+// shortName returns a name for the function that is shorter than FullName.
+// Examples:
+//
+// "slog.Info" (instead of "log/slog.Info")
+// "slog.Logger.With" (instead of "(*log/slog.Logger).With")
+func shortName(fn *types.Func) string {
+ var r string
+ if recv := fn.Type().(*types.Signature).Recv(); recv != nil {
+ t := recv.Type()
+ if pt, ok := t.(*types.Pointer); ok {
+ t = pt.Elem()
+ }
+ if nt, ok := t.(*types.Named); ok {
+ r = nt.Obj().Name()
+ } else {
+ r = recv.Type().String()
+ }
+ r += "."
+ }
+ return fmt.Sprintf("%s.%s%s", fn.Pkg().Name(), r, fn.Name())
+}
+
+// If fn is a slog function that has a ...any parameter for key-value pairs,
+// kvFuncSkipArgs returns the number of arguments to skip over to reach the
+// corresponding arguments, and true.
+// Otherwise it returns (0, false).
+func kvFuncSkipArgs(fn *types.Func) (int, bool) {
+ if pkg := fn.Pkg(); pkg == nil || pkg.Path() != "log/slog" {
+ return 0, false
+ }
+ recv := fn.Type().(*types.Signature).Recv()
+ if recv == nil {
+ // TODO: If #59204 is accepted, uncomment the lines below.
+ // if fn.Name() == "Group" {
+ // return 0, true
+ // }
+ skip, ok := slogOutputFuncs[fn.Name()]
+ return skip, ok
+ }
+ var recvName string
+ if pt, ok := recv.Type().(*types.Pointer); ok {
+ if nt, ok := pt.Elem().(*types.Named); ok {
+ recvName = nt.Obj().Name()
+ }
+ }
+ if recvName == "" {
+ return 0, false
+ }
+ // The methods on *Logger include all the top-level output methods, as well as "With".
+ if recvName == "Logger" {
+ if fn.Name() == "With" {
+ return 0, true
+ }
+ skip, ok := slogOutputFuncs[fn.Name()]
+ return skip, ok
+ }
+ if recvName == "Record" && fn.Name() == "Add" {
+ return 0, true
+ }
+ return 0, false
+}
+
+// The names of top-level functions and *Logger methods in log/slog that take
+// ...any for key-value pairs, mapped to the number of initial args to skip in
+// order to get to the ones that match the ...any parameter.
+var slogOutputFuncs = map[string]int{
+ "Debug": 1,
+ "Info": 1,
+ "Warn": 1,
+ "Error": 1,
+ "DebugCtx": 2,
+ "InfoCtx": 2,
+ "WarnCtx": 2,
+ "ErrorCtx": 2,
+ "Log": 3,
+}
diff --git a/go/analysis/passes/slog/slog_test.go b/go/analysis/passes/slog/slog_test.go
new file mode 100644
index 0000000..7184215
--- /dev/null
+++ b/go/analysis/passes/slog/slog_test.go
@@ -0,0 +1,19 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package slog_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/slog"
+ "golang.org/x/tools/internal/testenv"
+)
+
+func Test(t *testing.T) {
+ testenv.NeedsGo1Point(t, 21)
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, slog.Analyzer, "a")
+}
diff --git a/go/analysis/passes/slog/testdata/src/a/a.go b/go/analysis/passes/slog/testdata/src/a/a.go
new file mode 100644
index 0000000..bed7f70
--- /dev/null
+++ b/go/analysis/passes/slog/testdata/src/a/a.go
@@ -0,0 +1,77 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the slog checker.
+
+//go:build go1.21
+
+package a
+
+import (
+ "fmt"
+ "log/slog"
+)
+
+func F() {
+ var (
+ l *slog.Logger
+ r slog.Record
+ )
+
+ // Unrelated call.
+ fmt.Println("ok")
+
+ // Valid calls.
+ slog.Info("msg")
+ slog.Info("msg", "a", 1)
+ l.Debug("msg", "a", 1)
+ l.With("a", 1)
+ slog.Warn("msg", slog.Int("a", 1), "k", 2)
+ l.WarnCtx(nil, "msg", "a", 1, slog.Int("b", 2), slog.Int("c", 3), "d", 4)
+ r.Add("a", 1, "b", 2)
+
+ // bad
+ slog.Info("msg", 1) // want `slog.Info arg "1" should be a string or a slog.Attr`
+ l.Info("msg", 2) // want `slog.Logger.Info arg "2" should be a string or a slog.Attr`
+ slog.Debug("msg", "a") // want `call to slog.Debug missing a final value`
+ slog.Warn("msg", slog.Int("a", 1), "k") // want `call to slog.Warn missing a final value`
+ slog.ErrorCtx(nil, "msg", "a", 1, "b") // want `call to slog.ErrorCtx missing a final value`
+ r.Add("K", "v", "k") // want `call to slog.Record.Add missing a final value`
+ l.With("a", "b", 2) // want `slog.Logger.With arg "2" should be a string or a slog.Attr`
+
+ slog.Log(nil, slog.LevelWarn, "msg", "a", "b", 2) // want `slog.Log arg "2" should be a string or a slog.Attr`
+
+ // Skip calls with spread args.
+ var args []any
+ slog.Info("msg", args...)
+
+ // The variadic part of all the calls below begins with an argument of
+ // static type any, followed by an integer.
+ // Even though the we don't know the dynamic type of the first arg, and thus
+ // whether it is a key, an Attr, or something else, the fact that the
+ // following integer arg cannot be a key allows us to assume that we should
+ // expect a key to follow.
+ var a any = "key"
+
+ // This is a valid call for which we correctly produce no diagnostic.
+ slog.Info("msg", a, 7, "key2", 5)
+
+ // This is an invalid call because the final value is missing, but we can't
+ // be sure that's the reason.
+ slog.Info("msg", a, 7, "key2") // want `call to slog.Info has a missing or misplaced value`
+
+ // Here our guess about the unknown arg (a) is wrong: we assume it's a string, but it's an Attr.
+ // Therefore the second argument should be a key, but it is a number.
+ // Ideally our diagnostic would pinpoint the problem, but we don't have enough information.
+ a = slog.Int("a", 1)
+ slog.Info("msg", a, 7, "key2") // want `call to slog.Info has a missing or misplaced value`
+
+ // This call is invalid for the same reason as the one above, but we can't
+ // detect that.
+ slog.Info("msg", a, 7, "key2", 5)
+
+ // Another invalid call we can't detect. Here the first argument is wrong.
+ a = 1
+ slog.Info("msg", a, 7, "b", 5)
+}
To view, visit change 484737. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages