golangci-lint icon indicating copy to clipboard operation
golangci-lint copied to clipboard

Support of generics

Open ldez opened this issue 3 years ago • 112 comments
trafficstars

Since the version v1.45.0 golangci-lint supports go1.18.

But some linters don't work with generics, golangci-lint disable those linters automatically with go1.18.

Since v1.45.1, golangci-lint can detect the Go version used by a project.

To get the Go version golangci-lint will use, in order:

  • the Go version defined by the CLI flag, ex: --go=1.18
  • the Go version defined in the configuration file, ex:
    run:
      go: '1.18'
    
  • the Go version defined in the go.mod, ex;
    module github.com/org/my-project
    
    go 1.18
    
  • Go version defined by the env var GOVERSION
  • fallback on go1.17.

Notes:

  • some rules in go-critic (externalErrorReassign) don't work with generics and must be disabled by hand
  • if you are not using generics (and your dependencies) you can use the following configuration:
    run:
      go: '1.17'
    

The keyword any works well and produces no error (it's expected because it's just a type alias)


The problem with SSA is now fixed but some linters need to update their code base to handle generics.

About the linters:

  • for go-critic, there is an existing issue https://github.com/go-critic/go-critic/issues/1193

Linter issues: (checked if the problems are solved)


The binary compiled with go1.17 doesn't work when running on go1.18:

$ golangci-lint run
panic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.22()
        github.com/go-critic/[email protected]/checkers/embedded_rules.go:46 +0x4b4


Compatibility table

🟢️: means that the linter seems to work, but it's not 100% sure, it needs to be tested on more go1.18 code. 🟠: means that the linter works partially. 🔴: means that the linter panic.

The current state:

Linter State Problem
asciicheck 🟢️
bidichk 🟢️
bodyclose 🟢️
containedctx 🟢️
contextcheck 🟢️
cyclop 🟢️
deadcode 🟢️
decorder 🟢️
depguard 🟢️
dogsled 🟢️
dupl 🟢️
durationcheck 🟢️
errcheck 🟢️
errchkjson 🟢️
errname 🟢️
errname 🟢️
errorlint 🟢️
exhaustive 🟢️
exhaustivestruct 🟢️
exportloopref 🟢️
forbidigo 🟢️
forcetypeassert 🟢️
funlen 🟢️
gci 🟢️
gochecknoglobals 🟢️
gochecknoinits 🟢️
gocognit 🟢️
goconst 🟢️
gocritic 🟠 type assertion
gocyclo 🟢️
godot 🟢️
godox 🟢️
goerr113 🟢️
gofmt 🟢️
gofumpt 🟢️
goheader 🟢️
goimports 🟢️
gomnd 🟢️
gomoddirectives 🟢️
gomodguard 🟢️
goprintffuncname 🟢️
gosec 🟢️
gosimple 🟢️
govet 🟢️
grouper 🟢️
ifshort 🟢️
importas 🟢️
ineffassign 🟢️
ireturn 🟢️
lll 🟢️
maintidx 🟢️
makezero 🟢️
misspell 🟢️
nakedret 🟢️
nestif 🟢️
nilerr 🟢️
nilnil 🟢️
nlreturn 🟢️
noctx 🟢️
nolintlint 🟢️
paralleltest 🟢️
prealloc 🟢️
predeclared 🟢️
promlinter 🟢️
revive 🟢️
rowserrcheck 🟠 some elements are not detected
sqlclosecheck 🟠 some elements are not detected
staticcheck 🟢️
structcheck 🟠 false positives
stylecheck 🟢️
tagliatelle 🟢️
tenv 🟢️
testpackage 🟢️
thelper 🟢️
tparallel 🟢️
typecheck 🟢️
unconvert 🟢️
unparam 🟢️
unused 🟢️
varcheck 🟢️
varnamelen 🟢️
wastedassign 🟠 some elements are not detected
whitespace 🟢️
wrapcheck 🟢️
wsl 🟢️

ldez avatar Mar 16 '22 11:03 ldez

I think we can pin this issue and lists all issues related to Go 1.18 support.

ipfans avatar Mar 16 '22 13:03 ipfans

If someone want to help, you can contribute to solving this issue https://github.com/golang/go/issues/48525

ldez avatar Mar 16 '22 13:03 ldez

In our project after go version update to 1.18, the CI detected a lot of typecheck error. It is unable to load the types from ginkgo and also detects this module as unused:

"github.com/onsi/ginkgo" imported but not used (typecheck)
. "github.com/onsi/ginkgo"
undeclared name: `Describe` (typecheck)
var _ = Describe("Authenticator Unit Tests", func() {
        ^

vmatyus avatar Mar 16 '22 14:03 vmatyus

thats strange, I'm able to build golangci-lint with go1.18 without any issues however, I'm unable to run previously installed (before update to go 1.18) rpm :thinking:

$ /usr/local/bin/golangci-lint run ./...
panic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.22()
	github.com/go-critic/[email protected]/checkers/embedded_rules.go:46 +0x4b4

after building linter by myself:

[giu (1) ]$ cd /tmp/golangci-lint/
[golangci-lint (0) ]$ go build -o golangci-lint cmd/golangci-lint/main.go 
[golangci-lint (0) ]$ cd ~/myproject
[myproject (0) ]$ /tmp/golangci-lint/golangci-lint run ./...
[myproject (0) ]$ 

gucio321 avatar Mar 16 '22 16:03 gucio321

@gucio321 take a look at #2438

ldez avatar Mar 16 '22 17:03 ldez

@gucio321 take a look at https://github.com/golangci/golangci-lint/pull/2438

yah I swa it, but cannot understand what's going on... what is a difference between linter from here an this build with go-build? and how to reproduce this issue?

gucio321 avatar Mar 16 '22 17:03 gucio321

Message for everyone

I'm working to find quick solutions (proposals and implementations), I will try to send a status update tomorrow.

I do my best, stay tuned.

ldez avatar Mar 16 '22 20:03 ldez

Status Update - Message for everyone

What I have tried:

  1. use builds tags
  2. add a noop system inside each linter with a global noop linter
  3. add a global way to apply a noop linter

The problems:

  1. with solution 1: don't allow go1.17 users to use linters that works for go1.17
  2. with solution 2: too much code and too much noise in the linters code and required to define the Go version manually
  3. with solution 3: required to define the Go version manually

Advantages:

  1. with solution 1: be able to have the same behavior for all Go version
  2. with solutions 2 and 3: be able to run all linters if you are using go1.17

Because of the good ratio between pros and cons, I choose to follow solution 3.

I didn't add an auto-detection of the Go version for now, because it requires time to be well tested, but I think that's possible to do.

So I added a new configuration option inside the run section. We will not have to remove this option in the future because we will be able to use it for some linters that have a Go version option.

An example:

run:
    go: 1.18

By default, the supported Go version will be go1.17 because it allows us to run all our tests without a huge rewrite.

If you set the version to go1.18, the following linters will be inactive:

  • bodyclose
  • contextcheck
  • gosimple
  • nilerr
  • noctx
  • rowserrcheck
  • sqlclosecheck
  • staticcheck
  • stylecheck
  • tparallel
  • unparam
  • unused
  • wastedassign

It's far from perfection:

  • the real fix is related to SSA in golang.org/x/tools
  • some analyzers of govet (nilness and unusedwrite) don't work with generics and must be disabled by hand
  • one rule in go-critic (hugeParam) doesn't work with generics and must be disabled by hand

Also, I can't guarantee that there are no hidden problems, I tested golangci-lint on a codebase containing different use of generics but this is not exhaustive.

I hope you will agree with my choice, if you see some other solutions you're welcome to share them with me.

PR #2438 contains the implementation of solution 3.

Note: if you are using go1.18 and your codebase (and the dependencies) doesn't contain uses of generics, you can set the Go version to go1.17.

ldez avatar Mar 17 '22 08:03 ldez

Status Update - Message for everyone

The new release (v1.45.0) is available, it contains the solution that I explained here.

You can read the PR description if you want to get a configuration example and some explanation about the limitation of this new version.

I will continue to follow and work on this topic.

Note for linter's creators: the implementation of SSA by the Go team can be abandoned, so it's better to not use it.

If someone wants to help, you can contribute to solving this issue https://github.com/golang/go/issues/48525

During this period, I learned that golangci-lint is used by more people than I was thinking, the community is very large.

I would like to say a big thank you to everyone that send me some :heart: and support.

Thank you :heart: and thank you for supporting FOSS maintainers :heart:.

ldez avatar Mar 18 '22 19:03 ldez

I think structcheck should be added to the list of linters disabled under 1.18, as it is falsely telling me that every private member of my generic structs is unused. This sounds like the same SSA issue as the others I think?

Edit: Big thank you for getting things this far so fast :grinning:

mgabeler-lee-6rs avatar Mar 18 '22 20:03 mgabeler-lee-6rs

@mgabeler-lee-6rs can you provide a code example?

ldez avatar Mar 18 '22 21:03 ldez

@ldez wal-g is a code example https://github.com/wal-g/wal-g/pull/1186 structcheck was complaining about LazyCache's fields

(PS thanks for the help)

serprex avatar Mar 18 '22 23:03 serprex

I was able to reproduce the problem with structcheck:

snippet
package sample

import "fmt"

type Foo[K comparable] struct {
	bar K
}

func (l *Foo[K]) Load() {
	fmt.Println(l.bar)
}
`bar` is unused (structcheck)

So structcheck doesn't work well with generics, I think we will disable it for go1.18 in the next release.

ldez avatar Mar 19 '22 00:03 ldez

The keyword any seems not work with typecheck because I have this error:

undeclared name: `any` (typecheck)

Stranger thing even if I disable typecheck linter then this error still there.

jerome-laforge avatar Mar 19 '22 10:03 jerome-laforge

@jerome-laforge I don't reproduce your problem, can you provide information about your environment and a code example?

typecheck is not a real linter it's just a way to parse/display "compilation" and linters errors (linter reports are not errors), typecheck cannot be disabled because of that. So in your context, I think that you are not using v1.45.0.

ldez avatar Mar 19 '22 10:03 ldez

Sorry, I didn't give my context. I am using v1.45 but with docker golangci/golangci-lint:v1.45. If I run binary provided by aur on manjaro/archlinux Linux, I don't have the same error:

ERRO [runner] Panic: buildssa: package "api" (isInitialPkg: true, needAnalyzeSource: true): T: goroutine 22900 [running]:
runtime/debug.Stack()
        runtime/debug/stack.go:24 +0x65
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:101 +0x155
panic({0xf84220, 0xc00f76ca50})
        runtime/panic.go:838 +0x207
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc010c541a0, {0x1225b48?, 0xc00f76ca50?}, 0x0)
        golang.org/x/[email protected]/go/ssa/methods.go:237 +0x5b1
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc010c541a0, {0x1225940?, 0xc0217df5a8?}, 0x0)
        golang.org/x/[email protected]/go/ssa/methods.go:199 +0x4e8
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc010c541a0, {0x1225aa8?, 0xc019837580?}, 0x0)
        golang.org/x/[email protected]/go/ssa/methods.go:196 +0x347
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc010c541a0, {0x1225b20?, 0xc0217df5d8?}, 0x0)
        golang.org/x/[email protected]/go/ssa/methods.go:233 +0x708
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc010c541a0, {0x1225a80?, 0xc00db2b1c0?}, 0x0)
        golang.org/x/[email protected]/go/ssa/methods.go:209 +0x448
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc010c541a0, {0x1225a80?, 0xc00db2b1c0?})
        golang.org/x/[email protected]/go/ssa/methods.go:145 +0x70
golang.org/x/tools/go/ssa.(*Package).build(0xc0160b9020)
        golang.org/x/[email protected]/go/ssa/builder.go:2284 +0x111
sync.(*Once).doSlow(0xc010c541a0?, 0xc012c0a500?)
        sync/once.go:68 +0xc2
sync.(*Once).Do(...)
        sync/once.go:59
golang.org/x/tools/go/ssa.(*Package).Build(...)
        golang.org/x/[email protected]/go/ssa/builder.go:2272
golang.org/x/tools/go/analysis/passes/buildssa.run(0xc010c540d0)
        golang.org/x/[email protected]/go/analysis/passes/buildssa/buildssa.go:72 +0x2ee
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc003fe8530)
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:187 +0x9c4
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:105 +0x1d
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc004e49a90, {0xfed573, 0x8}, 0xc003813748)
        github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x4a
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc000ffc6c0?)
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:104 +0x85
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc003fe8530)
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0xb4
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
        github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x1eb 
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: buildssa: package "api" (isInitialPkg: true, needAnalyzeSource: true): T 
ERRO Running error: 1 error occurred:
        * can't run linter goanalysis_metalinter: goanalysis_metalinter: buildssa: package "api" (isInitialPkg: true, needAnalyzeSource: true): T

jerome-laforge avatar Mar 19 '22 14:03 jerome-laforge

