cmd/vet: printf: warn about verbs that don’t match a type parameter’s type set

[findleyr]

As a spin-off of the discussion in #48971, I propose to have cmd/vet report diagnostics for printf verbs that don’t match every element of a type parameter’s type set.

For example, consider the following:

func F[P ~int](p P) {
  fmt.Printf("%s", p)  // int is in the type set of P, and does not match %s
}

func G[Q any](q Q) {
  fmt.Printf(“%d”, q)  // many types in the type set of Q do not match %d
}

In this example, the type set of P is all types with int underlying. There are types in that type set (such as int itself) that do not match the verb %s, so we would report a diagnostic – something like fmt.Printf format %s does not match type int in the underlying type set for P.

Similarly, the type set of Q is all types, and there are of course many types in this type set that do not match %d, so we would report something like fmt.Printf format %d does not match constraint type ‘any’.

In these examples we might have false positives: it could be the case that F is only ever instantiated with arguments implementing fmt.Stringer. It could be the case that G is only ever instantiated with integral types. By comparison, we explicitly do not report such diagnostics for most arguments of interface type:

// Whether any particular verb is valid depends on the argument.
// The user may have reasonable prior knowledge of the contents of the interface.

However, with the new interface syntax in Go 1.18, we are able to express restrictions on both the method set and underlying type of a type parameter. If arguments of type parameter type are being passed to a %d verb, we can prove that this is OK if the type set is restricted to integral underlying types. On the other hand, if the type set is not restricted to integral underlying types, it is highly likely that this is a user error. Specifically, it is more likely than in the case of interface types, because users could have specified structural restrictions on the type parameter.

There is precedent here. In the initial implementation of printf checks for %w, it was decided that all arguments must be convertible to error (including, for example, arguments of type interface{}). The rationale behind this is similar to above: the user is able to express an ‘implements’ relationship between interface types, and if they did not it is likely a user error. [see also #48931].

Generics will be used to add type safety to existing code that previously used interfaces. By flagging potentially incorrect printf verbs, we can help users identify potential runtime errors and add accurate type restrictions to their type parameter constraints.

I have prototyped this feature, and think it is do-able for 1.18, though producing great diagnostic messages might be a bit tricky. I think it’s OK if we wait for 1.19.

CC @timothy-king @neild @bcmills @mvdan

[rsc]

There are two possible ways to extend the printf(format, arg) check for generics:

1. Complain if at least one possible type for arg does not match format.
2. Complain only if all possible types for arg do not match format.

So for example if you have a constraint int|string and format %d, then
in (1) vet would complain because the string case doesn't work with %d,
while in (2) vet would not complain because the int case is OK and maybe it's an int.

It sounds like you are suggesting (1).
That seems more in keeping with the rest of the generics rules.

Does anyone object to (1)?

[rsc]

Also, we may find that (2) is better by trying (1) and realizing we are getting too many errors.
If we don't do (1) now, we will not be able to switch to it easily later.
So that suggests we should do (1).

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[findleyr]

@rsc yes we're suggesting (1).

It's worth pointing out that when there no structural restrictions (such as with any), we would also complain about %d. That's just an application of the rule "at least one possible type doesn't match", but by analogy we don't currently complain about interfaces.

[timothy-king]

I did not see where the proposal states this explicitly or not so I'm going to just ask. Do we need to take the method set into account for some of the verbs? Stealing an example from https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md :

type StringableSignedInteger interface {
	~int | ~int8 | ~int16 | ~int32 | ~int64
	String() string
}

It seems like this should never complain about "%s". Maybe "%w" also needs to take this into account? Any other verbs?

[neild]

Maybe "%w" also needs to take this into account?

%w is very simple in its requirements: It needs a value which is assignable to error. The current vet check enforces this.

[findleyr]

Do we need to take the method set into account for some of the verbs?

Generally speaking, we will have already taken the method set into account before we get to checking underlying types. Checks for whether or not a type is a formatter or stringer or error will have used types.LookupFieldOrMethod or types.ConvertibleTo. Both of those APIs should work as expected for type parameters.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[gopherbot]

Change https://golang.org/cl/351832 mentions this issue: go/analysis/passes/printf: warn about verbs that don’t match a type

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[findleyr]

Whoops, forgot to close this. This is done.