Skip to content
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

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

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

dominiquelefevre
Copy link
Contributor

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.

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
Copy link
Contributor Author

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

@chavacava
Copy link
Collaborator

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

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

Thanks @dominiquelefevre !

@dominiquelefevre
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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,

  1. capturing loop variables directly no longer introduces aliasing.
  2. 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
Copy link
Contributor

Thank you @dominiquelefevre it's way clearer now.

@ldez
Copy link
Contributor

ldez commented Jun 2, 2024

@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
Copy link
Contributor

ldez commented Jun 2, 2024

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
Copy link
Contributor

ldez commented Jun 3, 2024

I will open a dedicated issue. -> #995

@yurrriq
Copy link

yurrriq commented Jul 31, 2024

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

yurrriq added a commit to yurrriq/nixpkgs that referenced this pull request Aug 1, 2024
@echoix
Copy link
Contributor

echoix commented Aug 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants