go/analysis/passes/printf: warn about verbs that don’t match a type

parameter’s type set

Pending the acceptance of golang/go#49038, this CL updates the printf
analyzer to warn if any elements of a type parameter's type set do not
match the printf verb being considered. Since this may be a source of
confusion, it also refactors slightly to allow providing a reason why a
match failed.

Updates golang/go#48704
Updates golang/go#49038

Change-Id: I92d4d58dd0e9218ae9d522bf2a2999f4c6f9fd84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/351832
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

go/analysis/passes/printf/printf.go

@@ -879,8 +879,12 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, state *formatState) (o
 			return
 		}
 		arg := call.Args[argNum]
-		if !matchArgType(pass, argInt, arg) {
-			pass.ReportRangef(call, "%s format %s uses non-int %s as argument of *", state.name, state.format, analysisutil.Format(pass.Fset, arg))
+		if reason, ok := matchArgType(pass, argInt, arg); !ok {
+			details := ""
+			if reason != "" {
+				details = " (" + reason + ")"
+			}
+			pass.ReportRangef(call, "%s format %s uses non-int %s%s as argument of *", state.name, state.format, analysisutil.Format(pass.Fset, arg), details)
 			return false
 		}
 	}
@@ -897,12 +901,16 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, state *formatState) (o
 		pass.ReportRangef(call, "%s format %s arg %s is a func value, not called", state.name, state.format, analysisutil.Format(pass.Fset, arg))
 		return false
 	}
