compile-time assert numControllers == len(controllersArray) by syedriko · Pull Request #1112 · knative/eventing

syedriko · 2019-04-27T16:50:30Z

Replace a runtime check with a compile-time assert in two controller mainlines
Sister PR in serving knative/serving#3923

knative-prow-robot · 2019-04-27T16:50:44Z

Hi @syedriko. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

syedriko · 2019-04-27T16:57:55Z

/cc @evankanderson

n3wscott · 2019-04-29T14:48:07Z

I find this pretty hard to read. We have e2e tests that will catch the error.

grantr · 2019-04-29T16:23:48Z

/ok-to-test

grantr

This is clever and I like it. Thanks @syedriko! The var _ line is a bit hard to read but a comment can explain it adequately. I think it's worth having - relying on E2E tests to debug this problem would be extremely frustrating.

I have some minor suggestions to improve the clarity of the other lines.

grantr · 2019-04-29T16:30:40Z

 	// Add new controllers to this array.
 	// You also need to modify numControllers above to match this.
-	controllers := []*kncontroller.Impl{
+	controllersArray := [...]*kncontroller.Impl{


This might be clearer as [numControllers]*kncontroller.Impl. That will also catch the issue of numControllers < len(controllersArray).

Thanks, @grantr!
I picked the [...] form to make sure the length of the array comes strictly from the list of initializers. If we specify the length as you're suggesting, there's a risk of 0-valued elements at the end of the array if there are fewer initializers than the specified length inside of [].
The [numControllers - len(controllersArray)] part of the incantation covers the case of numControllers < len(controllersArray), as that makes the difference negative.

Makes sense, I forgot about the zero value being filled in at the end.

grantr · 2019-04-29T16:35:47Z

-	if len(controllers) != numControllers {
-		logger.Fatalf("Number of controllers and QPS settings mismatch: %d != %d", len(controllers), numControllers)
-	}
+	controllers := controllersArray[:]


Since this seems to only be used in the StartAll call below, we could inline this declaration there:

go kncontroller.StartAll(stopCh, controllersArray[:]...)

grantr · 2019-04-29T16:43:26Z

-		logger.Fatalf("Number of controllers and QPS settings mismatch: %d != %d", len(controllers), numControllers)
-	}
+	controllers := controllersArray[:]
+	// compile-time assert numControllers == len(controllersArray)


This would be clearer as a complete sentence. I suggest something like

// This line asserts at compile time that the length of controllersArray is equal to numControllers.

grantr · 2019-04-29T17:52:16Z

 	// Start all of the controllers.
 	logger.Info("Starting controllers.")
-	go kncontroller.StartAll(stopCh, controllers...)
+	go kncontroller.StartAll(stopCh, controllers[:]...)


Just FYI this will conflict with #1118 which is also yours :)

Yep, the cost of separating the two different issues :)

grantr

Thanks for fixing this in both serving and eventing @syedriko! Those who find it unbearably ugly now have motivation to fix the need to specify numControllers. 😺

/lgtm

vaikas · 2019-04-29T23:55:21Z

+	// var _ [N-M]int
+	// asserts at compile time that N >= M, which we can use to establish equality of N and M:
+	// (N >= M) && (M >= N) => (N == M)
+	var _ [numControllers - len(controllers)][len(controllers) - numControllers]int


: This is freaking awesome! <well, slighlty changed>

I'm wary of relying on something that needs a 5-line comment to explain a 1-liner technique for compile-ish time-ish bounds checking. I don't really understand how this works and I expect that like most folks, I will assume that it works and not notice when if it somehow doesn't.

I'd be more comfortable with runtime checking supported by a test (since Go's type system doesn't idiomatically encode these kinds of restrictions).

I'm wary of relying on something that needs a 5-line comment

I started with a one-liner comment, but was quickly (and correctly) prompted that an explanation is in order.

I don't really understand how this works and I expect that like most folks, I will assume
that it works and not notice when if it somehow doesn't.

There's no need to assume, I wouldn't want to:
https://golang.org/ref/spec#Array_types
The length is part of the array's type; it must evaluate to a non-negative constant representable by a value of type int.

var _ [N-M]int
(N - M >=0) <=> (N => M) <=> ((N > M) || (N == M))

var _ [M-N]int
(M - N >=0) <=> (M => N) <=> ((M > N) || (M == N))

