From 64492b5e599dc735ac0718a84572a20a43bbbadc Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Mon, 19 Feb 2024 14:58:58 +0100 Subject: [PATCH] feat: disable copyloopvar and intrange on Go < 1.22 (#4397) --- pkg/commands/executor.go | 13 +++++- pkg/config/config.go | 8 ++-- pkg/config/config_test.go | 86 +++++++++++++++++++++++++++++++++++ pkg/golinters/govet.go | 2 +- pkg/golinters/paralleltest.go | 2 +- pkg/lint/linter/config.go | 27 ++++++----- pkg/lint/linter/linter.go | 21 ++++++--- pkg/lint/lintersdb/manager.go | 6 ++- test/linters_test.go | 3 +- test/run_test.go | 8 ++-- test/testdata/copyloopvar.go | 2 + test/testdata/intrange.go | 2 + 12 files changed, 148 insertions(+), 32 deletions(-) create mode 100644 pkg/config/config_test.go diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index d241f5656a15..a6e08a48b00d 100644 --- a/pkg/commands/executor.go +++ b/pkg/commands/executor.go @@ -116,7 +116,18 @@ func NewExecutor(buildInfo BuildInfo) *Executor { e.log.Fatalf("Can't read config: %s", err) } - if (commandLineCfg == nil || commandLineCfg.Run.Go == "") && e.cfg != nil && e.cfg.Run.Go == "" { + if commandLineCfg != nil && commandLineCfg.Run.Go != "" { + // This hack allow to have the right Run information at least for the Go version (because the default value of the "go" flag is empty). + // If you put a log for `m.cfg.Run.Go` inside `GetAllSupportedLinterConfigs`, + // you will observe that at end (without this hack) the value will have the right value but too late, + // the linters are already running with the previous uncompleted configuration. + // TODO(ldez) there is a major problem with the executor: + // the parsing of the configuration and the timing to load the configuration and linters are creating unmanageable situations. + // There is no simple solution because it's spaghetti code. + // I need to completely rewrite the command line system and the executor because it's extremely time consuming to debug, + // so it's unmaintainable. + e.cfg.Run.Go = commandLineCfg.Run.Go + } else if e.cfg.Run.Go == "" { e.cfg.Run.Go = config.DetectGoVersion() } diff --git a/pkg/config/config.go b/pkg/config/config.go index a5483a20e3c6..2eb82938dc69 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -41,18 +41,18 @@ type Version struct { Debug bool `mapstructure:"debug"` } -func IsGreaterThanOrEqualGo122(v string) bool { - v1, err := hcversion.NewVersion(strings.TrimPrefix(v, "go")) +func IsGoGreaterThanOrEqual(current, limit string) bool { + v1, err := hcversion.NewVersion(strings.TrimPrefix(current, "go")) if err != nil { return false } - limit, err := hcversion.NewVersion("1.22") + l, err := hcversion.NewVersion(limit) if err != nil { return false } - return v1.GreaterThanOrEqual(limit) + return v1.GreaterThanOrEqual(l) } func DetectGoVersion() string { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go new file mode 100644 index 000000000000..ac101b95fb53 --- /dev/null +++ b/pkg/config/config_test.go @@ -0,0 +1,86 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsGoGreaterThanOrEqual(t *testing.T) { + testCases := []struct { + desc string + current string + limit string + assert assert.BoolAssertionFunc + }{ + { + desc: "current (with minor.major) lower than limit", + current: "go1.21", + limit: "1.22", + assert: assert.False, + }, + { + desc: "current (with 0 patch) lower than limit", + current: "go1.21.0", + limit: "1.22", + assert: assert.False, + }, + { + desc: "current (current with multiple patches) lower than limit", + current: "go1.21.6", + limit: "1.22", + assert: assert.False, + }, + { + desc: "current lower than limit (with minor.major)", + current: "go1.22", + limit: "1.22", + assert: assert.True, + }, + { + desc: "current lower than limit (with 0 patch)", + current: "go1.22.0", + limit: "1.22", + assert: assert.True, + }, + { + desc: "current lower than limit (current with multiple patches)", + current: "go1.22.6", + limit: "1.22", + assert: assert.True, + }, + { + desc: "current greater than limit", + current: "go1.23.0", + limit: "1.22", + assert: assert.True, + }, + { + desc: "current with no prefix", + current: "1.22", + limit: "1.22", + assert: assert.True, + }, + { + desc: "invalid current value", + current: "go", + limit: "1.22", + assert: assert.False, + }, + { + desc: "invalid limit value", + current: "go1.22", + limit: "go", + assert: assert.False, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + test.assert(t, IsGoGreaterThanOrEqual(test.current, test.limit)) + }) + } +} diff --git a/pkg/golinters/govet.go b/pkg/golinters/govet.go index e2c7d2df50d9..066f7e682a3d 100644 --- a/pkg/golinters/govet.go +++ b/pkg/golinters/govet.go @@ -173,7 +173,7 @@ func analyzersFromConfig(settings *config.GovetSettings) []*analysis.Analyzer { func isAnalyzerEnabled(name string, cfg *config.GovetSettings, defaultAnalyzers []*analysis.Analyzer) bool { // TODO(ldez) remove loopclosure when go1.23 - if name == loopclosure.Analyzer.Name && config.IsGreaterThanOrEqualGo122(cfg.Go) { + if name == loopclosure.Analyzer.Name && config.IsGoGreaterThanOrEqual(cfg.Go, "1.22") { return false } diff --git a/pkg/golinters/paralleltest.go b/pkg/golinters/paralleltest.go index 5a99830b5636..8215619c730c 100644 --- a/pkg/golinters/paralleltest.go +++ b/pkg/golinters/paralleltest.go @@ -18,7 +18,7 @@ func NewParallelTest(settings *config.ParallelTestSettings) *goanalysis.Linter { "ignoremissingsubtests": settings.IgnoreMissingSubtests, } - if config.IsGreaterThanOrEqualGo122(settings.Go) { + if config.IsGoGreaterThanOrEqual(settings.Go, "1.22") { d["ignoreloopVar"] = true } diff --git a/pkg/lint/linter/config.go b/pkg/lint/linter/config.go index ed5e5508ceab..8e57d6bdf6d2 100644 --- a/pkg/lint/linter/config.go +++ b/pkg/lint/linter/config.go @@ -1,7 +1,8 @@ package linter import ( - "golang.org/x/tools/go/analysis" + "fmt" + "golang.org/x/tools/go/packages" "github.com/golangci/golangci-lint/pkg/config" @@ -133,23 +134,27 @@ func (lc *Config) Name() string { return lc.Linter.Name() } -func (lc *Config) WithNoopFallback(cfg *config.Config) *Config { - if cfg != nil && config.IsGreaterThanOrEqualGo122(cfg.Run.Go) { - lc.Linter = &Noop{ - name: lc.Linter.Name(), - desc: lc.Linter.Desc(), - run: func(_ *analysis.Pass) (any, error) { - return nil, nil - }, - } - +func (lc *Config) WithNoopFallback(cfg *config.Config, cond func(cfg *config.Config) error) *Config { + if err := cond(cfg); err != nil { + lc.Linter = NewNoop(lc.Linter, err.Error()) lc.LoadMode = 0 + return lc.WithLoadFiles() } return lc } +func IsGoLowerThanGo122() func(cfg *config.Config) error { + return func(cfg *config.Config) error { + if cfg == nil || config.IsGoGreaterThanOrEqual(cfg.Run.Go, "1.22") { + return nil + } + + return fmt.Errorf("this linter is disabled because the Go version (%s) of your project is lower than Go 1.22", cfg.Run.Go) + } +} + func NewConfig(linter Linter) *Config { lc := &Config{ Linter: linter, diff --git a/pkg/lint/linter/linter.go b/pkg/lint/linter/linter.go index a65d6b927852..e086bbe5fcc7 100644 --- a/pkg/lint/linter/linter.go +++ b/pkg/lint/linter/linter.go @@ -3,8 +3,6 @@ package linter import ( "context" - "golang.org/x/tools/go/analysis" - "github.com/golangci/golangci-lint/pkg/result" ) @@ -15,14 +13,23 @@ type Linter interface { } type Noop struct { - name string - desc string - run func(pass *analysis.Pass) (any, error) + name string + desc string + reason string +} + +func NewNoop(l Linter, reason string) Noop { + return Noop{ + name: l.Name(), + desc: l.Desc(), + reason: reason, + } } func (n Noop) Run(_ context.Context, lintCtx *Context) ([]result.Issue, error) { - lintCtx.Log.Warnf("%s 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.", n.name) + if n.reason != "" { + lintCtx.Log.Warnf("%s: %s", n.name, n.reason) + } return nil, nil } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 977ef0d1c61d..e6a171f1ff5d 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -303,7 +303,8 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { linter.NewConfig(golinters.NewCopyLoopVar()). WithSince("v1.57.0"). WithPresets(linter.PresetStyle). - WithURL("https://github.com/karamaru-alpha/copyloopvar"), + WithURL("https://github.com/karamaru-alpha/copyloopvar"). + WithNoopFallback(m.cfg, linter.IsGoLowerThanGo122()), linter.NewConfig(golinters.NewCyclop(cyclopCfg)). WithSince("v1.37.0"). @@ -617,7 +618,8 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { linter.NewConfig(golinters.NewIntrange()). WithSince("v1.57.0"). - WithURL("https://github.com/ckaznocha/intrange"), + WithURL("https://github.com/ckaznocha/intrange"). + WithNoopFallback(m.cfg, linter.IsGoLowerThanGo122()), linter.NewConfig(golinters.NewIreturn(ireturnCfg)). WithSince("v1.43.0"). diff --git a/test/linters_test.go b/test/linters_test.go index 1019a1f09c42..ada3dbeb9e45 100644 --- a/test/linters_test.go +++ b/test/linters_test.go @@ -84,7 +84,7 @@ func testOneSource(t *testing.T, log *logutils.StderrLog, binPath, sourcePath st } args := []string{ - "--allow-parallel-runners", + "--go=1.22", // TODO(ldez) remove this line when we will run go1.23 on the CI. (related to intrange, copyloopvar) "--disable-all", "--out-format=json", "--max-same-issues=100", @@ -99,7 +99,6 @@ func testOneSource(t *testing.T, log *logutils.StderrLog, binPath, sourcePath st cmd := testshared.NewRunnerBuilder(t). WithBinPath(binPath). - WithNoParallelRunners(). WithArgs(caseArgs...). WithRunContext(rc). WithTargetPath(sourcePath). diff --git a/test/run_test.go b/test/run_test.go index 2ec4b64280a0..9fad30e13dc2 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -133,12 +133,12 @@ func TestTestsAreLintedByDefault(t *testing.T) { func TestCgoOk(t *testing.T) { testshared.NewRunnerBuilder(t). WithNoConfig(). - WithArgs( - "--timeout=3m", + WithArgs("--timeout=3m", "--enable-all", "-D", - "nosnakecase,gci", + "nosnakecase", ). + WithArgs("--go=1.22"). // TODO(ldez) remove this line when we will run go1.23 on the CI. (related to intrange, copyloopvar) WithTargetPath(testdataDir, "cgo"). Runner(). Install(). @@ -356,6 +356,7 @@ func TestUnsafeOk(t *testing.T) { testshared.NewRunnerBuilder(t). WithNoConfig(). WithArgs("--enable-all"). + WithArgs("--go=1.22"). // TODO(ldez) remove this line when we will run go1.23 on the CI. (related to intrange, copyloopvar) WithTargetPath(testdataDir, "unsafe"). Runner(). Install(). @@ -514,6 +515,7 @@ func TestEnableAllFastAndEnableCanCoexist(t *testing.T) { testshared.NewRunnerBuilder(t). WithNoConfig(). WithArgs(test.args...). + WithArgs("--go=1.22"). // TODO(ldez) remove this line when we will run go1.23 on the CI. (related to intrange, copyloopvar) WithTargetPath(testdataDir, minimalPkg). Runner(). Run(). diff --git a/test/testdata/copyloopvar.go b/test/testdata/copyloopvar.go index a1fb44164d3f..7fdd2b04b579 100644 --- a/test/testdata/copyloopvar.go +++ b/test/testdata/copyloopvar.go @@ -1,3 +1,5 @@ +//go:build go1.22 + //golangcitest:args -Ecopyloopvar package testdata diff --git a/test/testdata/intrange.go b/test/testdata/intrange.go index 3d9b711d38bc..2220d9618fdf 100644 --- a/test/testdata/intrange.go +++ b/test/testdata/intrange.go @@ -1,3 +1,5 @@ +//go:build go1.22 + //golangcitest:args -Eintrange package testdata