Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/vet: decide on support for type parameters in the printf analyzer #48971

Closed
findleyr opened this issue Oct 14, 2021 · 9 comments
Closed

cmd/vet: decide on support for type parameters in the printf analyzer #48971

findleyr opened this issue Oct 14, 2021 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Oct 14, 2021

This is a separate issue from #48704, because we need to make a non-trivial decision about the printf analyzer for 1.18.

The printf analyzer has come up several times in the context of updating tools for generics, because it is an example of where we might want to interrogate the type set of a type parameter -- see below. After going back-and-forth, in my opinion we should be conservative and treat type parameters like interfaces, at least for 1.18.

For discussion, consider the following examples:

// Example 1
func _[T ~int](t T) {
  fmt.Printf("%s", t)  // is this a mismatching verb? t might be a fmt.Stringer.
}

// Example 2
func _[T any](t T) {
  fmt.Printf("%d", t)  // t might be integral, or a fmt.Formatter
}

// Example 3
func _[T int|float64](t T) {
  fmt.Printf(“%s”, t)  // this is provably an error
}

In all of these examples, there are elements of the type parameter type set that do not match the printf verb being used, but in Examples 1 and 2 there may be false positives:

  • In Example 1, the type argument must have int underlying, which doesn’t match %s, but we don’t know if it implements fmt.Stringer or fmt.Formatter.
  • In Example 2, the type argument can be any type, and we don’t know if it is integral, or a fmt.Formatter.
  • In Example 3, the type set is finite and we can prove that no type argument matches %s.

In the future we may want to report diagnostics in all three examples, but in the interest of caution I think we should allow all three for 1.18. Vet has very high standards for avoiding false positives, and in Examples 1 and 2, the following advice about interfaces from the printf source applies:

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

We could reasonably produce a diagnostic just for Example 3, but I think it will probably be uncommon to have a finite type set, and it is OK to miss this true positive in the interest of simplifying the implementation.

The counter argument to doing nothing is that with the new embedded elements in interfaces, we can now express type sets more precisely and it is more likely that a mismatching verb is actually a user error. There is some precedent here, in both directions:

This counter argument is compelling, and I think we should consider adding these diagnostics for 1.19 -- in fact I prototyped this in CL 351832 -- but for 1.18 in particular I am concerned that we don’t have a good understanding of how generics will be used, particularly when updating existing code, and having opinionated vet checks here may get in the way.

Regardless, I think this needs to be discussed (hence the issue).

CC @timothy-king @scott-cotton @mvdan

@bcmills
Copy link
Contributor

bcmills commented Oct 14, 2021

In my opinion:

  • Example 1 should emit a warning, because the type parameter may be instantiated with a type that does not implement fmt.Stringer, and for such parameters the format string is wrong.
  • Similarly for example 2. The warning should be suppressed only if every valid instantiation of T matches the formatting verb.

While it is true that “the user may have reasonable prior knowledge of the contents of an interface”, it is also the case that type sets can provide much more precision than interface types, and I think vet should make use of that extra precision.

@mvdan
Copy link
Member

mvdan commented Oct 14, 2021

@bcmills from the user's point of view I agree with you: I'd like to be warned about these cases, to catch real bugs or be nudged towards writing slightly more defensive code. However, I understand that would go against the precedent set by how interfaces are treated conservatively, which I think is following vet's rule to avoid false positives.

In any case, in the context of 1.18, I fully agree that we should keep the printf analyzer conservative. I already think 1.18 may be late with all that we're trying to include in it. Even if we agree to add type parameter warnings to printf, I think those should go into 1.19.

@bcmills
Copy link
Contributor

bcmills commented Oct 14, 2021

On the flip-side, type sets are not yet as precise as they ought to be. If I understand #45346 (comment) correctly (@griesemer), the type constraint in this example would today be rejected by the compiler:

type StringFormattable interface {
	~string | ~[]byte | error | fmt.Stringer
}

func _[T StringFormattable](t T) {
  fmt.Printf("%s\n", t)  // t is definitely string-formattable, but it may or may not implement fmt.Stringer.
}

@findleyr
Copy link
Contributor Author

@bcmills I think this is the counter argument here:

The counter argument to doing nothing is that with the new embedded elements in interfaces, we can now express type sets more precisely

In fact my first draft of this issue drew the exact conclusion you suggest: we should warn whenever there are any elements of the type parameter type set that doesn't match the verb. However, after some discussion and looking for precedent, I think moving slowly may be wise here.

@findleyr
Copy link
Contributor Author

If I understand #45346 (comment) correctly

Your understanding is correct, and that's a great example, thanks.

@findleyr
Copy link
Contributor Author

Another consideration: we can include the prototyped additions to the printf analyzer in gopls now. This will allow developers using gopls to benefit from the additional type safety and will give us some experience with the check.

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 15, 2021
@toothrot toothrot added this to the Backlog milestone Oct 15, 2021
@neild
Copy link
Contributor

neild commented Oct 15, 2021

It’s easier to relax restrictions than it is to add them. A strict check added now will definitely not affect any existing code, and can be adjusted or removed if it produces false positives.

When it comes to warnings for dubious use of type parameters, I think it will be better to err on the side of more checks to begin with.

@findleyr findleyr modified the milestones: Backlog, Go1.18 Oct 15, 2021
@findleyr
Copy link
Contributor Author

@bcmills @neild I can't say I disagree. I think adding a check should go through the proposal process though; I'll file an issue. I don't think this absolutely has to get done for 1.18.

@findleyr
Copy link
Contributor Author

findleyr commented Jan 5, 2022

Closing this as the associated proposal was accepted and implemented.

@findleyr findleyr closed this as completed Jan 5, 2022
@golang golang locked and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants