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 <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>

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

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,
+}

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")
+}

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)
+}