From ce501357b6cd2c2cc08b07992ea758a30c46de73 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Fri, 14 Apr 2023 10:13:22 -0400 Subject: [PATCH] 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 gopls-CI: kokoro Reviewed-by: Alan Donovan Run-TryBot: Jonathan Amsterdam --- go/analysis/passes/slog/doc.go | 23 +++ go/analysis/passes/slog/slog.go | 218 ++++++++++++++++++++ go/analysis/passes/slog/slog_test.go | 19 ++ go/analysis/passes/slog/testdata/src/a/a.go | 77 +++++++ 4 files changed, 337 insertions(+) create mode 100644 go/analysis/passes/slog/doc.go create mode 100644 go/analysis/passes/slog/slog.go create mode 100644 go/analysis/passes/slog/slog_test.go create mode 100644 go/analysis/passes/slog/testdata/src/a/a.go diff --git a/go/analysis/passes/slog/doc.go b/go/analysis/passes/slog/doc.go new file mode 100644 index 00000000000..ecb10e0948c --- /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 00000000000..c5fcfece1ec --- /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 00000000000..7184215d5fd --- /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 00000000000..bed7f706a4c --- /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) +}