diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index 2abbeda4f38..0206073578d 100644 --- a/go/analysis/passes/printf/printf.go +++ b/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{'#'}) { diff --git a/go/analysis/passes/printf/printf_test.go b/go/analysis/passes/printf/printf_test.go index fd22cf6d381..142afa14e89 100644 --- a/go/analysis/passes/printf/printf_test.go +++ b/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...) } diff --git a/go/analysis/passes/printf/testdata/src/a/a.go b/go/analysis/passes/printf/testdata/src/a/a.go index a2a85a9d5a1..5eca3172dec 100644 --- a/go/analysis/passes/printf/testdata/src/a/a.go +++ b/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 diff --git a/go/analysis/passes/printf/testdata/src/typeparams/diagnostics.go b/go/analysis/passes/printf/testdata/src/typeparams/diagnostics.go new file mode 100644 index 00000000000..71f3f9c0c12 --- /dev/null +++ b/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" +} diff --git a/go/analysis/passes/printf/types.go b/go/analysis/passes/printf/types.go index 4a173067491..81bf36e1eef 100644 --- a/go/analysis/passes/printf/types.go +++ b/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. +// +// 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. // -// (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 { +// 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 -}