Add support for the new behaviour of for loops in go 1.22. #993

dominiquelefevre · 2024-05-28T14:36:11Z

Go 1.22 has changed the behaviour of for loops. Every iteration makes new loop variables. It is now safe to take their addresses because they are guaranteed to be unique. Similarly, it is now safe to capture loop variables in functions.

dominiquelefevre · 2024-05-29T15:54:51Z

Hi, is anybody out here to take a look at the patch? @denisvmedia, @chavacava?

chavacava · 2024-05-30T07:55:04Z

Hi @dominiquelefevre, thanks for the PR !
I' ll try to review before the weekend.

chavacava · 2024-06-02T09:56:10Z

Thanks @dominiquelefevre !

dominiquelefevre · 2024-06-02T10:07:51Z

@chavacava could you please tag a new release so that I can make a PR to golangci-lint to use the up-to-date revive?

ccoVeille · 2024-06-02T11:56:50Z

Let me step out the data race fix for a moment and go back to the initial needs described in the ticket

How different is the feature brought by this PR from https://github.com/karamaru-alpha/copyloopvar created by @karamaru-alpha

And added to golangci-lint via golangci/golangci-lint#4382

I'm glad so see it added to revive if it's the same feature but it could have consequence for copyloopvar or golangci projects maybe

dominiquelefevre · 2024-06-02T12:10:53Z

Before go 1.22, all iterations of a loop shared their loop variables. If you created a function inside the loop, all those functions would capture the same one variable. Hence, if you iterated over a collection and wanted to create a function for every item in a collection, you needed to make copies of loop variables manually.

Starting with 1.22, the go compiler does that for you. Thus,

capturing loop variables directly no longer introduces aliasing.
making copies manually is no longer needed,

This PR addresses 1 and removes false reports about errors introduced by aliasing. Copyloopvar addresses 2 and warns about extraneous copies.

ccoVeille · 2024-06-02T14:21:34Z

Thank you @dominiquelefevre it's way clearer now.

ldez · 2024-06-02T23:31:32Z

@chavacava This PR will create a performance regression because go list is expensive and will be called on each package (even when range-val-address is not used).

"Funny thing", the regression impacts all the rules except range-val-address because this rule is skipped with go1.22.

It will be a performance regression for revive and for golangci-lint.
For golangci-lint, a way to define the Go version externally to bypass the go list is a solution.
I don't know enough about the revive design to propose a solution for revive.

We encountered the same problem with gosec golangci/golangci-lint#4735

log the call to detectGoVersion on revive's code

$ cd revive
$ ./revive ./...
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
rule/datarace.go:83:3: var getIds should be getIDs
lint/file.go:191:22: parameter 'filename' seems to be unused, consider removing or renaming it as _

benchmark on kubernetes

# sample.toml
ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0

#[rule.range-val-address]
[rule.context-as-argument]

$ cd kubernetes
$ rm go.work go.work.sum
$ hyperfine './revive-bbe5eb7  -config sample.toml ./...' './revive-4242f24  -config sample.toml ./...'
Benchmark 1: ./revive-bbe5eb7  -config sample.toml ./...
  Time (mean ± σ):      1.989 s ±  0.125 s    [User: 11.202 s, System: 1.519 s]
  Range (min … max):    1.796 s …  2.220 s    10 runs
 
Benchmark 2: ./revive-4242f24  -config sample.toml ./...
  Time (mean ± σ):      3.880 s ±  0.171 s    [User: 22.235 s, System: 14.092 s]
  Range (min … max):    3.633 s …  4.193 s    10 runs
 
Summary
  ./revive-bbe5eb7  -config sample.toml ./... ran
    1.95 ± 0.15 times faster than ./revive-4242f24  -config sample.toml ./...

$ cd revive
$ git lg
* 4242f24 - Add support for the new implementation of for loop variables in go 1.22. (#993)
* bbe5eb7 - fix(deps): update module github.com/burntsushi/toml to v1.4.0 (#992)
...

With the commit, the run of context-as-argument (just an example) is about 2x slower than the previous commit (and about 10x slower on the system).

ldez · 2024-06-02T23:59:43Z

Also, the implementation has a bug with Go workspaces:

can't parse the output of go list: invalid character '{' after top-level value

Inside a Go workspace, go list always returns all the modules, not just the current module.

As a reference, a working implementation: https://github.com/golangci/modinfo/blob/main/module.go

$ cd kubernetes
$ go list -m -json                                
{
        "Path": "k8s.io/kubernetes",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/api",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/api",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/api/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apiextensions-apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiextensions-apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiextensions-apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apimachinery",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apimachinery",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apimachinery/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cli-runtime",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cli-runtime",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cli-runtime/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/client-go",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/client-go",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/client-go/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cloud-provider",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cloud-provider",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cloud-provider/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cluster-bootstrap",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cluster-bootstrap",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cluster-bootstrap/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/code-generator",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/code-generator",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/code-generator/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/component-base",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-base",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-base/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/component-helpers",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-helpers",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-helpers/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/controller-manager",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/controller-manager",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/controller-manager/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cri-api",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-api",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-api/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cri-client",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-client",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-client/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/csi-translation-lib",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/csi-translation-lib",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/csi-translation-lib/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/dynamic-resource-allocation",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/dynamic-resource-allocation",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/dynamic-resource-allocation/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/endpointslice",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/endpointslice",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/endpointslice/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kms",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kms",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kms/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-aggregator",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-aggregator",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-aggregator/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-controller-manager",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-controller-manager",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-controller-manager/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-proxy",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-proxy",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-proxy/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-scheduler",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-scheduler",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-scheduler/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kubectl",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubectl",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubectl/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kubelet",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubelet",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubelet/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/metrics",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/metrics",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/metrics/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/mount-utils",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/mount-utils",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/mount-utils/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/pod-security-admission",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/pod-security-admission",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/pod-security-admission/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-cli-plugin",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-cli-plugin",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-cli-plugin/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-controller",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-controller",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-controller/go.mod",
        "GoVersion": "1.22.0"
}

FYI, the issue comment (golang/go#44753 (comment)), used as a reference inside the PR, is outdated since Go workspace (go1.18).

ldez · 2024-06-03T00:59:00Z

I will open a dedicated issue. -> #995

yurrriq · 2024-07-31T18:12:35Z

This change requires go on the path at runtime, which should probably be noted in the README and/or release notes. I know most users will have go at runtime, but this effectively broke the Nix package.

linting - retrieving failures channel: command go list: exec: "go": executable file not found in $PATH

This is needed due to mgechev/revive#993.

echoix · 2024-08-15T22:08:25Z

This change requires go on the path at runtime, which should probably be noted in the README and/or release notes. I know most users will have go at runtime, but this effectively broke the Nix package.

linting - retrieving failures channel: command go list: exec: "go": executable file not found in $PATH

Ah, thanks @yurrriq for this, now I understand why the Megalinter builds (that include revive) failed when trying to update the images with a version past v1.3.7. It’s a shame that this dependency is added, it was running as an independent executable that could be copied as is in docker.

dominiquelefevre mentioned this pull request May 28, 2024

False positive datarace detection in Go 1.22 #975

Closed

adds documentation for public function

48894a3

chavacava merged commit 4242f24 into mgechev:master Jun 2, 2024
4 checks passed

ldez mentioned this pull request Jun 3, 2024

Regressions related to 4242f24 #995

Closed

yurrriq mentioned this pull request Jul 31, 2024

revive: 1.3.7 -> 1.3.9 NixOS/nixpkgs#329700

Merged

yurrriq mentioned this pull request Jul 31, 2024

revive: ensure go on PATH, re-enable tests NixOS/nixpkgs#331346

Merged

13 tasks

yurrriq added a commit to yurrriq/nixpkgs that referenced this pull request Aug 1, 2024

revive: ensure go is on the PATH at runtime

38cfa2e

This is needed due to mgechev/revive#993.

echoix mentioned this pull request Aug 15, 2024

chore(deps): update dependency mgechev/revive to v1.3.9 oxsecurity/megalinter#3813

Merged

1 task

chavacava mentioned this pull request Aug 16, 2024

extract go version without depending on go list #1025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the new behaviour of for loops in go 1.22. #993

Add support for the new behaviour of for loops in go 1.22. #993

dominiquelefevre commented May 28, 2024

dominiquelefevre commented May 29, 2024

chavacava commented May 30, 2024

chavacava commented Jun 2, 2024

dominiquelefevre commented Jun 2, 2024

ccoVeille commented Jun 2, 2024

dominiquelefevre commented Jun 2, 2024

ccoVeille commented Jun 2, 2024

ldez commented Jun 2, 2024 •

edited

Loading

ldez commented Jun 2, 2024 •

edited

Loading

ldez commented Jun 3, 2024 •

edited

Loading

yurrriq commented Jul 31, 2024

echoix commented Aug 15, 2024

Add support for the new behaviour of for loops in go 1.22. #993

Add support for the new behaviour of for loops in go 1.22. #993

Conversation

dominiquelefevre commented May 28, 2024

dominiquelefevre commented May 29, 2024

chavacava commented May 30, 2024

chavacava commented Jun 2, 2024

dominiquelefevre commented Jun 2, 2024

ccoVeille commented Jun 2, 2024

dominiquelefevre commented Jun 2, 2024

ccoVeille commented Jun 2, 2024

ldez commented Jun 2, 2024 • edited Loading

ldez commented Jun 2, 2024 • edited Loading

ldez commented Jun 3, 2024 • edited Loading

yurrriq commented Jul 31, 2024

echoix commented Aug 15, 2024

ldez commented Jun 2, 2024 •

edited

Loading

ldez commented Jun 2, 2024 •

edited

Loading

ldez commented Jun 3, 2024 •

edited

Loading