var _ [N-M][M-N]int
For what N and M
((N > M) || (N == M)) && ((M > N) || (M == N)) == true?

For integers M and N, there're only 3 ordering possibilities:

N > M. Substitute:
(true || false) && (false || false) == false

M > N
(false || false) && (true || false) == false

N == M
The only alternative left. But still substitute:
(false || true) && (false || true) == true

compile-ish time-ish

I'm not familiar with that program translation phase. At the present this is a compile-time check. Even if somehow in some Go implementation far, far away that line is evaluated at runtime, it will still fail and we're going to be no worse off than we started.

The earlier a problem is detected, the quicker it can be fixed. It'd be much more productive to find a way to make that assert unnecessary in the first place.

vaikas · 2019-04-29T23:56:40Z

/lgtm
/approve

knative-prow-robot · 2019-04-29T23:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, syedriko, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [grantr,vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

syedriko · 2019-04-30T14:11:34Z

/test pull-knative-eventing-integration-tests

Remove echo statements, add -x flag for verbosity on jenkins runs

…5 updates (knative#1112) * [release-v1.16][gomod]: Bump the patch group across 1 directory with 5 updates Bumps the patch group with 2 updates in the / directory: [k8s.io/api](https://github.com/kubernetes/api) and [k8s.io/apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver). Updates `k8s.io/api` from 0.30.3 to 0.30.10 - [Commits](kubernetes/api@v0.30.3...v0.30.10) Updates `k8s.io/apiextensions-apiserver` from 0.30.3 to 0.30.10 - [Release notes](https://github.com/kubernetes/apiextensions-apiserver/releases) - [Commits](kubernetes/apiextensions-apiserver@v0.30.3...v0.30.10) Updates `k8s.io/apimachinery` from 0.30.3 to 0.30.10 - [Commits](kubernetes/apimachinery@v0.30.3...v0.30.10) Updates `k8s.io/apiserver` from 0.30.3 to 0.30.10 - [Commits](kubernetes/apiserver@v0.30.3...v0.30.10) Updates `k8s.io/client-go` from 0.30.3 to 0.30.10 - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.30.3...v0.30.10) --- updated-dependencies: - dependency-name: k8s.io/api dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apiextensions-apiserver dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apimachinery dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/apiserver dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch - dependency-name: k8s.io/client-go dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch ... Signed-off-by: dependabot[bot] <support@github.com> * Run make generate-release --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

compile-time assert numControllers == len(controllersArray)

fa6501c

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 27, 2019

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 27, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 27, 2019

knative-prow-robot requested review from evankanderson and matzew April 27, 2019 16:50

This was referenced Apr 27, 2019

compile-time assert numControllers == len(controllersArray) knative/serving#3923

Closed

compile-time assert numControllers == len(controllersArray) knative/serving#3924

Merged

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2019

grantr reviewed Apr 29, 2019

View reviewed changes

syedriko added 2 commits April 29, 2019 13:33

Incorporated review feedback from @grantr

8570573

Typo fix

0f87900

grantr reviewed Apr 29, 2019

View reviewed changes

Merge branch 'master' into compile_time_assert

6761c27

grantr approved these changes Apr 29, 2019

View reviewed changes

knative-prow-robot assigned grantr Apr 29, 2019

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 29, 2019

vaikas reviewed Apr 29, 2019

View reviewed changes

knative-prow-robot assigned vaikas Apr 29, 2019

knative-prow-robot merged commit c67c680 into knative:master Apr 30, 2019

syedriko deleted the compile_time_assert branch April 30, 2019 14:36

lberk added a commit to lberk/eventing that referenced this pull request Mar 2, 2021

Merge pull request knative#1112 from lberk/bash-flag-mirror

a736c6a

Remove echo statements, add -x flag for verbosity on jenkins runs

Conversation

syedriko commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot commented Apr 27, 2019

Uh oh!

syedriko commented Apr 27, 2019

Uh oh!

n3wscott commented Apr 29, 2019

Uh oh!

grantr commented Apr 29, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syedriko Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syedriko May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas commented Apr 29, 2019

Uh oh!

knative-prow-robot commented Apr 29, 2019

Uh oh!

syedriko commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

syedriko commented Apr 27, 2019 •

edited

Loading

syedriko Apr 29, 2019 •

edited

Loading

syedriko May 3, 2019 •

edited

Loading