search

nunnatsa / ginkgolinter

golang linter for ginkgo and gomega
MIT License
24 stars 6 forks source link

[BUG] automatic fixing not working #63

Closed pohly closed 1 year ago

pohly commented 1 year ago

Describe the bug

I tried ginkgolinter through golangci-lint v1.51.2 on the Kubernetes E2E test suite. When running with --fix, it reported an issue, but didn't fix it.

To Reproduce

  1. check out https://github.com/pohly/kubernetes/tree/lint-gomega
  2. patch test/e2e/storage/testsuites/subpath.go:
patch -p1 <<EOF
diff --git a/test/e2e/storage/testsuites/subpath.go b/test/e2e/storage/testsuites/subpath.go
index c8c2b787d81..e56948ce7d7 100644
--- a/test/e2e/storage/testsuites/subpath.go
+++ b/test/e2e/storage/testsuites/subpath.go
@@ -1007,7 +1007,7 @@ func testSubpathReconstruction(ctx context.Context, f *framework.Framework, host
                mountPointsAfter := storageutils.FindVolumeGlobalMountPoints(ctx, hostExec, podNode)
                s1 := mountPointsAfter.Difference(mountPoints)
                s2 := mountPoints.Difference(mountPointsAfter)
-               gomega.Expect(s1).To(gomega.BeEmpty(), "global mount points leaked: %v", s1)
+               gomega.Expect(len(s1)).To(gomega.Equal(0), "global mount points leaked: %v", s1)
                gomega.Expect(s2).To(gomega.BeEmpty(), "global mount points not found: %v", s2)
        }
 }

EOF
  1. hack/verify-golangci-lint.sh ./test/e2e/storage/testsuites

Once hack/verify-golangci-lint.sh has been used once, it is also possible to invoke golangci-lint manually:

$ _output/local/bin/golangci-lint run -v --fix ./test/e2e/storage/testsuites/
INFO [config_reader] Config search paths: [./ /nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites /nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage /nvme/gopath/src/k8s.io/kubernetes/test/e2e /nvme/gopath/src/k8s.io/kubernetes/test /nvme/gopath/src/k8s.io/kubernetes /nvme/gopath/src/k8s.io /nvme/gopath/src /nvme/gopath /nvme / /home/pohly] 
INFO [config_reader] Used config file .golangci.yaml 
INFO [lintersdb] Active 6 linters: [ginkgolinter gocritic ineffassign staticcheck stylecheck unused] 
INFO [loader] Go packages loading at mode 575 (compiled_files|deps|types_sizes|exports_file|files|imports|name) took 745.600412ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 10.598521ms 
INFO [linters_context/goanalysis] analyzers took 1m48.224534715s with top 10 stages: buildir: 1m42.856018911s, nilness: 3.116718852s, fact_purity: 828.873639ms, typedness: 527.014267ms, SA5012: 401.171985ms, ginkgolinter: 37.625298ms, directives: 31.602195ms, unused: 24.088056ms, SA4030: 21.908012ms, SA1012: 20.821358ms 
INFO [runner] Processors filtering stat (out/in): skip_files: 1/1, identifier_marker: 1/1, source_code: 1/1, path_prefixer: 1/1, sort_results: 1/1, filename_unadjuster: 1/1, autogenerated_exclude: 1/1, nolint: 1/1, uniq_by_line: 1/1, max_same_issues: 1/1, severity-rules: 1/1, cgo: 1/1, path_prettifier: 1/1, exclude: 1/1, skip_dirs: 1/1, exclude-rules: 1/1, diff: 1/1, max_per_file_from_linter: 1/1, max_from_linter: 1/1, path_shortener: 1/1 
INFO [runner] processing took 2.025624ms with stages: nolint: 1.712211ms, exclude-rules: 94.331µs, source_code: 76.895µs, identifier_marker: 49.305µs, autogenerated_exclude: 42.919µs, path_prettifier: 28.075µs, skip_files: 8.449µs, skip_dirs: 5.781µs, uniq_by_line: 1.739µs, max_from_linter: 1.209µs, cgo: 1.026µs, path_shortener: 945ns, filename_unadjuster: 856ns, max_same_issues: 634ns, exclude: 258ns, max_per_file_from_linter: 241ns, severity-rules: 212ns, path_prefixer: 198ns, diff: 187ns, sort_results: 153ns 
INFO [runner] linters took 13.03452066s with stages: goanalysis_metalinter: 13.032428247s 
INFO fixer took 0s with no stages                 
test/e2e/storage/testsuites/subpath.go:1010:3: ginkgo-linter: wrong length assertion; consider using `gomega.Expect(s1).To(gomega.BeEmpty(), "global mount points leaked: %v", s1)` instead (ginkgolinter)
        gomega.Expect(len(s1)).To(gomega.Equal(0), "global mount points leaked: %v", s1)
        ^
INFO File cache stats: 1 entries of total size 37.9KiB 
INFO Memory: 137 samples, avg is 2056.4MB, max is 3978.6MB 
INFO Execution took 13.798730346s

Note that the issue gets reported, which implies that https://github.com/golangci/golangci-lint/blob/7ac42b0dde93912bd521d0e89ebef8c191b1d10b/pkg/result/processors/fixer.go#L41-L73 didn't fix it - it's indeed still present in the file.

Expected behavior

It should fix the file.

Environment:

Additional context

After removing the logcheck plugin, it is possible to run under dlv:

diff --git a/.golangci.yaml b/.golangci.yaml
index 29de7f79162..2321cd93af5 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -23,18 +23,11 @@ linters:
     - ginkgolinter
     - gocritic
     - ineffassign
-    - logcheck
     - staticcheck
     - stylecheck
     - unused

 linters-settings: # please keep this alphabetized
-  custom:
-    logcheck:
-      # Installed there by hack/verify-golangci-lint.sh.
-      path: _output/local/bin/logcheck.so
-      description: structured logging checker
-      original-url: k8s.io/klog/hack/tools
   gocritic:
     enabled-checks:
       - equalFold
$ (cd hack/tools && dlv debug --wd /nvme/gopath/src/k8s.io/kubernetes github.com/golangci/golangci-lint/cmd/golangci-lint -- run --fix -v ./test/e2e/storage/testsuites )
(dlv) b fixer.go:41
...
(dlv) c
...
> github.com/golangci/golangci-lint/pkg/result/processors.Fixer.Process() /nvme/gopath/pkg/mod/github.com/golangci/golangci-lint@v1.51.2/pkg/result/processors/fixer.go:41 (hits goroutine(1):1 total:1) (PC: 0x1af5e32)
    36: 
    37: func (f Fixer) printStat() {
    38:     f.sw.PrintStages()
    39: }
    40: 
=>  41: func (f Fixer) Process(issues []result.Issue) []result.Issue {
    42:     if !f.cfg.Issues.NeedFix {
    43:         return issues
    44:     }
    45: 
    46:     outIssues := make([]result.Issue, 0, len(issues))
...
> github.com/golangci/golangci-lint/pkg/result/processors.Fixer.Process() /nvme/gopath/pkg/mod/github.com/golangci/golangci-lint@v1.51.2/pkg/result/processors/fixer.go:51 (PC: 0x1af6236)
    46:     outIssues := make([]result.Issue, 0, len(issues))
    47:     issuesToFixPerFile := map[string][]result.Issue{}
    48:     for i := range issues {
    49:         issue := &issues[i]
    50:         if issue.Replacement == nil {
=>  51:             outIssues = append(outIssues, *issue)
    52:             continue
    53:         }
    54: 
    55:         issuesToFixPerFile[issue.FilePath()] = append(issuesToFixPerFile[issue.FilePath()], *issue)
    56:     }
(dlv) p issue
*github.com/golangci/golangci-lint/pkg/result.Issue {
    FromLinter: "ginkgolinter",
    Text: "ginkgo-linter: wrong length assertion; consider using `gomega.Expect(s1).To(gomega.BeEmpty(), \"global mount points leaked: %v\", s1)` instead",
    Severity: "",
    SourceLines: []string len: 1, cap: 1, [
        "\t\tgomega.Expect(len(s1)).To(gomega.Equal(0), \"global mount points leaked: %v\", s1)",
    ],
    Replacement: *github.com/golangci/golangci-lint/pkg/result.Replacement nil,
    Pkg: *golang.org/x/tools/go/packages.Package {
        ID: "k8s.io/kubernetes/test/e2e/storage/testsuites",
        Name: "testsuites",
        PkgPath: "k8s.io/kubernetes/test/e2e/storage/testsuites",
        Errors: []golang.org/x/tools/go/packages.Error len: 0, cap: 0, nil,
        TypeErrors: []go/types.Error len: 0, cap: 0, nil,
        GoFiles: []string len: 19, cap: 32, [
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/capacity.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/disruptive.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/ephemeral.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/fsgroupchangepolicy.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/multivolume.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/provisioning.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/readwriteoncepod.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable_stress.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/topology.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_expand.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_io.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_stress.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumelimits.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumemode.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumeperf.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go",
        ],
        CompiledGoFiles: []string len: 19, cap: 32, [
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/capacity.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/disruptive.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/ephemeral.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/fsgroupchangepolicy.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/multivolume.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/provisioning.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/readwriteoncepod.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable_stress.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/topology.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_expand.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_io.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_stress.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumelimits.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumemode.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumeperf.go",
            "/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go",
        ],
        OtherFiles: []string len: 0, cap: 0, nil,
        EmbedFiles: []string len: 0, cap: 0, nil,
        EmbedPatterns: []string len: 0, cap: 0, nil,
        IgnoredFiles: []string len: 0, cap: 0, nil,
        ExportFile: "/home/pohly/.cache/go-build/9a/9aad1fdf1f98058a6de1e5d943ed660ef33ecc535d6e68664a073d95f053015b-d",
        Imports: map[string]*golang.org/x/tools/go/packages.Package [...],
        Types: *go/types.Package nil,
        Fset: *(*"go/token.FileSet")(0xc001530280),
        IllTyped: false,
        Syntax: []*go/ast.File len: 0, cap: 0, nil,
        TypesInfo: *go/types.Info nil,
        TypesSizes: go/types.Sizes nil,
        forTest: "",
        depsErrors: []*golang.org/x/tools/internal/packagesinternal.PackageError len: 0, cap: 0, nil,
        Module: *golang.org/x/tools/go/packages.Module nil,},
    LineRange: *github.com/golangci/golangci-lint/pkg/result.Range nil,
    Pos: go/token.Position {
        Filename: "test/e2e/storage/testsuites/subpath.go",
        Offset: 37472,
        Line: 1010,
        Column: 3,},
    HunkPos: 0,
    ExpectNoLint: false,
    ExpectedNoLintLinter: "",}

This shows that ginkgolinter didn't provide a replacement, and therefore fixing doesn't work.

pohly commented 1 year ago

The linter is correctly identifying 26 issues in k8s.io/kubernetes/test/e2e/...

That's a manageable number, so I can fix by hand. But being able to fix automatically would still be nicer :smile:

nunnatsa commented 1 year ago

The auto fix does work with the ginkgolinter executable, using the -fix=true flag. I'll check why it's not working with golangci-lint.

nunnatsa commented 1 year ago

@pohly - I think it's a golangci-lint limitation. See here for example: https://github.com/golangci/golangci-lint/issues/1779

The ginkgolinter does write SuggestedFixes and so it's working from the ginkgolinter executable. For now, golangci-lint dose not apply these suggested fixes, and it seems to be a work in progress.

Adding @ldez for reference.

ldez commented 1 year ago

Currently, we don't support SuggestedFixes but we have an internal way to handle fixes.

ex: https://github.com/golangci/golangci-lint/blob/610a2bd199a376bdbc37431ce2ba52d85f0ee307/pkg/golinters/godot.go#L93-L95

So it's not an issue with your linter.

nunnatsa commented 1 year ago

Thanks @ldez . Closing the issue