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: add test for use of constraints package in way that is not forward compatible #48365

Open
ianlancetaylor opened this issue Sep 13, 2021 · 8 comments

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 13, 2021

In the discussion of the constraints package @benjaminjkraft raised an issue (#47319 (comment)). This kind of code:

type MySigned interface { type int8, int16, int32, int64 }
func MyAbs[T MySigned](v T) T { ... }
func Abs[T constraints.Signed](v T) T { return MyAbs(v) }

will fail to compile if we add a new type to constraints.Signed, because the new type will not be accepted by the MySigned constraint. However, we want to be able to add new types to the constraints package if we add new predeclared types to the language. That is, this bit of code is not forward compatible with possible future changes to the constraints package.

We can avoid such difficulties by adding vet checks for the following:

  1. Given a type parameter whose constraint is taken from the constraints package, don't use that type parameter to instantiate a type/function whose constraint is not any and is not taken from the constraints package.
  2. Don't write a type switch to handle all types when using a constraint taken from the constraints package.

This issue is to add those vet checks.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 13, 2021

Don't write a type switch to handle all types when using a constraint taken from the constraints package.

How would you detect that? That is, how can you tell that the user is trying to handle all types?

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Sep 14, 2021

How would you detect that? That is, how can you tell that the user is trying to handle all types?

Look for a type switch on a variable constrained by a constraint from the constraints package. Give an error if all the types in the current constraints implementation appear as cases and there is no default case.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 14, 2021

What if there is a default case that panics? log.Fatalfs?
What if the switch explicitly lists n-1 cases and they use default for the nth case?
There are probably other permutations also.

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Sep 14, 2021

We can't be perfect, but we can do better than nothing.

@robpike
Copy link
Contributor

@robpike robpike commented Sep 14, 2021

But vet needs to be close to perfect. "Better than nothing" is not good enough.

@zpavlinovic
Copy link

@zpavlinovic zpavlinovic commented Sep 14, 2021

My concern is that this vet checker would not find bugs in the user code per se, yet perhaps issues in the design. The current code is not buggy they way I see it.

So the checker findings IMO could be interpreted as warnings saying if new types are added to the constraints package in the future and you do not refactor your code in the meantime, then you'll see a compilation error. Not sure if this is what a vet checker should do.

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Sep 15, 2021

But vet needs to be close to perfect. "Better than nothing" is not good enough.

Fair enough. We may have to drop the type switch part of this idea.

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Sep 15, 2021

@zpavlinovic I see this as similar to the current vet warning about using an unkeyed composite literal with a struct type defined in a different package. It warns about code that is correct at the moment but may break in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants