-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
transport/cache: statically assert that tlsCacheKey is comparable #112680
transport/cache: statically assert that tlsCacheKey is comparable #112680
Conversation
@@ -0,0 +1,22 @@ | |||
//go:build go1.18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this a new file with a build constraint in case someone was using an older go version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we force the go version too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This felt like a "nicer" approach since this check is meant for the Kube devs to keep this stuff from drifting. I am sure over time we will pick up features from go 1.18 and thus naturally force library consumers to newer Go versions.
package transport | ||
|
||
// isComparable asserts at compile time that tlsCacheKey is comparable in a way that will never panic at runtime. | ||
func isComparable[T comparable]() { isComparable[tlsCacheKey]() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add an interface field to tlsCacheKey
, this will no longer compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be in a _test file and be equally effective?
also, is there a way to construct this that doesn't make a function that infinitely recursively calls itself, like
func isComparable[T comparable]() bool { return true }
var _ = isComparable[tlsCacheKey]()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be in a _test file and be equally effective?
We don't generally put interface assertions in test files because you only get a failure if you actually compile/run the test. Having it in the regular file makes it impossible to miss.
also, is there a way to construct this that doesn't make a function that infinitely recursively calls itself
Yeah, that is basically what I had originally and I got too clever with my desire to fit in in one line :) (also our CI linter doesn't like that the function is unused so I have to fix it anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't generally put interface assertions in test files
git grep "var _ = " | grep _test.go
disagrees :)
because you only get a failure if you actually compile/run the test.
Which we do, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't generally put interface assertions in test files
git grep "var _ = " | grep _test.go
disagrees :)
Hence I used generally. When we really want to make sure stuff does not drift we do it in the regular code like:
kubernetes/pkg/registry/core/service/storage/storage.go
Lines 70 to 76 in 664f0f5
var ( | |
_ rest.CategoriesProvider = &REST{} | |
_ rest.ShortNamesProvider = &REST{} | |
_ rest.StorageVersionProvider = &REST{} | |
_ rest.ResetFieldsStrategy = &REST{} | |
_ rest.Redirector = &REST{} | |
) |
because you only get a failure if you actually compile/run the test.
Which we do, right?
Why push an assertion down to a later time?
Signed-off-by: Monis Khan <mok@microsoft.com>
5afe3af
to
a6d0f48
Compare
/retest |
/lgtm TIL you can force this check at build time |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I can't tell if the go1.20 changes make it possible for this to fail at runtime: from https://tip.golang.org/doc/go1.20#language
|
We need to tweak our code to keep the static check: golang/go#56548 (comment) |
I assume that's something we can do now in master ahead of go1.20? |
Yeah I just had not gotten around to it yet. |
Signed-off-by: Monis Khan mok@microsoft.com
/kind cleanup
/triage accepted
/priority backlog
/assign @liggitt @aojea