Linter for incorrect var capture #104835

uthark · 2021-09-08T05:44:02Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Just enabling linter and fixing few flakes.
Thinking of a better way to introduce an allowlist for existing violations. Seems like golangci doesn't support it (See upstream issue golangci/golangci-lint#913)

Does this PR introduce a user-facing change?

No.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-09-08T05:44:10Z

Hi @uthark. Thanks for your PR.

I'm waiting for a kubernetes 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.

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.

omertuc · 2021-09-08T08:53:50Z

I'm not entirely qualified so take everything I say with a grain of salt, but the way I understand things is -

exportloopref may be an over-sensitive solution to this (#98213) problem. iiuc sometimes it's reasonable to pass &loopvar to a function to avoid a copy, it's not always a bad thing. The issues start when that function actually stores that address and have it be used after the loop var is already changed on the next iterations or completely gone.

Same thing can be said about closures capturing the loop var and then being ran only after the loop var is already changed on the next iterations - which is what happens with Ginkgo - it "collects" all the closures created by the loop and runs them only after the loop is done, and at that point the loop var contains the final iterable value, and thus the same value is being used by all the closures that were created. But in contrast, when using the built-in go testing package - there's a similar common pattern of creating test-case closures inside a loop with the closures referencing the loop var. But with the built-in go testing package this is (mostly*) not a problem because the closures get to run before the loop iteration is over. They're not being collected like in Ginkgo. So, if I understand correctly, exportloopref would incorrectly flag those legitimate uses as linter violations.

*I think the built in go testing package allows running tests in parallel, I think that may actually start to cause problems, not sure about this though

neolit123

SGTM, but up to the /hack maintainers to decide.

/ok-to-test

neolit123 · 2021-10-12T21:12:37Z

hack/.golangci.yaml

+  - unused
+  - varcheck
+  - ineffassign
+  - exportloopref


exportloopref seems to be a new addition?

neolit123 · 2021-10-12T21:13:50Z

hack/verify-golangci-lint.sh

-  -E unused \
-  -E varcheck \
-  -E ineffassign
+golangci-lint run --config "${KUBE_ROOT}/hack/.golangci.yaml"


my take: the benefit is minimal at this point, but having a config vs flags seems better in general.

neolit123 · 2021-10-12T21:17:52Z

test/utils/audit.go

@@ -121,6 +121,7 @@ func CheckAuditList(el auditinternal.EventList, expected []AuditEvent) (missing
 	expectations := newAuditEventTracker(expected)

 	for _, e := range el.Items {
+		e := e


these "add local var in a loop" changes seem fine.
great if there aren't more.

hm, we pass e by ref on line below. And this may result in bad behavior if testEventFromInternal will run any goroutine or will keep the ref for later use.

/hold

This may lead to an issue, #106371, is the change forced by linter?

that issue fixed exactly this problem, we have to capture the variable or the reference will only use the last value, or am I getting wrong your concern?

Oh, I read the PR backwards. Adding it makes a lot of sense. Sorry for the noise

neolit123 · 2021-10-12T21:20:42Z

/release-note-none

BenTheElder · 2021-11-09T19:37:05Z

hack/.golangci.yaml

@@ -0,0 +1,12 @@
+---


maybe we should put a comment at the top pointing to the upstream docs for this config format?

this got split out in another PR working on some other class of lints, upon rebase the change in this file should just be adding the new linter 😅

BenTheElder · 2021-11-09T19:52:47Z

/approve
/retest

BenTheElder · 2021-11-09T19:53:02Z

/triage accepted
/priority important-longterm

k8s-ci-robot · 2021-11-09T19:53:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, uthark

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:

~~hack/OWNERS~~ [BenTheElder]
~~test/OWNERS~~ [BenTheElder]

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

BenTheElder · 2021-11-09T19:53:49Z

xref another golangci PR that will have merge conflicts #103723

k8s-ci-robot · 2021-11-09T20:42:51Z

@uthark: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-verify	`703a107`	link	true	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

aojea · 2021-11-16T08:40:58Z

there are a few more issues that need to be fixed

running golangci-lint 
test/e2e/framework/pods.go:271:11: exporting a pointer for the loop variable e (exportloopref)
				ev = &e
				      ^
test/e2e/framework/util.go:1394:50: exporting a pointer for the loop variable watchEventIndex (exportloopref)
					foundEventIndexes[actualWatchEventIndex] = &watchEventIndex
					                                            ^
test/e2e/apimachinery/custom_resource_definition.go:207:15: exporting a pointer for the loop variable g (exportloopref)
					group = &g
					         ^
test/e2e/apimachinery/custom_resource_definition.go:217:17: exporting a pointer for the loop variable v (exportloopref)
					version = &v
					           ^
test/e2e/apimachinery/custom_resource_definition.go:235:17: exporting a pointer for the loop variable v (exportloopref)
					version = &v
					           ^
test/e2e/apimachinery/webhook.go:125:15: exporting a pointer for the loop variable g (exportloopref)
					group = &g
					         ^
test/e2e/apimachinery/webhook.go:135:17: exporting a pointer for the loop variable v (exportloopref)
					version = &v
					           ^
test/e2e/auth/service_accounts.go:568:43: exporting a pointer for the loop variable tc (exportloopref)
				pod.Spec.SecurityContext.RunAsUser = &tc.wantUID
				                                      ^
test/e2e/auth/service_accounts.go:571:41: exporting a pointer for the loop variable tc (exportloopref)
				pod.Spec.SecurityContext.FSGroup = &tc.wantGID
				                                    ^
test/e2e/autoscaling/cluster_size_autoscaling.go:1253:27: exporting a pointer for the loop variable node (exportloopref)
			nodes = append(nodes, &node)
			                       ^
test/e2e/storage/nfs_persistent_volume-disruptive.go:126:20: exporting a pointer for the loop variable node (exportloopref)
					clientNode = &node
					              ^
test/e2e/storage/volume_provisioning.go:971:42: exporting a pointer for the loop variable pv (exportloopref)
				remainingPVs = append(remainingPVs, &pv)
				                                     ^
pkg/proxy/userspace/proxier_test.go:117:19: exporting a pointer for the loop variable port (exportloopref)
			servicePort = &port
			               ^
plugin/pkg/admission/securitycontext/scdeny/admission_test.go:143:31: exporting a pointer for the loop variable test (exportloopref)
		pod.Spec.SecurityContext = &test.securityContext
		                            ^
test/e2e_node/restart_test.go:57:39: exporting a pointer for the loop variable pod (exportloopref)
			runningPods = append(runningPods, &pod)
			                                   ^
+++ exit code: 1
+++ error: 1
FAILED   verify-golangci-lint.sh	436s

k8s-ci-robot · 2021-11-17T22:49:48Z

@uthark: PR needs rebase.

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.

dims · 2022-01-14T13:51:44Z

this PR has been on hold for about 2 weeks. Can you please see how to unblock this by reviewing comments etc? or please close this out (Trying to declutter our open list of PRs)

dims · 2022-02-11T14:44:52Z

This PR is older than 4 weeks and needs as rebase.

Please rebase the PR against latest master and reopen if still needed.

/close

k8s-ci-robot · 2022-02-11T14:51:17Z

@dims: Closed this PR.

In response to this:

This PR is older than 4 weeks and needs as rebase.

Please rebase the PR against latest master and reopen if still needed.

/close

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.

Linter for incorrect var capture

703a107

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 8, 2021

k8s-ci-robot requested review from lavalamp and SataQiu September 8, 2021 05:45

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 8, 2021

neolit123 reviewed Oct 12, 2021

View reviewed changes

k8s-ci-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 Oct 12, 2021

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 12, 2021

BenTheElder reviewed Nov 9, 2021

View reviewed changes

BenTheElder mentioned this pull request Nov 9, 2021

Some "vectorized" tests never run due to bad closure var capture #98213

Open

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2021

BenTheElder mentioned this pull request Nov 10, 2021

Add staticcheck to verify-golangci #103723

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2021

k8s-ci-robot closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linter for incorrect var capture #104835

Linter for incorrect var capture #104835

uthark commented Sep 8, 2021

k8s-ci-robot commented Sep 8, 2021

omertuc commented Sep 8, 2021 •

edited

neolit123 left a comment

neolit123 Oct 12, 2021

neolit123 Oct 12, 2021

neolit123 Oct 12, 2021

SergeyKanzhelev Nov 15, 2021

SergeyKanzhelev Nov 15, 2021

aojea Nov 15, 2021

SergeyKanzhelev Nov 15, 2021

neolit123 commented Oct 12, 2021

BenTheElder Nov 9, 2021

BenTheElder Nov 18, 2021

BenTheElder commented Nov 9, 2021

BenTheElder commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

BenTheElder commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021 •

edited

aojea commented Nov 16, 2021

k8s-ci-robot commented Nov 17, 2021

dims commented Jan 14, 2022

dims commented Feb 11, 2022

k8s-ci-robot commented Feb 11, 2022

Linter for incorrect var capture #104835

Linter for incorrect var capture #104835

Conversation

uthark commented Sep 8, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 8, 2021

omertuc commented Sep 8, 2021 • edited

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Nov 9, 2021

BenTheElder commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

BenTheElder commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021 • edited

aojea commented Nov 16, 2021

k8s-ci-robot commented Nov 17, 2021

dims commented Jan 14, 2022

dims commented Feb 11, 2022

k8s-ci-robot commented Feb 11, 2022

omertuc commented Sep 8, 2021 •

edited

k8s-ci-robot commented Nov 9, 2021 •

edited