go/analysis/passes/slog: do not report multiple incorrect keys

Stops the slog analyzer from reporting multiple issues for the
same call.

Adds additional unit tests.

Change-Id: I6eec033a5c88fafc792a1d7cc7eb50986bf260a6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487475
Commit-Queue: Tim King <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Tim King <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: timothy-king

go/analysis/passes/internal/analysisutil/util.go

@@ -118,3 +118,12 @@ func Imports(pkg *types.Package, path string) bool {
 	}
 	return false
 }
+
+// IsNamed reports whether t is exactly a named type in a package with a given path.
+func IsNamed(t types.Type, path, name string) bool {
+	if n, ok := t.(*types.Named); ok {
+		obj := n.Obj()
+		return obj.Pkg().Path() == path && obj.Name() == name
+	}
+	return false
+}

go/analysis/passes/slog/slog.go

@@ -66,6 +66,10 @@ func run(pass *analysis.Pass) (any, error) {
 			// Not a slog function that takes key-value pairs.
 			return
 		}
+		if isMethodExpr(pass.TypesInfo, call) {
+			// Call is to a method value. Skip the first argument.
+			skipArgs++
+		}
 		if len(call.Args) <= skipArgs {
 			// Too few args; perhaps there are no k-v pairs.
 			return
@@ -74,7 +78,7 @@ func run(pass *analysis.Pass) (any, error) {
 		// Check this call.
 		// The first position should hold a key or Attr.
 		pos := key
-		sawUnknown := false
+		var unknownArg ast.Expr // nil or the last unknown argument
 		for _, arg := range call.Args[skipArgs:] {
 			t := pass.TypesInfo.Types[arg].Type
 			switch pos {
@@ -86,17 +90,19 @@ func run(pass *analysis.Pass) (any, error) {
 				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.)
+					// As we do not do dataflow, we do not know what the dynamic type is.
+					// It could be a string or an Attr so we don't know what to expect next.
 					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
+					if unknownArg == nil {
+						pass.ReportRangef(arg, "%s arg %q should be a string or a slog.Attr (possible missing key or value)",
+							shortName(fn), analysisutil.Format(pass.Fset, arg))
+					} else {
+						pass.ReportRangef(arg, "%s arg %q should probably be a string or a slog.Attr (previous arg %q cannot be a key)",
+							shortName(fn), analysisutil.Format(pass.Fset, arg), analysisutil.Format(pass.Fset, unknownArg))
+					}
+					// Stop here so we report at most one missing key per call.
+					return
 				}
 
 			case value:
@@ -105,39 +111,35 @@ func run(pass *analysis.Pass) (any, error) {
 				pos = key
 
 			case unknown:
-				// We don't know anything about this position, but all hope is not lost.
+				// Once we encounter an unknown position, we can never be
+				// sure if a problem later or at the end of the call is due to a
+				// missing final value, or a non-key in key position.
+				// In both cases, unknownArg != nil.
+				unknownArg = arg
+
+				// We don't know what is expected 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
+					// unknownArg cannot 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 {
+			if unknownArg == nil {
 				pass.ReportRangef(call, "call to %s missing a final value", shortName(fn))
+			} else {
+				pass.ReportRangef(call, "call to %s has a missing or misplaced value", shortName(fn))
 			}
 		}
 	})
 	return nil, nil
 }
 
 func isAttr(t types.Type) bool {
-	return t.String() == "log/slog.Attr"
+	return analysisutil.IsNamed(t, "log/slog", "Attr")
 }
 
 // shortName returns a name for the function that is shorter than FullName.
@@ -216,3 +218,13 @@ var slogOutputFuncs = map[string]int{
 	"ErrorCtx": 2,
 	"Log":      3,
 }
+
+// isMethodExpr reports whether a call is to a MethodExpr.
+func isMethodExpr(info *types.Info, c *ast.CallExpr) bool {
+	s, ok := c.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+	sel := info.Selections[s]
+	return sel != nil && sel.Kind() == types.MethodExpr
+}

go/analysis/passes/slog/slog_test.go