@jerome-laforge it is easier for us to manage if you open a dedicated issue. I need more information (configuration, code example, etc.)

As I explained here and here, if you are using generics you have to set the following configuration:

run:
    go: 1.18

https://golangci-lint.run/usage/configuration/#run-configuration

ldez avatar Mar 19 '22 18:03 ldez

Maybe the root cause of this problem can be that docker golangci/golangci-lint:v1.45 is based on Go 1.17 and not Go 1.18 (https://github.com/golangci/golangci-lint/blob/master/build/Dockerfile & https://github.com/golangci/golangci-lint/blob/master/build/Dockerfile.alpine)

jerome-laforge avatar Mar 20 '22 15:03 jerome-laforge

@jerome-laforge But that shouldn't affect the GitHub action? That one seems to be using Go <1.18 as well?!

dominikschulz avatar Mar 20 '22 21:03 dominikschulz

But that shouldn't affect the GitHub action?

no, because the GitHub action uses the binary and not the Docker image.

I fixed the problem is https://github.com/golangci/golangci-lint/pull/2661

ldez avatar Mar 20 '22 21:03 ldez

Thx @ldez, is it possible to push this fixed image on docker hub?

jerome-laforge avatar Mar 21 '22 06:03 jerome-laforge

@jerome-laforge it is easier for us to manage if you open a dedicated issue. I need more information (configuration, code example, etc.)

As I explained here and here, if you are using generics you have to set the following configuration:

run:
    go: 1.18

https://golangci-lint.run/usage/configuration/#run-configuration

That's the best and assertive answer so far.

gandarez avatar Mar 22 '22 13:03 gandarez

I'm using this image golangci/golangci-lint:v1.45-alpine and I do have the configuration setting as described above:

run:
    go: 1.18

And still typecheck fails.

I assume that this is because the underlying go version is still 1.17 https://hub.docker.com/layers/golangci/golangci-lint/v1.45-alpine/images/sha256-dc5a9bc46a7bf9e87e6f8f25d70f60bdf9c22adde14458c2dcb712f763f0e0ed?context=explore

jozuenoon avatar Mar 22 '22 14:03 jozuenoon

@jozuenoon the problem is already reported https://github.com/golangci/golangci-lint/issues/2649#issuecomment-1073274033 and fixed #2661

ldez avatar Mar 22 '22 14:03 ldez

@ldez yes I saw it, but it begs for docker image update on Docker Hub?

jozuenoon avatar Mar 22 '22 14:03 jozuenoon

is it possible to push this fixed image on Docker Hub?

When we will create a new release, I hope this week.

ldez avatar Mar 22 '22 14:03 ldez

also waiting ✌️ ❤️

loeffel-io avatar Mar 22 '22 17:03 loeffel-io

A casual mention for everyone following this thread: @ldez has a GitHub sponsors

techknowlogick avatar Mar 23 '22 04:03 techknowlogick

Good hint :)

frzifus avatar Mar 23 '22 08:03 frzifus

is it possible to push this fixed image on Docker Hub?

When we will create a new release, I hope this week.

Thx @ldez for the push on docker hub. This image works fine now!

jerome-laforge avatar Mar 24 '22 13:03 jerome-laforge