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

syedriko · 2019-04-27T17:13:11Z

Replace a runtime check with a compile-time assert in controller mainline
Sister PR in eventing knative/eventing#1112

knative-prow-robot · 2019-04-27T17:13:25Z

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.

vagababov · 2019-04-27T17:31:01Z

/ok-to-test

vagababov · 2019-04-27T17:35:12Z

While this does nicely exercise the language gymnastics, I am not a big fan of those checks because Go specifically has no assert by design: https://golang.org/doc/faq#assertions
And this just circumvents the language design.

syedriko · 2019-04-27T18:27:22Z

My motivation was in early detection of a violation, instead of leaving it until runtime. I don't think this check is very different from the very widespread one, that checks for interface conformance, like here

serving/pkg/apis/serving/v1beta1/revision_types.go

Line 47 in f5dde75

// Verify that Revision adheres to the appropriate interfaces.

Go specifically has no assert by design: https://golang.org/doc/faq#assertions

That paragraph is a bit too generic IMO, it seems like it's talking about an equivalent of assert() from C/C++, which can be pretty evil. The Go language spec is fine with type assertions though https://golang.org/ref/spec#Type_assertions.

IMO, the API in this area needs to be reworked to eliminate the circular dependency with the number of controllers and remove the need for this check in the first place.

vagababov · 2019-04-27T18:37:59Z

Well, type assertion is just a downcast :-)
Anyway, I am not strictly against it, though I am unsure how much there's benefit vs readability loss.
I'll let others decide.

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

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

syedriko · 2019-04-29T17:50:39Z

@n3wscott, could you take another look?

grantr · 2019-04-29T18:45:25Z

I approved the equivalent change in knative/eventing#1112. The "assertion" is unusual but good insurance to avoid debugging an incorrect numControllers using E2E tests. That could take hours. With the expanded comment added in 5925a8d, the purpose of this line is easy to understand.

We are already circumventing language design every time we assert an interface type and every time we use interface{} 😉

I agree with @syedriko that the real fix is to remove the need to specify numControllers. Anyone who finds this line unbearably ugly can work on that :)

syedriko · 2019-05-02T14:29:17Z

/cc @markusthoemmes , could you take a look at this?

markusthoemmes

I agree with @grantr

/approve

Let's let it burn our eyes until we decide to fix it for good. I don't want to overrule the other opinions here though.

@vagababov strong opinion on your end?

markusthoemmes · 2019-05-02T14:40:31Z

Hmm I can't approve that. doh.

/lgtm

then!

syedriko · 2019-05-02T15:23:57Z

/cc @tcnghia, what's your take?

matzew · 2019-05-03T13:09:15Z

/lgtm

dprotaso · 2019-05-03T14:00:33Z

 	// Build all of our controllers, with the clients constructed above.
 	// Add new controllers to this array.
-	controllers := []*controller.Impl{
+	controllers := [...]*controller.Impl{


If we're only concerned about the upper bound then you could just have this line be[numControllers]*controller.Impl

Then you can drop the compile time assertion below - since the compiler will complain if this array has more that numController elements

ie. https://play.golang.org/p/Q3Coj5TecnI

But it won't if there're less: knative/eventing#1112 (comment)

markusthoemmes

/approve
/lgtm

knative-prow-robot · 2019-05-29T08:39:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, syedriko

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:

~~cmd/controller/OWNERS~~ [markusthoemmes]

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

markusthoemmes · 2019-05-29T08:57:45Z

/test pull-knative-serving-upgrade-tests

@grantr

…#3924) * compile-time assert numControllers == len(controllersArray) * Incorporated review feedback from @grantr

@grantr

…#3924) * compile-time assert numControllers == len(controllersArray) * Incorporated review feedback from @grantr

syedriko added 2 commits April 27, 2019 12:56

compile-time assert numControllers == len(controllersArray)

09134c8

Merge branch 'master' into compile-time-assert

8b5cb66

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

knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 27, 2019

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/API API objects and controllers labels Apr 27, 2019

knative-prow-robot requested review from dgerd and dprotaso April 27, 2019 17:13

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 27, 2019

Merge branch 'master' into compile_time_assert

6dd42eb

syedriko added 2 commits April 29, 2019 13:43

Merge branch 'master' into compile_time_assert

4a01be3

Incorporated review feedback from @grantr

5925a8d

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 29, 2019

markusthoemmes approved these changes May 2, 2019

View reviewed changes

knative-prow-robot assigned markusthoemmes May 2, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2019

knative-prow-robot assigned matzew May 3, 2019

dprotaso reviewed May 3, 2019

View reviewed changes

Merge branch 'master' into compile_time_assert

8db60f7

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 3, 2019

Merge branch 'master' into compile_time_assert

7ffdd89

markusthoemmes approved these changes May 29, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2019

knative-prow-robot merged commit 44d9045 into knative:master May 29, 2019

syedriko deleted the compile_time_assert branch May 29, 2019 15:32

JRBANCEL pushed a commit to JRBANCEL/serving that referenced this pull request May 29, 2019

compile-time assert numControllers == len(controllersArray) (knative…

69b8d54

…#3924) * compile-time assert numControllers == len(controllersArray) * Incorporated review feedback from @grantr

joshrider pushed a commit to joshrider/serving that referenced this pull request Jun 10, 2019

compile-time assert numControllers == len(controllersArray) (knative…

8b119dd

…#3924) * compile-time assert numControllers == len(controllersArray) * Incorporated review feedback from @grantr

Conversation

syedriko commented Apr 27, 2019

Uh oh!

knative-prow-robot commented Apr 27, 2019

Uh oh!

vagababov commented Apr 27, 2019

Uh oh!

vagababov commented Apr 27, 2019

Uh oh!

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

Uh oh!

Uh oh!

vagababov commented Apr 27, 2019

Uh oh!

n3wscott commented Apr 29, 2019

Uh oh!

syedriko commented Apr 29, 2019

Uh oh!

grantr commented Apr 29, 2019

Uh oh!

syedriko commented May 2, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented May 2, 2019

Uh oh!

syedriko commented May 2, 2019

Uh oh!

matzew commented May 3, 2019

Uh oh!

dprotaso May 3, 2019

Choose a reason for hiding this comment

Uh oh!

syedriko May 3, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 29, 2019

Uh oh!

markusthoemmes commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

syedriko commented Apr 27, 2019 •

edited

Loading