Skip to content

feat: disable copyloopvar and intrange on Go < 1.22 #4397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 19, 2024
Merged
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offtop: let's make an issue?
or how do you track to-do backlog?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently working on the topic.

// 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