denisvmedia
commented
4 months ago @ldez thanks for your report!
@chavacava should we revert it?
cc @dominiquelefevre
Closed ldez closed 3 months ago
denisvmedia
commented
4 months ago @ldez thanks for your report!
@chavacava should we revert it?
cc @dominiquelefevre
dominiquelefevre
commented
4 months ago 
ldez
commented
4 months ago From the golangci-lint POV, we need an option to set the Go version instead of detecting it inside revive.
dominiquelefevre
commented
3 months ago Ping @denisvmedia, @chavacava, @ldez.
dominiquelefevre
commented
3 months ago Hey, is anybody here to read the PR that fixes this issue? @denisvmedia @chavacava @ldez ?
PR #993 creates a performance regression because
go listis expensive and will be called on each package (even whenrange-val-addressis not used)."Funny thing", the regression impacts all the rules except
range-val-addressbecause 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 listis a solution. I don't know enough about the revive design to propose a solution for revive.We encountered the same problem with gosec https://github.com/golangci/golangci-lint/issues/4735
log the call to detectGoVersion on revive's code
```console $ 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
```toml # sample.toml ignoreGeneratedHeader = false severity = "warning" confidence = 0.8 errorCode = 0 warningCode = 0 #[rule.range-val-address] [rule.context-as-argument] ``` ```console $ 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 ./... ``` ```console $ 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).Also, the implementation has a bug with Go workspaces:
Inside a Go workspace,
go listalways returns all the modules, not just the current module.As a reference, a working implementation: https://github.com/golangci/modinfo/blob/main/module.go
FYI, the issue comment (https://github.com/golang/go/issues/44753#issuecomment-790089020), used as a reference inside the PR, is outdated since Go workspace (go1.18).