ginkgolinter crashes golangci-lint when enabled

[oscr]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc.).
• Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

When enabling ginkgolinter in the cluster-api project using v1.51.0 golangci-lint crashes.

git diff of the change needed to crash:

git diff
diff --git a/.golangci.yml b/.golangci.yml
index 986fe81eb..a01b22b66 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -25,6 +25,7 @@ linters:
   - errchkjson
   - exportloopref
   - gci
+  - ginkgolinter
   - goconst
   - gocritic
   - godot

I've tried go version 1.18.10, 1.19.5 and 1.20 with the same result.
I've also tried adding the default linter settings to see if that helps.
I've tested to run ginkgolinter downloaded as a stand alone from github and it seems to work for me.

You can recreate this by enabling ginkgolinter and running make lint in cluster-api project.

Version of golangci-lint

$ golangci-lint --version
v1.51.0

Configuration file

$ cat .golangci.yml
  - stylecheck
  - thelper
  - typecheck
  - unconvert
  - unparam
  - unused
  - usestdlibvars
  - whitespace

linters-settings:
  godot:
    #   declarations - for top level declaration comments (default);
    #   toplevel     - for top level comments;
    #   all          - for all comments.
    scope: toplevel
    exclude:
    - '^ \+.*'
    - '^ ANCHOR.*'
  gci:
    local-prefixes: "sigs.k8s.io/cluster-api"
  importas:
    no-unaliased: true
    alias:
      # Kubernetes
      - pkg: k8s.io/api/core/v1
        alias: corev1
      - pkg: k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1
        alias: apiextensionsv1
      - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
        alias: metav1
      - pkg: k8s.io/apimachinery/pkg/api/errors
        alias: apierrors
      - pkg: k8s.io/apimachinery/pkg/util/errors
        alias: kerrors
      - pkg: k8s.io/component-base/logs/api/v1
        alias: logsv1
      # Controller Runtime
      - pkg: sigs.k8s.io/controller-runtime
        alias: ctrl
      # CABPK
      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha3
        alias: bootstrapv1alpha3
      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha4
        alias: bootstrapv1alpha4
      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1
        alias: bootstrapv1
      # KCP
      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3
        alias: controlplanev1alpha3
      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4
        alias: controlplanev1alpha4
      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1
        alias: controlplanev1
      # CAPI
      - pkg: sigs.k8s.io/cluster-api/api/v1alpha3
        alias: clusterv1alpha3
      - pkg: sigs.k8s.io/cluster-api/api/v1alpha4
        alias: clusterv1alpha4
      - pkg: sigs.k8s.io/cluster-api/api/v1beta1
        alias: clusterv1
      # CAPI exp
      - pkg: sigs.k8s.io/cluster-api/exp/api/v1alpha3
        alias: expv1alpha3
      - pkg: sigs.k8s.io/cluster-api/exp/api/v1alpha4
        alias: expv1alpha4
      - pkg: sigs.k8s.io/cluster-api/exp/api/v1beta1
        alias: expv1
      # CAPI exp addons
      - pkg: sigs.k8s.io/cluster-api/exp/addons/api/v1alpha3
        alias: addonsv1alpha3
      - pkg: sigs.k8s.io/cluster-api/exp/addons/api/v1alpha4
        alias: addonsv1alpha4
      - pkg: sigs.k8s.io/cluster-api/exp/addons/api/v1beta1
        alias: addonsv1
      # CAPI exp runtime
      - pkg: sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1
        alias: runtimev1
      - pkg: sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1
        alias: runtimehooksv1
      - pkg: sigs.k8s.io/cluster-api/exp/runtime/controllers
        alias: runtimecontrollers
      - pkg: sigs.k8s.io/cluster-api/exp/runtime/catalog
        alias: runtimecatalog
      - pkg: sigs.k8s.io/cluster-api/internal/runtime/client
        alias: runtimeclient
      - pkg: sigs.k8s.io/cluster-api/internal/runtime/registry
        alias: runtimeregistry
      - pkg: sigs.k8s.io/cluster-api/internal/webhooks/runtime
        alias: runtimewebhooks
      # CAPD
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1alpha3
        alias: infrav1alpha3
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1alpha4
        alias: infrav1alpha4
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1beta1
        alias: infrav1
      # CAPD exp
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/exp/api/v1alpha3
        alias: infraexpv1alpha3
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/exp/api/v1alpha4
        alias: infraexpv1alpha4
      - pkg: sigs.k8s.io/cluster-api/test/infrastructure/docker/exp/api/v1beta1
        alias: infraexpv1
  nolintlint:
    allow-unused: false
    allow-leading-space: false
    require-specific: true
  revive:
    rules:
      # The following rules are recommended https://github.com/mgechev/revive#recommended-configuration
      - name: blank-imports
      - name: context-as-argument
      - name: context-keys-type
      - name: dot-imports
      - name: error-return
      - name: error-strings
      - name: error-naming
      - name: exported
      - name: if-return
      - name: increment-decrement
      - name: var-naming
      - name: var-declaration
      - name: package-comments
      - name: range
      - name: receiver-naming
      - name: time-naming
      - name: unexported-return
      - name: indent-error-flow
      - name: errorf
      - name: empty-block
      - name: superfluous-else
      - name: unused-parameter
      - name: unreachable-code
      - name: redefines-builtin-id
      #
      # Rules in addition to the recommended configuration above.
      #
      - name: bool-literal-in-expr
      - name: constant-logical-expr
  gosec:
    excludes:
    - G307 # Deferring unsafe method "Close" on type "\*os.File"
    - G108 # Profiling endpoint is automatically exposed on /debug/pprof
  gocritic:
    enabled-tags:
      - diagnostic
      - experimental
      - performance
    disabled-checks:
    - appendAssign
    - dupImport # https://github.com/go-critic/go-critic/issues/845
    - evalOrder
    - ifElseChain
    - octalLiteral
    - regexpSimplify
    - sloppyReassign
    - truncateCmp
    - typeDefFirst
    - unnamedResult
    - unnecessaryDefer
    - whyNoLint
    - wrapperFunc
    - rangeValCopy
    - hugeParam

issues:
  max-same-issues: 0
  max-issues-per-linter: 0
  # We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant
  # changes in PRs and avoid nitpicking.
  exclude-use-default: false
  exclude-rules:
  # Specific exclude rules for deprecated fields that are still part of the codebase. These should be removed as the referenced deprecated item is removed from the project.
  - linters:
      - staticcheck
    text: "SA1019: (bootstrapv1.ClusterStatus|scope.Config.Spec.UseExperimentalRetryJoin|DockerMachine.Spec.Bootstrapped|machineStatus.Bootstrapped) is deprecated"
  - linters:
    - revive
    text: "exported: exported method .*\\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported"
  - linters:
    - errcheck
    text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
  # Exclude some packages or code to require comments, for example test code, or fake clients.
  - linters:
    - revive
    text: exported (method|function|type|const) (.+) should have comment or be unexported
    source: (func|type).*Fake.*
  - linters:
    - revive
    text: exported (method|function|type|const) (.+) should have comment or be unexported
    path: fake_\.go
  - linters:
    - revive
    text: exported (method|function|type|const) (.+) should have comment or be unexported
    path: cmd/clusterctl/internal/test/providers.*.go
  - linters:
    - revive
    text: exported (method|function|type|const) (.+) should have comment or be unexported
    path: "(framework|e2e)/.*.go"
  # Disable unparam "always receives" which might not be really
  # useful when building libraries.
  - linters:
    - unparam
    text: always receives
  # Dot imports for gomega or ginkgo are allowed
  # within test files.
  - path: _test\.go
    text: should not use dot imports
  - path: (framework|e2e)/.*.go
    text: should not use dot imports
  - path: _test\.go
    text: cyclomatic complexity
  # Append should be able to assign to a different var/slice.
  - linters:
    - gocritic
    text: "appendAssign: append result not assigned to the same slice"
 # Disable linters for conversion
  - linters:
    - staticcheck
    text: "SA1019: in.(.+) is deprecated"
    path: .*(api|types)\/.*\/conversion.*\.go$
  - linters:
      - revive
    # Checking if an error is nil to just after return the error or nil is redundant
    text: "if-return: redundant if ...; err != nil check, just return error instead"
    # Ignoring stylistic checks for generated code
    path: .*(api|types|test)\/.*\/conversion.*\.go$
  - linters:
    - revive
    # Exported function and methods should have comments. This warns on undocumented exported functions and methods.
    text: exported (method|function|type|const) (.+) should have comment or be unexported
    # Ignoring stylistic checks for generated code
    path: .*(api|types|test)\/.*\/conversion.*\.go$
  - linters:
    - revive
    # This rule warns when initialism, variable or package naming conventions are not followed.
    text: "var-naming: don't use underscores in Go names;"
    # Ignoring stylistic checks for generated code
    path: .*(api|types|test)\/.*\/conversion.*\.go$
  - linters:
    - revive
    # By convention, receiver names in a method should reflect their identity.
    text: "receiver-naming: receiver name"
    # Ignoring stylistic checks for generated code
    path: .*(api|types)\/.*\/conversion.*\.go$
  - linters:
    - stylecheck
    text: "ST1003: should not use underscores in Go names;"
    path: .*(api|types|test)\/.*\/conversion.*\.go$
  - linters:
    - stylecheck
    text: "ST1016: methods on the same type should have the same receiver name"
    path: .*(api|types)\/.*\/conversion.*\.go$
  # hack/tools
  - linters:
    - typecheck
    text: import (".+") is a program, not an importable package
    path: ^tools\.go$
  # We don't care about defer in for loops in test files.
  - linters:
    - gocritic
    text: "deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop"
    path: _test\.go

source: https://github.com/kubernetes-sigs/cluster-api/blob/main/.golangci.yml

Go environment

$ go version && go env
go version go1.19.5 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/oscaru/.cache/go-build"
GOENV="/home/oscaru/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/oscaru/.asdf/installs/golang/1.19.5/packages/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/oscaru/.asdf/installs/golang/1.19.5/packages"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/oscaru/.asdf/installs/golang/1.19.5/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/oscaru/.asdf/installs/golang/1.19.5/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/oscaru/go/src/k8s.io/kubernetes-sig/cluster-api/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build889999732=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
/home/oscaru/go/src/k8s.io/kubernetes-sig/cluster-api/hack/tools/bin/golangci-lint run -v 
INFO [config_reader] Config search paths: [./ /home/oscaru/go/src/k8s.io/kubernetes-sig/cluster-api /home/oscaru/go/src/k8s.io/kubernetes-sig /home/oscaru/go/src/k8s.io /home/oscaru/go/src /home/oscaru/go /home/oscaru /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 44 linters: [asasalint asciicheck bidichk bodyclose containedctx depguard dogsled dupword durationcheck errcheck errchkjson exportloopref gci ginkgolinter goconst gocritic godot gofmt goimports goprintffuncname gosec gosimple govet importas ineffassign misspell nakedret nilerr noctx nolintlint nosprintfhostport prealloc predeclared revive rowserrcheck staticcheck stylecheck thelper typecheck unconvert unparam unused usestdlibvars whitespace] 
INFO [loader] Using build tags: [tools e2e]       
INFO [loader] Go packages loading at mode 575 (files|name|types_sizes|compiled_files|deps|exports_file|imports) took 23.896040448s 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 55.212405ms 
INFO [linters_context/goanalysis] analyzers took 10m36.628002635s with top 10 stages: gocritic: 4m19.701638511s, buildir: 2m7.664665563s, goimports: 38.255442469s, the_only_name: 31.341261074s, buildssa: 22.80547123s, gci: 7.619224328s, nilness: 7.081789628s, unconvert: 6.523007002s, unparam: 4.500342253s, inspect: 4.395145917s 
ERRO [runner] Panic: bodyclose: package "v1beta1" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA: goroutine 63675 [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:102 +0x155
panic({0xfa16e0, 0xc00d5f68d0})
	runtime/panic.go:884 +0x213
github.com/timakin/bodyclose/passes/bodyclose.runner.run({0xc0839a5b30, {0x0, 0x0}, 0x0, {0x0, 0x0}, 0x0, 0x0}, 0xc0839a5b30)
	github.com/timakin/bodyclose@v0.0.0-20221125081123-e39cf3fc478e/passes/bodyclose/bodyclose.go:45 +0x665
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc00615faa0)
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:188 +0x9df
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:106 +0x1d
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc001f0d360, {0x10e8385, 0x9}, 0xc006cbdf48)
	github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x4a
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc03be0cf58?)
	github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:105 +0x85
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc00615faa0)
	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: bodyclose: package "v1beta1" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA 
WARN [linters_context] rowserrcheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649. 
INFO [runner] processing took 2.432µs with stages: skip_dirs: 486ns, nolint: 402ns, max_same_issues: 291ns, exclude-rules: 140ns, path_prettifier: 128ns, identifier_marker: 122ns, cgo: 117ns, filename_unadjuster: 113ns, skip_files: 107ns, autogenerated_exclude: 103ns, source_code: 102ns, max_from_linter: 58ns, path_shortener: 36ns, sort_results: 36ns, exclude: 33ns, severity-rules: 32ns, diff: 32ns, path_prefixer: 32ns, uniq_by_line: 31ns, max_per_file_from_linter: 31ns 
INFO [runner] linters took 31.447096381s with stages: goanalysis_metalinter: 31.447064985s, rowserrcheck: 4.307µs 
ERRO Running error: 1 error occurred:
	* can't run linter goanalysis_metalinter: goanalysis_metalinter: bodyclose: package "v1beta1" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is nil, not *buildssa.SSA
 
INFO Memory: 545 samples, avg is 2129.6MB, max is 6093.9MB 
INFO Execution took 55.441817627s                 
make: *** [Makefile:547: lint] Error 3

Code example or link to a public repository

https://github.com/kubernetes-sigs/cluster-api

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[ldez]

It seems to be an interaction between ginkgolinter and the SSA-based linters, and something in the code of cluster-api.

SSA-based linters:

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

I think that ginkgolinter is modifying the AST, what should never be done.

ping @nunnatsa

[ldez]

I confirm the root cause is ginkgolinter, so for now you have to disable it.

[oscr]

Thank you for confirming ldez. It is disabled for now.

[ldez]

Ldez not Idez 😉

[oscr]

My apologies! I didn't want to mention you and trigger a notification. 🙏

[ldez]

No problem 👍

@nunnatsa can you confirm my assumption: ginkgolinter is modifying the AST

[nunnatsa]

Unless there is a bug, ginkgolinter only sets the suggested fix field, but not directly changes the ast.

[ldez]

FYI, only ginkgolinter is producing the current bug.
@nunnatsa Do you intend to investigate in the following days?

[nunnatsa]

Of course @ldez