Skip to content

Commit

Permalink
feat: disable copyloopvar and intrange on Go < 1.22 (golangci#4397)
Browse files Browse the repository at this point in the history
  • Loading branch information
ldez authored and Antonboom committed Mar 3, 2024
1 parent cce1236 commit ae36c90
Show file tree
Hide file tree
Showing 12 changed files with 148 additions and 32 deletions.
13 changes: 12 additions & 1 deletion pkg/commands/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
86 changes: 86 additions & 0 deletions pkg/config/config_test.go
Original file line number Diff line number Diff line change
@@ -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))
})
}
}
2 changes: 1 addition & 1 deletion pkg/golinters/govet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/golinters/paralleltest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
27 changes: 16 additions & 11 deletions pkg/lint/linter/config.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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,
Expand Down
21 changes: 14 additions & 7 deletions pkg/lint/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package linter
import (
"context"

"golang.org/x/tools/go/analysis"

"github.com/golangci/golangci-lint/pkg/result"
)

Expand All @@ -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
}

Expand Down
6 changes: 4 additions & 2 deletions pkg/lint/lintersdb/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Expand Down Expand Up @@ -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").
Expand Down
3 changes: 1 addition & 2 deletions test/linters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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).
Expand Down
8 changes: 5 additions & 3 deletions test/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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().
Expand Down Expand Up @@ -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().
Expand Down Expand Up @@ -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().
Expand Down
2 changes: 2 additions & 0 deletions test/testdata/copyloopvar.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//go:build go1.22

//golangcitest:args -Ecopyloopvar
package testdata

Expand Down
2 changes: 2 additions & 0 deletions test/testdata/intrange.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//go:build go1.22

//golangcitest:args -Eintrange
package testdata

Expand Down

0 comments on commit ae36c90

Please sign in to comment.