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

Closed
ianlancetaylor opened this issue Sep 13, 2021 · 11 comments
Labels
generics Issue is related to generics NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository. labels Sep 13, 2021
@ianlancetaylor ianlancetaylor added this to the Go1.18 milestone Sep 13, 2021
@ianlancetaylor ianlancetaylor added the generics Issue is related to generics label Sep 13, 2021
@randall77
Copy link
Contributor

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

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

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

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

@robpike
Copy link
Contributor

robpike commented Sep 14, 2021

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

@zpavlinovic
Copy link
Contributor

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

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

@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.

@ianlancetaylor
Copy link
Contributor Author

Moving to 1.19.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Go1.19 Jan 29, 2022
@ianlancetaylor
Copy link
Contributor Author

Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Backlog Jun 24, 2022
@ianlancetaylor
Copy link
Contributor Author

We decided against putting the constraints package into the standard library, so this issue is of little interest. Closing.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generics Issue is related to generics NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Development

No branches or pull requests

4 participants