@@ -15,5 +15,5 @@ import (
 func Test(t *testing.T) {
 	testenv.NeedsGo1Point(t, 21)
 	testdata := analysistest.TestData()
-	analysistest.Run(t, testdata, slog.Analyzer, "a")
+	analysistest.Run(t, testdata, slog.Analyzer, "a", "b")
 }

go/analysis/passes/slog/testdata/src/a/a.go

@@ -9,6 +9,7 @@
 package a
 
 import (
+	"context"
 	"fmt"
 	"log/slog"
 )
@@ -25,11 +26,18 @@ func F() {
 	// Valid calls.
 	slog.Info("msg")
 	slog.Info("msg", "a", 1)
+	slog.Info("", "a", 1, "b", "two")
 	l.Debug("msg", "a", 1)
 	l.With("a", 1)
+	slog.Warn("msg", slog.Int("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)
+	l.DebugCtx(nil, "msg", "a", 1, slog.Int("b", 2), slog.Int("c", 3), "d", 4, slog.Int("e", 5))
 	r.Add("a", 1, "b", 2)
+	(*slog.Logger).Debug(l, "msg", "a", 1, "b", 2)
+
+	var key string
+	r.Add(key, 1)
 
 	// bad
 	slog.Info("msg", 1)                     // want `slog.Info arg "1" should be a string or a slog.Attr`
@@ -40,12 +48,24 @@ func F() {
 	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`
 
+	// Report the first problem if there are multiple bad keys.
+	slog.Debug("msg", "a", 1, 2, 3, 4) // want `slog.Debug arg "2" should be a string or a slog.Attr`
+	slog.Debug("msg", "a", 1, 2, 3, 4) // want `slog.Debug 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`
 
+	// Test method expression call.
+	(*slog.Logger).Debug(l, "msg", "a", 1, 2, 3) // want `slog.Logger.Debug arg "2" should be a string or a slog.Attr`
+
 	// Skip calls with spread args.
 	var args []any
 	slog.Info("msg", args...)
 
+	// Report keys that are statically not exactly "string".
+	type MyString string
+	myKey := MyString("a")  // any(x) looks like <MyString, "a">.
+	slog.Info("", myKey, 1) // want `slog.Info arg "myKey" should be a string or a slog.Attr`
+
 	// 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
@@ -74,4 +94,56 @@ func F() {
 	// Another invalid call we can't detect. Here the first argument is wrong.
 	a = 1
 	slog.Info("msg", a, 7, "b", 5)
+
+	// We can detect the first case as the type of key is UntypedNil,
+	// e.g. not yet assigned to any and not yet an interface.
+	// We cannot detect the second.
+	slog.Debug("msg", nil, 2) // want `slog.Debug arg "nil" should be a string or a slog.Attr`
+	slog.Debug("msg", any(nil), 2)
+
+	// Recovery from unknown value.
+	slog.Debug("msg", any(nil), "a")
+	slog.Debug("msg", any(nil), "a", 2)
+	slog.Debug("msg", any(nil), "a", 2, "b") // want `call to slog.Debug has a missing or misplaced value`
+	slog.Debug("msg", any(nil), 2, 3, 4)     // want "slog.Debug arg \\\"3\\\" should probably be a string or a slog.Attr \\(previous arg \\\"2\\\" cannot be a key\\)"
+}
+
+func All() {
+	// Test all functions and methods at least once.
+	var (
+		l   *slog.Logger
+		r   slog.Record
+		ctx context.Context
+	)
+	slog.Debug("msg", 1, 2) // want `slog.Debug arg "1" should be a string or a slog.Attr`
+	slog.Error("msg", 1, 2) // want `slog.Error arg "1" should be a string or a slog.Attr`
+	slog.Info("msg", 1, 2)  // want `slog.Info arg "1" should be a string or a slog.Attr`
+	slog.Warn("msg", 1, 2)  // want `slog.Warn arg "1" should be a string or a slog.Attr`
+
+	slog.DebugCtx(ctx, "msg", 1, 2) // want `slog.DebugCtx arg "1" should be a string or a slog.Attr`
+	slog.ErrorCtx(ctx, "msg", 1, 2) // want `slog.ErrorCtx arg "1" should be a string or a slog.Attr`
+	slog.InfoCtx(ctx, "msg", 1, 2)  // want `slog.InfoCtx arg "1" should be a string or a slog.Attr`
+	slog.WarnCtx(ctx, "msg", 1, 2)  // want `slog.WarnCtx arg "1" should be a string or a slog.Attr`
+
+	slog.Log(ctx, slog.LevelDebug, "msg", 1, 2) // want `slog.Log arg "1" should be a string or a slog.Attr`
+
+	l.Debug("msg", 1, 2) // want `slog.Logger.Debug arg "1" should be a string or a slog.Attr`
+	l.Error("msg", 1, 2) // want `slog.Logger.Error arg "1" should be a string or a slog.Attr`
+	l.Info("msg", 1, 2)  // want `slog.Logger.Info arg "1" should be a string or a slog.Attr`
+	l.Warn("msg", 1, 2)  // want `slog.Logger.Warn arg "1" should be a string or a slog.Attr`
+
+	l.DebugCtx(ctx, "msg", 1, 2) // want `slog.Logger.DebugCtx arg "1" should be a string or a slog.Attr`
+	l.ErrorCtx(ctx, "msg", 1, 2) // want `slog.Logger.ErrorCtx arg "1" should be a string or a slog.Attr`
+	l.InfoCtx(ctx, "msg", 1, 2)  // want `slog.Logger.InfoCtx arg "1" should be a string or a slog.Attr`
+	l.WarnCtx(ctx, "msg", 1, 2)  // want `slog.Logger.WarnCtx arg "1" should be a string or a slog.Attr`
+
+	l.Log(ctx, slog.LevelDebug, "msg", 1, 2) // want `slog.Logger.Log arg "1" should be a string or a slog.Attr`
+
+	_ = l.With(1, 2) // want `slog.Logger.With arg "1" should be a string or a slog.Attr`
+
+	r.Add(1, 2) // want `slog.Record.Add arg "1" should be a string or a slog.Attr`
+
 }
+
+// Used in tests by package b.
+var MyLogger = slog.Default()

go/analysis/passes/slog/testdata/src/b/b.go

@@ -0,0 +1,15 @@
+// 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 b
+
+import "a"
+
+func Imported() {
+	_ = a.MyLogger.With("a", 1, 2, 3) // want `slog.Logger.With arg "2" should be a string or a slog.Attr`
+}