-	if !matchArgType(pass, v.typ, arg) {
+	if reason, ok := matchArgType(pass, v.typ, arg); !ok {
 		typeString := ""
 		if typ := pass.TypesInfo.Types[arg].Type; typ != nil {
 			typeString = typ.String()
 		}
-		pass.ReportRangef(call, "%s format %s has arg %s of wrong type %s", state.name, state.format, analysisutil.Format(pass.Fset, arg), typeString)
+		details := ""
+		if reason != "" {
+			details = " (" + reason + ")"
+		}
+		pass.ReportRangef(call, "%s format %s has arg %s of wrong type %s%s", state.name, state.format, analysisutil.Format(pass.Fset, arg), typeString, details)
 		return false
 	}
 	if v.typ&argString != 0 && v.verb != 'T' && !bytes.Contains(state.flags, []byte{'#'}) {

go/analysis/passes/printf/printf_test.go

@@ -9,10 +9,16 @@ import (
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/printf"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
 	printf.Analyzer.Flags.Set("funcs", "Warn,Warnf")
-	analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt")
+
+	tests := []string{"a", "b", "nofmt"}
+	if typeparams.Enabled {
+		tests = append(tests, "typeparams")
+	}
+	analysistest.Run(t, testdata, printf.Analyzer, tests...)
 }

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

@@ -698,6 +698,7 @@ type unexportedInterface struct {
 type unexportedStringer struct {
 	t ptrStringer
 }
+
 type unexportedStringerOtherFields struct {
 	s string
 	t ptrStringer
@@ -708,6 +709,7 @@ type unexportedStringerOtherFields struct {
 type unexportedError struct {
 	e error
 }
+
 type unexportedErrorOtherFields struct {
 	s string
 	e error

go/analysis/passes/printf/testdata/src/typeparams/diagnostics.go

@@ -0,0 +1,111 @@
+// Copyright 2021 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.
+
+//go:build go1.18
+// +build go1.18
+
+package typeparams
+
+import "fmt"
+
+func TestBasicTypeParams[T interface{ ~int }, E error, F fmt.Formatter, S fmt.Stringer, A any](t T, e E, f F, s S, a A) {
+	fmt.Printf("%d", t)
+	fmt.Printf("%s", t) // want "wrong type.*contains ~int"
+	fmt.Printf("%v", t)
+	fmt.Printf("%d", e)
+	fmt.Printf("%s", e)
+	fmt.Errorf("%w", e)
+	fmt.Printf("%a", f)
+	fmt.Printf("%d", f)
+	fmt.Printf("%T", f.Format)
+	fmt.Printf("%p", f.Format)
+	fmt.Printf("%s", s)
+	fmt.Errorf("%w", s) // want "wrong type"
+	fmt.Printf("%d", a) // want "wrong type"
+	fmt.Printf("%s", a) // want "wrong type"
+	fmt.Printf("%v", a)
+	fmt.Printf("%T", a)
+}
+
+func TestNestedTypeParams[T interface{ ~int }, S interface{ ~string }]() {
+	var x struct {
+		f int
+		t T
+	}
+	fmt.Printf("%d", x)
+	fmt.Printf("%s", x) // want "wrong type"
+	var y struct {
+		f string
+		t S
+	}
+	fmt.Printf("%d", y) // want "wrong type"
+	fmt.Printf("%s", y)
+	var m1 map[T]T
+	fmt.Printf("%d", m1)
+	fmt.Printf("%s", m1) // want "wrong type"
+	var m2 map[S]S
+	fmt.Printf("%d", m2) // want "wrong type"
+	fmt.Printf("%s", m2)
+}
+
+type R struct {
+	F []R
+}
+
+func TestRecursiveTypeDefinition() {
+	var r []R
+	fmt.Printf("%d", r) // No error: avoids infinite recursion.
+}
+
+func TestRecursiveTypeParams[T1 ~[]T2, T2 ~[]T1 | string, T3 ~struct{ F T3 }](t1 T1, t2 T2, t3 T3) {
+	// No error is reported on the following lines to avoid infinite recursion.
+	fmt.Printf("%s", t1)
+	fmt.Printf("%s", t2)
+	fmt.Printf("%s", t3)
+}
+
+func TestRecusivePointers[T1 ~*T2, T2 ~*T1](t1 T1, t2 T2) {
+	// No error: we can't determine if pointer rules apply.
+	fmt.Printf("%s", t1)
+	fmt.Printf("%s", t2)
+}
+
+func TestEmptyTypeSet[T interface{ int | string; float64 }](t T) {
+	fmt.Printf("%s", t) // No error: empty type set.
+}
+
+func TestPointerRules[T ~*[]int|*[2]int](t T) {
+	var slicePtr *[]int
+	var arrayPtr *[2]int
+	fmt.Printf("%d", slicePtr)
+	fmt.Printf("%d", arrayPtr)
+	fmt.Printf("%d", t)
+}
+
+func TestInterfacePromotion[E interface {
+	~int
+	Error() string
+}, S interface {
+	float64
+	String() string
+}](e E, s S) {
+	fmt.Printf("%d", e)
+	fmt.Printf("%s", e)
+	fmt.Errorf("%w", e)
+	fmt.Printf("%d", s) // want "wrong type.*contains float64"
+	fmt.Printf("%s", s)
+	fmt.Errorf("%w", s) // want "wrong type"
+}
+
+type myInt int
+
+func TestTermReduction[T1 interface{ ~int | string }, T2 interface {
+	~int | string
+	myInt
+}](t1 T1, t2 T2) {
+	fmt.Printf("%d", t1) // want "wrong type.*contains string"
+	fmt.Printf("%s", t1) // want "wrong type.*contains ~int"
+	fmt.Printf("%d", t2)
+	fmt.Printf("%s", t2) // want "wrong type.*contains typeparams.myInt"
+}

go/analysis/passes/printf/types.go

@@ -5,38 +5,60 @@
 package printf
 
 import (
+	"fmt"
 	"go/ast"
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 var errorType = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
 
-// matchArgType reports an error if printf verb t is not appropriate
-// for operand arg.
-func matchArgType(pass *analysis.Pass, t printfArgType, arg ast.Expr) bool {
+// matchArgType reports an error if printf verb t is not appropriate for
+// operand arg.
+//
+// If arg is a type parameter, the verb t must be appropriate for every type in
+// the type parameter type set.
+func matchArgType(pass *analysis.Pass, t printfArgType, arg ast.Expr) (reason string, ok bool) {
 	// %v, %T accept any argument type.
 	if t == anyType {
-		return true
+		return "", true
 	}
+
 	typ := pass.TypesInfo.Types[arg].Type
 	if typ == nil {
-		return true // probably a type check problem
+		return "", true // probably a type check problem
 	}
-	return matchArgTypeInternal(t, typ, make(map[types.Type]bool))
+
+	m := &argMatcher{t: t, seen: make(map[types.Type]bool)}
+	ok = m.match(typ, true)
+	return m.reason, ok
 }
 
-// matchArgTypeInternal is the internal version of matchArgType. It carries a map
-// remembering what types are in progress so we don't recur when faced with recursive
-// types or mutually recursive types.
+// argMatcher recursively matches types against the printfArgType t.
 //
-// (Recursion arises from the compound types {map,chan,slice} which
-// may be printed with %d etc. if that is appropriate for their element
-// types.)
-func matchArgTypeInternal(t printfArgType, typ types.Type, inProgress map[types.Type]bool) bool {
+// To short-circuit recursion, it keeps track of types that have already been
+// matched (or are in the process of being matched) via the seen map. Recursion
+// arises from the compound types {map,chan,slice} which may be printed with %d
+// etc. if that is appropriate for their element types, as well as from type
+// parameters, which are expanded to the constituents of their type set.
+//
+// The reason field may be set to report the cause of the mismatch.
+type argMatcher struct {
+	t      printfArgType
+	seen   map[types.Type]bool
+	reason string
+}
+
+// match checks if typ matches m's printf arg type. If topLevel is true, typ is
+// the actual type of the printf arg, for which special rules apply. As a
+// special case, top level type parameters pass topLevel=true when checking for
+// matches among the constituents of their type set, as type arguments will
+// replace the type parameter at compile time.
+func (m *argMatcher) match(typ types.Type, topLevel bool) bool {
 	// %w accepts only errors.
-	if t == argError {
+	if m.t == argError {
 		return types.ConvertibleTo(typ, errorType)
 	}
 
@@ -44,50 +66,106 @@ func matchArgTypeInternal(t printfArgType, typ types.Type, inProgress map[types.
 	if isFormatter(typ) {
 		return true
 	}
+
 	// If we can use a string, might arg (dynamically) implement the Stringer or Error interface?
-	if t&argString != 0 && isConvertibleToString(typ) {
+	if m.t&argString != 0 && isConvertibleToString(typ) {
+		return true
+	}
+
+	if typ, _ := typ.(*typeparams.TypeParam); typ != nil {
+		// Avoid infinite recursion through type parameters.
+		if m.seen[typ] {
+			return true
+		}
+		m.seen[typ] = true
+		terms, err := typeparams.StructuralTerms(typ)
+		if err != nil {
+			return true // invalid type (possibly an empty type set)
+		}
+
+		if len(terms) == 0 {
+			// No restrictions on the underlying of typ. Type parameters implementing
+			// error, fmt.Formatter, or fmt.Stringer were handled above, and %v and
+			// %T was handled in matchType. We're about to check restrictions the
+			// underlying; if the underlying type is unrestricted there must be an
+			// element of the type set that violates one of the arg type checks
+			// below, so we can safely return false here.
+
+			if m.t == anyType { // anyType must have already been handled.
+				panic("unexpected printfArgType")
+			}
+			return false
+		}
+
+		// Only report a reason if typ is the argument type, otherwise it won't
+		// make sense. Note that it is not sufficient to check if topLevel == here,
+		// as type parameters can have a type set consisting of other type
+		// parameters.
+		reportReason := len(m.seen) == 1
+
+		for _, term := range terms {
+			if !m.match(term.Type(), topLevel) {
+				if reportReason {
+					if term.Tilde() {
+						m.reason = fmt.Sprintf("contains ~%s", term.Type())
+					} else {
+						m.reason = fmt.Sprintf("contains %s", term.Type())
+					}
+				}
+				return false
+			}
+		}
 		return true
 	}
 
 	typ = typ.Underlying()
-	if inProgress[typ] {
-		// We're already looking at this type. The call that started it will take care of it.
+	if m.seen[typ] {
+		// We've already considered typ, or are in the process of considering it.
+		// In case we've already considered typ, it must have been valid (else we
+		// would have stopped matching). In case we're in the process of
+		// considering it, we must avoid infinite recursion.
+		//
+		// There are some pathological cases where returning true here is
+		// incorrect, for example `type R struct { F []R }`, but these are
+		// acceptable false negatives.
 		return true
 	}
-	inProgress[typ] = true
+	m.seen[typ] = true
 
 	switch typ := typ.(type) {
 	case *types.Signature:
-		return t == argPointer
+		return m.t == argPointer
 
 	case *types.Map:
-		return t == argPointer ||
-			// Recur: map[int]int matches %d.
-			(matchArgTypeInternal(t, typ.Key(), inProgress) && matchArgTypeInternal(t, typ.Elem(), inProgress))
+		if m.t == argPointer {
+			return true
+		}
+		// Recur: map[int]int matches %d.
+		return m.match(typ.Key(), false) && m.match(typ.Elem(), false)
 
 	case *types.Chan:
-		return t&argPointer != 0
+		return m.t&argPointer != 0
 
 	case *types.Array:
 		// Same as slice.
-		if types.Identical(typ.Elem().Underlying(), types.Typ[types.Byte]) && t&argString != 0 {
+		if types.Identical(typ.Elem().Underlying(), types.Typ[types.Byte]) && m.t&argString != 0 {
 			return true // %s matches []byte
 		}
 		// Recur: []int matches %d.
-		return matchArgTypeInternal(t, typ.Elem(), inProgress)
+		return m.match(typ.Elem(), false)
 
 	case *types.Slice:
 		// Same as array.
-		if types.Identical(typ.Elem().Underlying(), types.Typ[types.Byte]) && t&argString != 0 {
+		if types.Identical(typ.Elem().Underlying(), types.Typ[types.Byte]) && m.t&argString != 0 {
 			return true // %s matches []byte
 		}
-		if t == argPointer {
+		if m.t == argPointer {
 			return true // %p prints a slice's 0th element
 		}
 		// Recur: []int matches %d. But watch out for
 		//	type T []T
 		// If the element is a pointer type (type T[]*T), it's handled fine by the Pointer case below.
-		return matchArgTypeInternal(t, typ.Elem(), inProgress)
+		return m.match(typ.Elem(), false)
 
 	case *types.Pointer:
 		// Ugly, but dealing with an edge case: a known pointer to an invalid type,
@@ -96,31 +174,45 @@ func matchArgTypeInternal(t printfArgType, typ types.Type, inProgress map[types.
 			return true // special case
 		}
 		// If it's actually a pointer with %p, it prints as one.
-		if t == argPointer {
+		if m.t == argPointer {
 			return true
 		}
 
 		under := typ.Elem().Underlying()
 		switch under.(type) {
+		case *typeparams.TypeParam:
+			return true // We don't know whether the logic below applies. Give up.
 		case *types.Struct: // see below
 		case *types.Array: // see below
 		case *types.Slice: // see below
 		case *types.Map: // see below
 		default:
 			// Check whether the rest can print pointers.
-			return t&argPointer != 0
+			return m.t&argPointer != 0
 		}
-		// If it's a top-level pointer to a struct, array, slice, or
+		// If it's a top-level pointer to a struct, array, slice, type param, or
 		// map, that's equivalent in our analysis to whether we can
 		// print the type being pointed to. Pointers in nested levels
 		// are not supported to minimize fmt running into loops.
-		if len(inProgress) > 1 {
+		if !topLevel {
 			return false
 		}
-		return matchArgTypeInternal(t, under, inProgress)
+		return m.match(under, false)
 
 	case *types.Struct:
-		return matchStructArgType(t, typ, inProgress)
+		// report whether all the elements of the struct match the expected type. For
+		// instance, with "%d" all the elements must be printable with the "%d" format.
+		for i := 0; i < typ.NumFields(); i++ {
+			typf := typ.Field(i)
+			if !m.match(typf.Type(), false) {
+				return false
+			}
+			if m.t&argString != 0 && !typf.Exported() && isConvertibleToString(typf.Type()) {
+				// Issue #17798: unexported Stringer or error cannot be properly formatted.
+				return false
+			}
+		}
+		return true
 
 	case *types.Interface:
 		// There's little we can do.
@@ -132,7 +224,7 @@ func matchArgTypeInternal(t printfArgType, typ types.Type, inProgress map[types.
 		switch typ.Kind() {
 		case types.UntypedBool,
 			types.Bool:
-			return t&argBool != 0
+			return m.t&argBool != 0
 
 		case types.UntypedInt,
 			types.Int,
@@ -146,27 +238,27 @@ func matchArgTypeInternal(t printfArgType, typ types.Type, inProgress map[types.
 			types.Uint32,
 			types.Uint64,
 			types.Uintptr:
-			return t&argInt != 0
+			return m.t&argInt != 0
 
 		case types.UntypedFloat,
 			types.Float32,
 			types.Float64:
-			return t&argFloat != 0
+			return m.t&argFloat != 0
 
 		case types.UntypedComplex,
 			types.Complex64,
 			types.Complex128:
-			return t&argComplex != 0
+			return m.t&argComplex != 0
 
 		case types.UntypedString,
 			types.String:
-			return t&argString != 0
+			return m.t&argString != 0
 
 		case types.UnsafePointer:
-			return t&(argPointer|argInt) != 0
+			return m.t&(argPointer|argInt) != 0
 
 		case types.UntypedRune:
-			return t&(argInt|argRune) != 0
+			return m.t&(argInt|argRune) != 0
 
 		case types.UntypedNil:
 			return false
@@ -215,19 +307,3 @@ func hasBasicType(pass *analysis.Pass, x ast.Expr, kind types.BasicKind) bool {
 	b, ok := t.(*types.Basic)
 	return ok && b.Kind() == kind
 }
-
-// matchStructArgType reports whether all the elements of the struct match the expected
-// type. For instance, with "%d" all the elements must be printable with the "%d" format.
-func matchStructArgType(t printfArgType, typ *types.Struct, inProgress map[types.Type]bool) bool {
-	for i := 0; i < typ.NumFields(); i++ {
-		typf := typ.Field(i)
-		if !matchArgTypeInternal(t, typf.Type(), inProgress) {
-			return false
-		}
-		if t&argString != 0 && !typf.Exported() && isConvertibleToString(typf.Type()) {
-			// Issue #17798: unexported Stringer or error cannot be properly formatted.
-			return false
-		}
-	}
-	return true
-}