cmd/vet: printf analysis no longer flags interfaces that don't implement error

[findleyr]

I'm not sure if this is a bug or a feature.

In CL 177601, it was explicitly noted that %w verbs would not match interface{}.

Subsequently, in CL 217180, printf analyzer logic was changed to account for the fact that any argument of interface type could be a fmt.Formatter. More context in #36564. As a side-effect, we no longer report diagnostics if arguments for %w of interface type do not implement error.

Either one of these outcomes is reasonable to me:

• the type system allows us to express 'implements' relationships between interfaces, so it is likely a user error if the argument type for %w does not implement error (the same is not true of, say, %d, as we can't currently express that an interface's dynamic type is integral).
• ...but on the other hand we have very low tolerance for false positives in vet. If reasonable user code is generating a false positive, it should be fixed.

I just wanted to flag this inconsistency. What is correct here?

CC @neild @timothy-king

(FWIW, I'm interested in this as a matter of precedent, as I'm trying to figure out what to do with type parameters, where users may express structural restrictions on the underlying of a type parameter).

[neild]

My opinion in CL 177601 was that I couldn't think of any real-world cases where you would want to use the %w verb on an interface value that doesn't implement error, and that you can trivially write a x.(error) type assertion to convert back to an error in the unlikely case that this comes up.

In the time since the check being added, I don't think I've ever heard of a vet false positive for this case.

I think vet should continue to flag using the %w verb with a value that is not an error.

[findleyr]

I think vet should continue to flag using the %w verb with a value that is not an error.

To be clear this is not currently the case, as of CL 217180:
https://play.golang.org/p/JHbPwrqf3l5

Vet only reported arguments of interface type in Go 1.14. Per your reasoning, this is a regression. I agree with your logic, but thought perhaps we had encountered reasonable false positives. @kortschak, is this consistent with your expectation? Do you still think that we should exempt arguments of interface type from this check? Should %w be special?

[kortschak]

I think that it's reasonable to make %w special. The difference between this case and the cases in the issue I filed is that fmt.Formatter is an optional interface for all verbs, while an argument associated with a %w must be an error. While errors are just values, they are semantically laden and I think it's a good thing to make it explicit that a value being rendered is an error and not just an interface that also implements error.

[gopherbot]

Change https://golang.org/cl/355730 mentions this issue: go/analysis/passes/printf: warn against using non-error interface values with %w

[timothy-king]

Bit late to the party. I am having a hard time recalling a real false positive, and I am struggling to think of a realistic false positive. A piece of support for being strict (/ suggesting x.(error) as a replacement) is the documentation for fmt.Errorf:

It is invalid to include more than one %w verb or to supply it with an operand that does not implement the error interface.