diff --git a/.golangci.next.reference.yml b/.golangci.next.reference.yml index 90a34668e795..8757aadc8fce 100644 --- a/.golangci.next.reference.yml +++ b/.golangci.next.reference.yml @@ -85,6 +85,7 @@ linters: - mirror - misspell - mnd + - modernize - musttag - nakedret - nestif @@ -199,6 +200,7 @@ linters: - mirror - misspell - mnd + - modernize - musttag - nakedret - nestif @@ -2106,6 +2108,45 @@ linters: - '^math\.' - '^http\.StatusText$' + modernize: + disable: + # Replace interface{} with any. + - any + # Replace for-range over b.N with b.Loop. + - bloop + # Replace []byte(fmt.Sprintf) with fmt.Appendf. + - fmtappendf + # Remove redundant re-declaration of loop variables. + - forvar + # Replace explicit loops over maps with calls to maps package. + - mapsloop + # Replace if/else statements with calls to min or max. + - minmax + # Simplify code by using go1.26's new(expr). + - newexpr + # Suggest replacing omitempty with omitzero for struct fields. + - omitzero + # Replace 3-clause for loops with for-range over integers. + - rangeint + # Replace reflect.TypeOf(x) with TypeFor[T](). + - reflecttypefor + # Replace loops with slices.Contains or slices.ContainsFunc. + - slicescontains + # Replace sort.Slice with slices.Sort for basic types. + - slicessort + # Use iterators instead of Len/At-style APIs. + - stditerators + # Replace HasPrefix/TrimPrefix with CutPrefix. + - stringscutprefix + # Replace ranging over Split/Fields with SplitSeq/FieldsSeq. + - stringsseq + # Replace += with strings.Builder. + - stringsbuilder + # Replace context.WithCancel with t.Context in tests. + - testingcontext + # Replace wg.Add(1)/go/wg.Done() with wg.Go. + - waitgroup + musttag: # A set of custom functions to check in addition to the builtin ones. # Default: json, xml, gopkg.in/yaml.v3, BurntSushi/toml, mitchellh/mapstructure, jmoiron/sqlx diff --git a/jsonschema/golangci.next.jsonschema.json b/jsonschema/golangci.next.jsonschema.json index f471fd3dadff..a99b88922808 100644 --- a/jsonschema/golangci.next.jsonschema.json +++ b/jsonschema/golangci.next.jsonschema.json @@ -706,6 +706,28 @@ "header" ] }, + "modernize-analyzers": { + "enum": [ + "any", + "bloop", + "fmtappendf", + "forvar", + "mapsloop", + "minmax", + "newexpr", + "omitzero", + "rangeint", + "reflecttypefor", + "slicescontains", + "slicessort", + "stditerators", + "stringscutprefix", + "stringsseq", + "stringsbuilder", + "testingcontext", + "waitgroup" + ] + }, "wsl-checks": { "enum": [ "assign", @@ -835,6 +857,7 @@ "mirror", "misspell", "mnd", + "modernize", "musttag", "nakedret", "nestif", @@ -2829,6 +2852,19 @@ } } }, + "modernizeSettings": { + "type": "object", + "additionalProperties": false, + "properties": { + "disable": { + "description": "List of analyzers to disable.", + "type": "array", + "items": { + "$ref": "#/definitions/modernize-analyzers" + } + } + } + }, "nolintlintSettings": { "type": "object", "additionalProperties": false, @@ -4747,6 +4783,9 @@ "mnd": { "$ref": "#/definitions/settings/definitions/mndSettings" }, + "modernize": { + "$ref": "#/definitions/settings/definitions/modernizeSettings" + }, "nolintlint":{ "$ref": "#/definitions/settings/definitions/nolintlintSettings" }, diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index a33a70bf3d62..f251cac906dd 100644 --- a/pkg/config/linters_settings.go +++ b/pkg/config/linters_settings.go @@ -269,6 +269,7 @@ type LintersSettings struct { Makezero MakezeroSettings `mapstructure:"makezero"` Misspell MisspellSettings `mapstructure:"misspell"` Mnd MndSettings `mapstructure:"mnd"` + Modernize ModernizeSettings `mapstructure:"modernize"` MustTag MustTagSettings `mapstructure:"musttag"` Nakedret NakedretSettings `mapstructure:"nakedret"` Nestif NestifSettings `mapstructure:"nestif"` @@ -758,6 +759,10 @@ type MndSettings struct { IgnoredFunctions []string `mapstructure:"ignored-functions"` } +type ModernizeSettings struct { + Disable []string `mapstructure:"disable"` +} + type NoLintLintSettings struct { RequireExplanation bool `mapstructure:"require-explanation"` RequireSpecific bool `mapstructure:"require-specific"` diff --git a/pkg/golinters/modernize/modernize.go b/pkg/golinters/modernize/modernize.go new file mode 100644 index 000000000000..97825c07e0d6 --- /dev/null +++ b/pkg/golinters/modernize/modernize.go @@ -0,0 +1,34 @@ +package modernize + +import ( + "slices" + + "github.com/golangci/golangci-lint/v2/pkg/config" + "github.com/golangci/golangci-lint/v2/pkg/goanalysis" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/modernize" +) + +func New(settings *config.ModernizeSettings) *goanalysis.Linter { + var analyzers []*analysis.Analyzer + + if settings == nil { + analyzers = modernize.Suite + } else { + for _, analyzer := range modernize.Suite { + if slices.Contains(settings.Disable, analyzer.Name) { + continue + } + + analyzers = append(analyzers, analyzer) + } + } + + return goanalysis.NewLinter( + "modernize", + "A suite of analyzers that suggest simplifications to Go code, using modern language and library features.", + analyzers, + nil). + WithLoadMode(goanalysis.LoadModeTypesInfo) +} diff --git a/pkg/golinters/modernize/modernize_integration_test.go b/pkg/golinters/modernize/modernize_integration_test.go new file mode 100644 index 000000000000..ac02a5bf3c9b --- /dev/null +++ b/pkg/golinters/modernize/modernize_integration_test.go @@ -0,0 +1,19 @@ +package modernize + +import ( + "testing" + + "github.com/golangci/golangci-lint/v2/test/testshared/integration" +) + +func TestFromTestdata(t *testing.T) { + integration.RunTestdata(t) +} + +func TestFix(t *testing.T) { + integration.RunFix(t) +} + +func TestFixPathPrefix(t *testing.T) { + integration.RunFixPathPrefix(t) +} diff --git a/pkg/golinters/modernize/testdata/fix/in/any.go b/pkg/golinters/modernize/testdata/fix/in/any.go new file mode 100644 index 000000000000..36c7e00697cd --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/any.go @@ -0,0 +1,12 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package any + +func _(x interface{}) {} // want "interface{} can be replaced by any" + +func _() { + var x interface{} // want "interface{} can be replaced by any" + const any = 1 + var y interface{} // nope: any is shadowed here + _, _ = x, y +} diff --git a/pkg/golinters/modernize/testdata/fix/in/bloop_test.go b/pkg/golinters/modernize/testdata/fix/in/bloop_test.go new file mode 100644 index 000000000000..8a5c8678f541 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/bloop_test.go @@ -0,0 +1,117 @@ +//go:build go1.25 + +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package bloop + +import ( + "sync" + "testing" +) + +func BenchmarkA(b *testing.B) { + println("slow") + b.ResetTimer() + + for range b.N { // want "b.N can be modernized using b.Loop.." + } +} + +func BenchmarkB(b *testing.B) { + // setup + { + b.StopTimer() + println("slow") + b.StartTimer() + } + + for i := range b.N { // Nope. Should we change this to "for i := 0; b.Loop(); i++"? + print(i) + } + + b.StopTimer() + println("slow") +} + +func BenchmarkC(b *testing.B) { + // setup + { + b.StopTimer() + println("slow") + b.StartTimer() + } + + for i := 0; i < b.N; i++ { // want "b.N can be modernized using b.Loop.." + println("no uses of i") + } + + b.StopTimer() + println("slow") +} + +func BenchmarkD(b *testing.B) { + for i := 0; i < b.N; i++ { // want "b.N can be modernized using b.Loop.." + println(i) + } +} + +func BenchmarkE(b *testing.B) { + b.Run("sub", func(b *testing.B) { + b.StopTimer() // not deleted + println("slow") + b.StartTimer() // not deleted + + // ... + }) + b.ResetTimer() + + for i := 0; i < b.N; i++ { // want "b.N can be modernized using b.Loop.." + println("no uses of i") + } + + b.StopTimer() + println("slow") +} + +func BenchmarkF(b *testing.B) { + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < b.N; i++ { // nope: b.N accessed from a FuncLit + } + }() + wg.Wait() +} + +func BenchmarkG(b *testing.B) { + var wg sync.WaitGroup + poster := func() { + for i := 0; i < b.N; i++ { // nope: b.N accessed from a FuncLit + } + wg.Done() + } + wg.Add(2) + for i := 0; i < 2; i++ { + go poster() + } + wg.Wait() +} + +func BenchmarkH(b *testing.B) { + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for range b.N { // nope: b.N accessed from a FuncLit + } + }() + wg.Wait() +} + +func BenchmarkI(b *testing.B) { + for i := 0; i < b.N; i++ { // nope: b.N accessed more than once in benchmark + } + for i := 0; i < b.N; i++ { // nope: b.N accessed more than once in benchmark + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/fieldsseq.go b/pkg/golinters/modernize/testdata/fix/in/fieldsseq.go new file mode 100644 index 000000000000..a0c34d23b916 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/fieldsseq.go @@ -0,0 +1,42 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package fieldsseq + +import ( + "bytes" + "strings" +) + +func _() { + for _, line := range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient" + println(line) + } + for i, line := range strings.Fields("") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Fields("") { // nope: uses index var + println(i) + } + for i := range strings.Fields("") { // nope: uses index var + println(i) + } + for _ = range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient" + } + for range strings.Fields("") { // want "Ranging over FieldsSeq is more efficient" + } + for range bytes.Fields(nil) { // want "Ranging over FieldsSeq is more efficient" + } + { + lines := strings.Fields("") // want "Ranging over FieldsSeq is more efficient" + for _, line := range lines { + println(line) + } + } + { + lines := strings.Fields("") // nope: lines is used not just by range + for _, line := range lines { + println(line) + } + println(lines) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/fmtappendf.go b/pkg/golinters/modernize/testdata/fix/in/fmtappendf.go new file mode 100644 index 000000000000..482a6a75d28f --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/fmtappendf.go @@ -0,0 +1,46 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package fmtappendf + +import ( + "fmt" +) + +func two() string { + return "two" +} + +func bye() { + _ = []byte(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func funcsandvars() { + one := "one" + _ = []byte(fmt.Sprintf("bye %d %s %s", 1, two(), one)) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func typealias() { + type b = byte + type bt = []byte + _ = []b(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf" + _ = bt(fmt.Sprintf("bye %d", 1)) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func otherprints() { + _ = []byte(fmt.Sprint("bye %d", 1)) // want "Replace .*Sprint.* with fmt.Append" + _ = []byte(fmt.Sprintln("bye %d", 1)) // want "Replace .*Sprintln.* with fmt.Appendln" +} + +func comma() { + type S struct{ Bytes []byte } + var _ = struct{ A S }{ + A: S{ + Bytes: []byte( // want "Replace .*Sprint.* with fmt.Appendf" + fmt.Sprintf("%d", 0), + ), + }, + } + _ = []byte( // want "Replace .*Sprint.* with fmt.Appendf" + fmt.Sprintf("%d", 0), + ) +} diff --git a/pkg/golinters/modernize/testdata/fix/in/forvar.go b/pkg/golinters/modernize/testdata/fix/in/forvar.go new file mode 100644 index 000000000000..5273b3c04431 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/forvar.go @@ -0,0 +1,91 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package forvar + +func _(m map[int]int, s []int) { + // changed + for i := range s { + i := i // want "copying variable is unneeded" + go f(i) + } + for _, v := range s { + v := v // want "copying variable is unneeded" + go f(v) + } + for k, v := range m { + k := k // want "copying variable is unneeded" + v := v // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + v := v // want "copying variable is unneeded" + k := k // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + k, v := k, v // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + v, k := v, k // want "copying variable is unneeded" + go f(k) + go f(v) + } + for i := range s { + /* hi */ i := i // want "copying variable is unneeded" + go f(i) + } + // nope + var i, k, v int + + for i = range s { // nope, scope change + i := i + go f(i) + } + for _, v = range s { // nope, scope change + v := v + go f(v) + } + for k = range m { // nope, scope change + k := k + go f(k) + } + for k, v = range m { // nope, scope change + k := k + v := v + go f(k) + go f(v) + } + for _, v = range m { // nope, scope change + v := v + go f(v) + } + for _, v = range m { // nope, not x := x + v := i + go f(v) + } + for k, v := range m { // nope, LHS and RHS differ + v, k := k, v + go f(k) + go f(v) + } + for k, v := range m { // nope, not a simple redecl + k, v, x := k, v, 1 + go f(k) + go f(v) + go f(x) + } + for i := range s { // nope, not a simple redecl + i := (i) + go f(i) + } + for i := range s { // nope, not a simple redecl + i := i + 1 + go f(i) + } +} + +func f(n int) {} diff --git a/pkg/golinters/modernize/testdata/fix/in/mapsloop.go b/pkg/golinters/modernize/testdata/fix/in/mapsloop.go new file mode 100644 index 000000000000..9fec0b11d601 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/mapsloop.go @@ -0,0 +1,217 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package mapsloop + +import ( + "iter" + "maps" +) + +var _ = maps.Clone[M] // force "maps" import so that each diagnostic doesn't add one + +type M map[int]string + +// -- src is map -- + +func useCopy(dst, src map[int]string) { + // Replace loop by maps.Copy. + for key, value := range src { + // A + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } +} + +func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) { + // Replace loop by maps.Copy. + for key, value := range src { + // A + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } +} + +func useCopyNotClone(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace make(...) by maps.Copy. + dst := make(map[int]string, len(src)) + // A + for key, value := range src { + // B + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + // C + } + + // A + dst = map[int]string{} + // B + for key, value := range src { + // C + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useCopyParen(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace (make)(...) by maps.Clone. + dst := (make)(map[int]string, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + + dst = (map[int]string{}) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useCopy_typesDiffer(src M) { + // Replace loop but not make(...) as maps.Copy(src) would return wrong type M. + dst := make(map[int]string, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useCopy_typesDiffer2(src map[int]string) { + // Replace loop but not make(...) as maps.Copy(src) would return wrong type map[int]string. + dst := make(M, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useClone_typesDiffer3(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) as maps.Clone(src) returns map[int]string + // which is assignable to M. + var dst M + dst = make(M, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useClone_typesDiffer4(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) as maps.Clone(src) returns map[int]string + // which is assignable to M. + var dst M + dst = make(M, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) by maps.Clone + dst := make(Map, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} + +// -- src is iter.Seq2 -- + +func useInsert_assignableToSeq2(dst map[int]string, src func(yield func(int, string) bool)) { + // Replace loop by maps.Insert because src is assignable to iter.Seq2. + for k, v := range src { + dst[k] = v // want "Replace m\\[k\\]=v loop with maps.Insert" + } +} + +func useCollect(src iter.Seq2[int, string]) { + // Replace loop and make(...) by maps.Collect. + var dst map[int]string + dst = make(map[int]string) // A + // B + for key, value := range src { + // C + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Collect" + } +} + +func useInsert_typesDifferAssign(src iter.Seq2[int, string]) { + // Replace loop and make(...): maps.Collect returns an unnamed map type + // that is assignable to M. + var dst M + dst = make(M) + // A + for key, value := range src { + // B + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Collect" + } +} + +func useInsert_typesDifferDeclare(src iter.Seq2[int, string]) { + // Replace loop but not make(...) as maps.Collect would return an + // unnamed map type that would change the type of dst. + dst := make(M) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Insert" + } +} + +// -- non-matches -- + +type isomerOfSeq2 func(yield func(int, string) bool) + +func nopeInsertRequiresAssignableToSeq2(dst map[int]string, src isomerOfSeq2) { + for k, v := range src { // nope: src is not assignable to maps.Insert's iter.Seq2 parameter + dst[k] = v + } +} + +func nopeSingleVarRange(dst map[int]bool, src map[int]string) { + for key := range src { // nope: must be "for k, v" + dst[key] = true + } +} + +func nopeBodyNotASingleton(src map[int]string) { + var dst map[int]string + for key, value := range src { + dst[key] = value + println() // nope: other things in the loop body + } +} + +// Regression test for https://github.com/golang/go/issues/70815#issuecomment-2581999787. +func nopeAssignmentHasIncrementOperator(src map[int]int) { + dst := make(map[int]int) + for k, v := range src { + dst[k] += v + } +} + +func nopeNotAMap(src map[int]string) { + var dst []string + for k, v := range src { + dst[k] = v + } +} + +func nopeNotAMapGeneric[E any, M ~map[int]E, S ~[]E](src M) { + var dst S + for k, v := range src { + dst[k] = v + } +} + +func nopeHasImplicitWidening(src map[string]int) { + dst := make(map[string]any) + for k, v := range src { + dst[k] = v + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/mapsloop_dot.go b/pkg/golinters/modernize/testdata/fix/in/mapsloop_dot.go new file mode 100644 index 000000000000..f2a2dc371f38 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/mapsloop_dot.go @@ -0,0 +1,27 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package mapsloop + +import . "maps" + +var _ = Clone[M] // force "maps" import so that each diagnostic doesn't add one + +type M map[int]string + +func useCopyDot(dst, src map[int]string) { + // Replace loop by maps.Copy. + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } +} + +func useCloneDot(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace make(...) by maps.Copy. + dst := make(map[int]string, len(src)) + for key, value := range src { + dst[key] = value // want "Replace m\\[k\\]=v loop with maps.Copy" + } + println(dst) +} diff --git a/pkg/golinters/modernize/testdata/fix/in/minmax.go b/pkg/golinters/modernize/testdata/fix/in/minmax.go new file mode 100644 index 000000000000..59dc9cbfdbae --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/minmax.go @@ -0,0 +1,173 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package minmax + +func ifmin(a, b int) { + x := a // A + // B + if a < b { // want "if statement can be modernized using max" + // C + x = b // D + // E + } + print(x) +} + +func ifmax(a, b int) { + x := a + if a > b { // want "if statement can be modernized using min" + x = b + } + print(x) +} + +func ifminvariant(a, b int) { + x := a + if x > b { // want "if statement can be modernized using min" + x = b + } + print(x) +} + +func ifmaxvariant(a, b int) { + x := b + if a < x { // want "if statement can be modernized using min" + x = a + } + print(x) +} + +func ifelsemin(a, b int) { + var x int // A + // B + if a <= b { // want "if/else statement can be modernized using min" + // C + x = a // D + // E + } else { + // F + x = b // G + // H + } + print(x) +} + +func ifelsemax(a, b int) { + // A + var x int // B + // C + if a >= b { // want "if/else statement can be modernized using max" + // D + x = a // E + // F + } else { + // G + x = b + } + print(x) +} + +func shadowed() int { + hour, min := 3600, 60 + + var time int + if hour < min { // silent: the built-in min function is shadowed here + time = hour + } else { + time = min + } + return time +} + +func nopeIfStmtHasInitStmt() { + x := 1 + if y := 2; y < x { // silent: IfStmt has an Init stmt + x = y + } + print(x) +} + +// Regression test for a bug: fix was "y := max(x, y)". +func oops() { + x := 1 + y := 2 + if x > y { // want "if statement can be modernized using max" + y = x + } + print(y) +} + +// Regression test for a bug: += is not a simple assignment. +func nopeAssignHasIncrementOperator() { + x := 1 + y := 0 + y += 2 + if x > y { + y = x + } + print(y) +} + +// Regression test for https://github.com/golang/go/issues/71721. +func nopeNotAMinimum(x, y int) int { + // A value of -1 or 0 will use a default value (30). + if x <= 0 { + y = 30 + } else { + y = x + } + return y +} + +// Regression test for https://github.com/golang/go/issues/71847#issuecomment-2673491596 +func nopeHasElseBlock(x int) int { + y := x + // Before, this was erroneously reduced to y = max(x, 0) + if y < 0 { + y = 0 + } else { + y += 2 + } + return y +} + +func fix72727(a, b int) { + o := a - 42 + // some important comment. DO NOT REMOVE. + if o < b { // want "if statement can be modernized using max" + o = b + } +} + +type myfloat float64 + +// The built-in min/max differ in their treatment of NaN, +// so reject floating-point numbers (#72829). +func nopeFloat(a, b myfloat) (res myfloat) { + if a < b { + res = a + } else { + res = b + } + return +} + +// Regression test for golang/go#72928. +func underscoreAssign(a, b int) { + if a > b { + _ = a + } +} + +// Regression test for https://github.com/golang/go/issues/73576. +func nopeIfElseIf(a int) int { + x := 0 + if a < 0 { + x = 0 + } else if a > 100 { + x = 100 + } else { + x = a + } + return x +} diff --git a/pkg/golinters/modernize/testdata/fix/in/rangeint.go b/pkg/golinters/modernize/testdata/fix/in/rangeint.go new file mode 100644 index 000000000000..5f44a8721ed6 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/rangeint.go @@ -0,0 +1,264 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package rangeint + +import ( + "os" + os1 "os" +) + +func _(i int, s struct{ i int }, slice []int) { + for i := 0; i < 10; i++ { // want "for loop can be modernized using range over int" + println(i) + } + for j := int(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int8(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int16(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int32(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int64(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := uint8(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := uint16(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := uint32(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := uint64(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int8(0.); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := int8(.0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + for j := os.FileMode(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } + + { + var i int + for i = 0; i < 10; i++ { // want "for loop can be modernized using range over int" + } + // NB: no uses of i after loop. + } + for i := 0; i < 10; i++ { // want "for loop can be modernized using range over int" + // i unused within loop + } + for i := 0; i < len(slice); i++ { // want "for loop can be modernized using range over int" + println(slice[i]) + } + for i := 0; i < len(""); i++ { // want "for loop can be modernized using range over int" + // NB: not simplified to range "" + } + + // nope + for j := .0; j < 10; j++ { // nope: j is a float type + println(j) + } + for j := float64(0); j < 10; j++ { // nope: j is a float type + println(j) + } + for i := 0; i < 10; { // nope: missing increment + } + for i := 0; i < 10; i-- { // nope: negative increment + } + for i := 0; ; i++ { // nope: missing comparison + } + for i := 0; i <= 10; i++ { // nope: wrong comparison + } + for ; i < 10; i++ { // nope: missing init + } + for s.i = 0; s.i < 10; s.i++ { // nope: not an ident + } + for i := 0; i < 10; i++ { // nope: takes address of i + println(&i) + } + for i := 0; i < 10; i++ { // nope: increments i + i++ + } + for i := 0; i < 10; i++ { // nope: assigns i + i = 8 + } + + // The limit expression must be loop invariant; + // see https://github.com/golang/go/issues/72917 + for i := 0; i < f(); i++ { // nope + } + { + var s struct{ limit int } + for i := 0; i < s.limit; i++ { // nope: limit is not a const or local var + } + } + { + const k = 10 + for i := 0; i < k; i++ { // want "for loop can be modernized using range over int" + } + } + { + var limit = 10 + for i := 0; i < limit; i++ { // want "for loop can be modernized using range over int" + } + } + { + var limit = 10 + for i := 0; i < limit; i++ { // nope: limit is address-taken + } + print(&limit) + } + { + limit := 10 + limit++ + for i := 0; i < limit; i++ { // nope: limit is assigned other than by its declaration + } + } + for i := 0; i < Global; i++ { // nope: limit is an exported global var; may be updated elsewhere + } + for i := 0; i < len(table); i++ { // want "for loop can be modernized using range over int" + } + { + s := []string{} + for i := 0; i < len(s); i++ { // nope: limit is not loop-invariant + s = s[1:] + } + } + for i := 0; i < len(slice); i++ { // nope: i is incremented within loop + i += 1 + } + for Global = 0; Global < 10; Global++ { // nope: loop index is a global variable. + } +} + +var Global int + +var table = []string{"hello", "world"} + +func f() int { return 0 } + +// Repro for part of #71847: ("for range n is invalid if the loop body contains i++"): +func _(s string) { + var i int // (this is necessary) + for i = 0; i < len(s); i++ { // nope: loop body increments i + if true { + i++ // nope + } + } +} + +// Repro for #71952: for and range loops have different final values +// on i (n and n-1, respectively) so we can't offer the fix if i is +// used after the loop. +func nopePostconditionDiffers() { + i := 0 + for i = 0; i < 5; i++ { + println(i) + } + println(i) // must print 5, not 4 +} + +// Non-integer untyped constants need to be explicitly converted to int. +func issue71847d() { + const limit = 1e3 // float + for i := 0; i < limit; i++ { // want "for loop can be modernized using range over int" + } + for i := int(0); i < limit; i++ { // want "for loop can be modernized using range over int" + } + for i := uint(0); i < limit; i++ { // want "for loop can be modernized using range over int" + } + + const limit2 = 1 + 0i // complex + for i := 0; i < limit2; i++ { // want "for loop can be modernized using range over int" + } +} + +func issue72726() { + var n, kd int + for i := 0; i < n; i++ { // want "for loop can be modernized using range over int" + // nope: j will be invisible once it's refactored to 'for j := range min(n-j, kd+1)' + for j := 0; j < min(n-j, kd+1); j++ { // nope + _, _ = i, j + } + } + + for i := 0; i < i; i++ { // nope + } + + var i int + for i = 0; i < i/2; i++ { // nope + } + + var arr []int + for i = 0; i < arr[i]; i++ { // nope + } +} + +func todo() { + for j := os1.FileMode(0); j < 10; j++ { // want "for loop can be modernized using range over int" + println(j) + } +} + +type T uint +type TAlias = uint + +func Fn(a int) T { + return T(a) +} + +func issue73037() { + var q T + for a := T(0); a < q; a++ { // want "for loop can be modernized using range over int" + println(a) + } + for a := Fn(0); a < q; a++ { + println(a) + } + var qa TAlias + for a := TAlias(0); a < qa; a++ { // want "for loop can be modernized using range over int" + println(a) + } + for a := T(0); a < 10; a++ { // want "for loop can be modernized using range over int" + for b := T(0); b < 10; b++ { // want "for loop can be modernized using range over int" + println(a, b) + } + } +} + +func issue75289() { + // A use of i within a defer may be textually before the loop but runs + // after, so it should cause the loop to be rejected as a candidate + // to avoid it observing a different final value of i. + { + var i int + defer func() { println(i) }() + for i = 0; i < 10; i++ { // nope: i is accessed after the loop (via defer) + } + } + + // A use of i within a defer within the loop is also a dealbreaker. + { + var i int + for i = 0; i < 10; i++ { // nope: i is accessed after the loop (via defer) + defer func() { println(i) }() + } + } + + // This (outer) defer is irrelevant. + defer func() { + var i int + for i = 0; i < 10; i++ { // want "for loop can be modernized using range over int" + } + }() +} diff --git a/pkg/golinters/modernize/testdata/fix/in/reflecttypefor.go b/pkg/golinters/modernize/testdata/fix/in/reflecttypefor.go new file mode 100644 index 000000000000..4214ba03f91a --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/reflecttypefor.go @@ -0,0 +1,23 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package reflecttypefor + +import ( + "io" + "reflect" + "time" +) + +var ( + x any + _ = reflect.TypeOf(x) // nope (dynamic) + _ = reflect.TypeOf(0) // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(uint(0)) // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(error(nil)) // nope (likely a mistake) + _ = reflect.TypeOf((*error)(nil)) // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(io.Reader(nil)) // nope (likely a mistake) + _ = reflect.TypeOf((*io.Reader)(nil)) // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(*new(time.Time)) // nope (false negative of noEffects) + _ = reflect.TypeOf(time.Time{}) // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(time.Duration(0)) // want "reflect.TypeOf call can be simplified using TypeFor" +) diff --git a/pkg/golinters/modernize/testdata/fix/in/slicescontains.go b/pkg/golinters/modernize/testdata/fix/in/slicescontains.go new file mode 100644 index 000000000000..79b9f701d313 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/slicescontains.go @@ -0,0 +1,206 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package slicescontains + +import "slices" + +var _ = slices.Contains[[]int] // force import of "slices" to avoid duplicate import edits + +func nopeNoBreak(slice []int, needle int) { + for i := range slice { + if slice[i] == needle { + println("found") + } + } +} + +func rangeIndex(slice []int, needle int) { + for i := range slice { // want "Loop can be simplified using slices.Contains" + if slice[i] == needle { + println("found") + break + } + } +} + +func rangeValue(slice []int, needle int) { + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + println("found") + break + } + } +} + +func returns(slice []int, needle int) { + for i := range slice { // want "Loop can be simplified using slices.Contains" + if slice[i] == needle { + println("found") + return + } + } +} + +func assignTrueBreak(slice []int, needle int) { + found := false + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + found = true + break + } + } + print(found) +} + +func assignFalseBreak(slice []int, needle int) { // TODO: treat this specially like booleanTrue + found := true + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + found = false + break + } + } + print(found) +} + +func assignFalseBreakInSelectSwitch(slice []int, needle int) { + // Exercise RangeStmt in CommClause, CaseClause. + select { + default: + found := false + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + found = true + break + } + } + print(found) + } + switch { + default: + found := false + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + found = true + break + } + } + print(found) + } +} + +func returnTrue(slice []int, needle int) bool { + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + return true + } + } + return false +} + +func returnFalse(slice []int, needle int) bool { + for _, elem := range slice { // want "Loop can be simplified using slices.Contains" + if elem == needle { + return false + } + } + return true +} + +func containsFunc(slice []int, needle int) bool { + for _, elem := range slice { // want "Loop can be simplified using slices.ContainsFunc" + if predicate(elem) { + return true + } + } + return false +} + +func nopeLoopBodyHasFreeContinuation(slice []int, needle int) bool { + for _, elem := range slice { + if predicate(elem) { + if needle == 7 { + continue // this statement defeats loop elimination + } + return true + } + } + return false +} + +func generic[T any](slice []T, f func(T) bool) bool { + for _, elem := range slice { // want "Loop can be simplified using slices.ContainsFunc" + if f(elem) { + return true + } + } + return false +} + +func predicate(int) bool + +// Regression tests for bad fixes when needle +// and haystack have different types (#71313): + +func nopeNeedleHaystackDifferentTypes(x any, args []error) { + for _, arg := range args { + if arg == x { + return + } + } +} + +func nopeNeedleHaystackDifferentTypes2(x error, args []any) { + for _, arg := range args { + if arg == x { + return + } + } +} + +func nopeVariadicNamedContainsFunc(slice []int) bool { + for _, elem := range slice { + if variadicPredicate(elem) { + return true + } + } + return false +} + +func variadicPredicate(int, ...any) bool + +func nopeVariadicContainsFunc(slice []int) bool { + f := func(int, ...any) bool { + return true + } + for _, elem := range slice { + if f(elem) { + return true + } + } + return false +} + +// Negative test case for implicit C->I conversion +type I interface{ F() } +type C int + +func (C) F() {} + +func nopeImplicitConversionContainsFunc(slice []C, f func(I) bool) bool { + for _, elem := range slice { + if f(elem) { // implicit conversion from C to I + return true + } + } + return false +} + +func nopeTypeParamWidening[T any](slice []T, f func(any) bool) bool { + for _, elem := range slice { + if f(elem) { // implicit conversion from T to any + return true + } + } + return false +} diff --git a/pkg/golinters/modernize/testdata/fix/in/slicessort.go b/pkg/golinters/modernize/testdata/fix/in/slicessort.go new file mode 100644 index 000000000000..cf290134c899 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/slicessort.go @@ -0,0 +1,37 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package slicessort + +import "sort" + +type myint int + +func _(s []myint) { + sort.Slice(s, func(i, j int) bool { return s[i] < s[j] }) // want "sort.Slice can be modernized using slices.Sort" +} + +func _(x *struct{ s []int }) { + sort.Slice(x.s, func(first, second int) bool { return x.s[first] < x.s[second] }) // want "sort.Slice can be modernized using slices.Sort" +} + +func _(s []int) { + sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator +} + +func _(s []int) { + sort.Slice(s, func(i, j int) bool { return s[j] < s[i] }) // nope: wrong index var +} + +func _(sense bool, s2 []struct{ x int }) { + sort.Slice(s2, func(i, j int) bool { return s2[i].x < s2[j].x }) // nope: not a simple index operation + + // Regression test for a crash: the sole statement of a + // comparison func body is not necessarily a return! + sort.Slice(s2, func(i, j int) bool { + if sense { + return s2[i].x < s2[j].x + } else { + return s2[i].x > s2[j].x + } + }) +} diff --git a/pkg/golinters/modernize/testdata/fix/in/splitseq.go b/pkg/golinters/modernize/testdata/fix/in/splitseq.go new file mode 100644 index 000000000000..a4c09f34254f --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/splitseq.go @@ -0,0 +1,42 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package splitseq + +import ( + "bytes" + "strings" +) + +func _() { + for _, line := range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + println(line) + } + for i, line := range strings.Split("", "") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Split("", "") { // nope: uses index var + println(i) + } + for i := range strings.Split("", "") { // nope: uses index var + println(i) + } + for _ = range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range strings.Split("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range bytes.Split(nil, nil) { // want "Ranging over SplitSeq is more efficient" + } + { + lines := strings.Split("", "") // want "Ranging over SplitSeq is more efficient" + for _, line := range lines { + println(line) + } + } + { + lines := strings.Split("", "") // nope: lines is used not just by range + for _, line := range lines { + println(line) + } + println(lines) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/stditerators.go b/pkg/golinters/modernize/testdata/fix/in/stditerators.go new file mode 100644 index 000000000000..287c806ceb8f --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/stditerators.go @@ -0,0 +1,60 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stditerators + +import "go/types" + +func _(tuple *types.Tuple) { + for i := 0; i < tuple.Len(); i++ { // want "Len/At loop can simplified using Tuple.Variables iteration" + print(tuple.At(i)) + } +} + +func _(scope *types.Scope) { + for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + print(scope.Child(i)) + } + { + const child = 0 // shadowing of preferred name at def + for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + print(scope.Child(i)) + } + } + { + for i := 0; i < scope.NumChildren(); i++ { + const child = 0 // nope: shadowing of fresh name at use + print(scope.Child(i)) + } + } + { + for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + elem := scope.Child(i) // => preferred name = "elem" + print(elem) + } + } + { + for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + first := scope.Child(0) // the name heuristic should not be fooled by this + print(first, scope.Child(i)) + } + } +} + +func _(union, union2 *types.Union) { + for i := 0; i < union.Len(); i++ { // want "Len/Term loop can simplified using Union.Terms iteration" + print(union.Term(i)) + print(union.Term(i)) + } + for i := union.Len() - 1; i >= 0; i-- { // nope: wrong loop form + print(union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: wrong loop form + print(union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: use of i not in x.At(i) + print(i, union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: x.At and x.Len have different receivers + print(i, union2.Term(i)) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/stringsbuilder.go b/pkg/golinters/modernize/testdata/fix/in/stringsbuilder.go new file mode 100644 index 000000000000..6bb9493585d6 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/stringsbuilder.go @@ -0,0 +1,103 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stringsbuilder + +// basic test +func _() { + var s string + s += "before" + for range 10 { + s += "in" // want "using string \\+= string in a loop is inefficient" + s += "in2" + } + s += "after" + print(s) +} + +// with initializer +func _() { + var s = "a" + for range 10 { + s += "b" // want "using string \\+= string in a loop is inefficient" + } + print(s) +} + +// with empty initializer +func _() { + var s = "" + for range 10 { + s += "b" // want "using string \\+= string in a loop is inefficient" + } + print(s) +} + +// with short decl +func _() { + s := "a" + for range 10 { + s += "b" // want "using string \\+= string in a loop is inefficient" + } + print(s) +} + +// with short decl and empty initializer +func _() { + s := "" + for range 10 { + s += "b" // want "using string \\+= string in a loop is inefficient" + } + print(s) +} + +// nope: += must appear at least once within a loop. +func _() { + var s string + s += "a" + s += "b" + s += "c" + print(s) +} + +// nope: the declaration of s is not in a block. +func _() { + if s := "a"; true { + for range 10 { + s += "x" + } + print(s) + } +} + +// in a switch (special case of "in a block" logic) +func _() { + switch { + default: + s := "a" + for range 10 { + s += "b" // want "using string \\+= string in a loop is inefficient" + } + print(s) + } +} + +// nope: don't handle direct assignments to the string (only +=). +func _(x string) string { + var s string + s = x + for range 3 { + s += "" + x + } + return s +} + +// Regression test for bug in a GenDecl with parens. +func issue75318(slice []string) string { + var ( + msg string + ) + for _, s := range slice { + msg += s // want "using string \\+= string in a loop is inefficient" + } + return msg +} diff --git a/pkg/golinters/modernize/testdata/fix/in/stringscutprefix.go b/pkg/golinters/modernize/testdata/fix/in/stringscutprefix.go new file mode 100644 index 000000000000..7c1ce1c027dc --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/stringscutprefix.go @@ -0,0 +1,166 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stringscutprefix + +import ( + "strings" +) + +var ( + s, pre, suf string +) + +// test supported cases of pattern 1 - CutPrefix +func _() { + if strings.HasPrefix(s, pre) { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a := strings.TrimPrefix(s, pre) + _ = a + } + if strings.HasPrefix("", "") { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a := strings.TrimPrefix("", "") + _ = a + } + if strings.HasPrefix(s, "") { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + println([]byte(strings.TrimPrefix(s, ""))) + } + if strings.HasPrefix(s, "") { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b := "", strings.TrimPrefix(s, "") + _, _ = a, b + } + if strings.HasPrefix(s, "") { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b := strings.TrimPrefix(s, ""), strings.TrimPrefix(s, "") // only replace the first occurrence + s = "123" + b = strings.TrimPrefix(s, "") // only replace the first occurrence + _, _ = a, b + } + + var a, b string + if strings.HasPrefix(s, "") { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b = "", strings.TrimPrefix(s, "") + _, _ = a, b + } +} + +// test basic cases for CutSuffix - only covering the key differences +func _() { + if strings.HasSuffix(s, suf) { // want "HasSuffix \\+ TrimSuffix can be simplified to CutSuffix" + a := strings.TrimSuffix(s, suf) + _ = a + } + if strings.HasSuffix(s, "") { // want "HasSuffix \\+ TrimSuffix can be simplified to CutSuffix" + println([]byte(strings.TrimSuffix(s, ""))) + } +} + +// test cases that are not supported by pattern1 - CutPrefix +func _() { + ok := strings.HasPrefix("", "") + if ok { // noop, currently it doesn't track the result usage of HasPrefix + a := strings.TrimPrefix("", "") + _ = a + } + if strings.HasPrefix(s, pre) { + a := strings.TrimPrefix("", "") // noop, as the argument isn't the same + _ = a + } + if strings.HasPrefix(s, pre) { + var result string + result = strings.TrimPrefix("", "") // noop, as we believe define is more popular. + _ = result + } + if strings.HasPrefix("", "") { + a := strings.TrimPrefix(s, pre) // noop, as the argument isn't the same + _ = a + } + if s1 := s; strings.HasPrefix(s1, pre) { + a := strings.TrimPrefix(s1, pre) // noop, as IfStmt.Init is present + _ = a + } +} + +// test basic unsupported case for CutSuffix +func _() { + if strings.HasSuffix(s, suf) { + a := strings.TrimSuffix("", "") // noop, as the argument isn't the same + _ = a + } +} + +var value0 string + +// test supported cases of pattern2 - CutPrefix +func _() { + if after := strings.TrimPrefix(s, pre); after != s { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + if after := strings.TrimPrefix(s, pre); s != after { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + if after := strings.TrimPrefix(s, pre); s != after { // want "TrimPrefix can be simplified to CutPrefix" + println(strings.TrimPrefix(s, pre)) // noop here + } + if after := strings.TrimPrefix(s, ""); s != after { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + var ok bool // define an ok variable to test the fix won't shadow it for its if stmt body + _ = ok + if after := strings.TrimPrefix(s, pre); after != s { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + var predefined string + if predefined = strings.TrimPrefix(s, pre); s != predefined { // noop + println(predefined) + } + if predefined = strings.TrimPrefix(s, pre); s != predefined { // noop + println(&predefined) + } + var value string + if value = strings.TrimPrefix(s, pre); s != value { // noop + println(value) + } + lhsMap := make(map[string]string) + if lhsMap[""] = strings.TrimPrefix(s, pre); s != lhsMap[""] { // noop + println(lhsMap[""]) + } + arr := make([]string, 0) + if arr[0] = strings.TrimPrefix(s, pre); s != arr[0] { // noop + println(arr[0]) + } + type example struct { + field string + } + var e example + if e.field = strings.TrimPrefix(s, pre); s != e.field { // noop + println(e.field) + } +} + +// test basic cases for pattern2 - CutSuffix +func _() { + if before := strings.TrimSuffix(s, suf); before != s { // want "TrimSuffix can be simplified to CutSuffix" + println(before) + } + if before := strings.TrimSuffix(s, suf); s != before { // want "TrimSuffix can be simplified to CutSuffix" + println(before) + } +} + +// test cases that not supported by pattern2 - CutPrefix +func _() { + if after := strings.TrimPrefix(s, pre); s != pre { // noop + println(after) + } + if after := strings.TrimPrefix(s, pre); after != pre { // noop + println(after) + } + if strings.TrimPrefix(s, pre) != s { + println(strings.TrimPrefix(s, pre)) + } +} + +// test basic unsupported case for pattern2 - CutSuffix +func _() { + if before := strings.TrimSuffix(s, suf); s != suf { // noop + println(before) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/in/testingcontext_test.go b/pkg/golinters/modernize/testdata/fix/in/testingcontext_test.go new file mode 100644 index 000000000000..aca65b612e70 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/testingcontext_test.go @@ -0,0 +1,79 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package testingcontext + +import ( + "context" + "testing" +) + +func Test(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using t.Context" + defer cancel() + _ = ctx + + func() { + ctx, cancel := context.WithCancel(context.Background()) // Nope. scope of defer is not the testing func. + defer cancel() + _ = ctx + }() + + { + ctx, cancel := context.WithCancel(context.TODO()) // want "context.WithCancel can be modernized using t.Context" + defer cancel() + _ = ctx + var t int // not in scope of the call to WithCancel + _ = t + } + + { + ctx := context.Background() + ctx, cancel := context.WithCancel(context.Background()) // Nope. ctx is redeclared. + defer cancel() + _ = ctx + } + + { + var t int + ctx, cancel := context.WithCancel(context.Background()) // Nope. t is shadowed. + defer cancel() + _ = ctx + _ = t + } + + t.Run("subtest", func(t2 *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using t2.Context" + defer cancel() + _ = ctx + }) +} + +func TestAlt(t2 *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using t2.Context" + defer cancel() + _ = ctx +} + +func Testnot(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) // Nope. Not a test func. + defer cancel() + _ = ctx +} + +func Benchmark(b *testing.B) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using b.Context" + defer cancel() + _ = ctx + + b.Run("subtest", func(b2 *testing.B) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using b2.Context" + defer cancel() + _ = ctx + }) +} + +func Fuzz(f *testing.F) { + ctx, cancel := context.WithCancel(context.Background()) // want "context.WithCancel can be modernized using f.Context" + defer cancel() + _ = ctx +} diff --git a/pkg/golinters/modernize/testdata/fix/in/waitgroup.go b/pkg/golinters/modernize/testdata/fix/in/waitgroup.go new file mode 100644 index 000000000000..19b189a9db05 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/in/waitgroup.go @@ -0,0 +1,188 @@ +//go:build go1.25 + +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package waitgroup + +import ( + "fmt" + "sync" +) + +// supported case for pattern 1. +func _() { + var wg sync.WaitGroup + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + defer wg.Done() + fmt.Println() + }() + + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + defer wg.Done() + }() + + for range 10 { + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + defer wg.Done() + fmt.Println() + }() + } +} + +// supported case for pattern 2. +func _() { + var wg sync.WaitGroup + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + fmt.Println() + wg.Done() + }() + + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + wg.Done() + }() + + for range 10 { + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + fmt.Println() + wg.Done() + }() + } +} + +// this function puts some wrong usages but waitgroup modernizer will still offer fixes. +func _() { + var wg sync.WaitGroup + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + defer wg.Done() + defer wg.Done() + fmt.Println() + }() + + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + defer wg.Done() + fmt.Println() + wg.Done() + }() + + wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + fmt.Println() + wg.Done() + wg.Done() + }() +} + +// this function puts the unsupported cases of pattern 1. +func _() { + var wg sync.WaitGroup + wg.Add(1) + go func() {}() + + wg.Add(1) + go func(i int) { + defer wg.Done() + fmt.Println(i) + }(1) + + wg.Add(1) + go func() { + fmt.Println() + defer wg.Done() + }() + + wg.Add(1) + go func() { // noop: no wg.Done call inside function body. + fmt.Println() + }() + + go func() { // noop: no Add call before this go stmt. + defer wg.Done() + fmt.Println() + }() + + wg.Add(2) // noop: only support Add(1). + go func() { + defer wg.Done() + }() + + var wg1 sync.WaitGroup + wg1.Add(1) // noop: Add and Done should be the same object. + go func() { + defer wg.Done() + fmt.Println() + }() + + wg.Add(1) // noop: Add and Done should be the same object. + go func() { + defer wg1.Done() + fmt.Println() + }() +} + +// this function puts the unsupported cases of pattern 2. +func _() { + var wg sync.WaitGroup + wg.Add(1) + go func() { + wg.Done() + fmt.Println() + }() + + go func() { // noop: no Add call before this go stmt. + fmt.Println() + wg.Done() + }() + + var wg1 sync.WaitGroup + wg1.Add(1) // noop: Add and Done should be the same object. + go func() { + fmt.Println() + wg.Done() + }() + + wg.Add(1) // noop: Add and Done should be the same object. + go func() { + fmt.Println() + wg1.Done() + }() +} + +type Server struct { + wg sync.WaitGroup +} + +type ServerContainer struct { + serv Server +} + +func _() { + var s Server + s.wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + print() + s.wg.Done() + }() + + var sc ServerContainer + sc.serv.wg.Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + print() + sc.serv.wg.Done() + }() + + var wg sync.WaitGroup + arr := [1]*sync.WaitGroup{&wg} + arr[0].Add(1) // want "Goroutine creation can be simplified using WaitGroup.Go" + go func() { + print() + arr[0].Done() + }() +} diff --git a/pkg/golinters/modernize/testdata/fix/out/any.go b/pkg/golinters/modernize/testdata/fix/out/any.go new file mode 100644 index 000000000000..f62fcf83945a --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/any.go @@ -0,0 +1,12 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package any + +func _(x any) {} // want "interface{} can be replaced by any" + +func _() { + var x any // want "interface{} can be replaced by any" + const any = 1 + var y interface{} // nope: any is shadowed here + _, _ = x, y +} diff --git a/pkg/golinters/modernize/testdata/fix/out/bloop_test.go b/pkg/golinters/modernize/testdata/fix/out/bloop_test.go new file mode 100644 index 000000000000..66a781bf70ae --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/bloop_test.go @@ -0,0 +1,111 @@ +//go:build go1.25 + +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package bloop + +import ( + "sync" + "testing" +) + +func BenchmarkA(b *testing.B) { + println("slow") + + for b.Loop() { // want "b.N can be modernized using b.Loop.." + } +} + +func BenchmarkB(b *testing.B) { + // setup + { + b.StopTimer() + println("slow") + b.StartTimer() + } + + for i := range b.N { // Nope. Should we change this to "for i := 0; b.Loop(); i++"? + print(i) + } + + b.StopTimer() + println("slow") +} + +func BenchmarkC(b *testing.B) { + // setup + { + + println("slow") + + } + + for b.Loop() { // want "b.N can be modernized using b.Loop.." + println("no uses of i") + } + + b.StopTimer() + println("slow") +} + +func BenchmarkD(b *testing.B) { + for i := 0; b.Loop(); i++ { // want "b.N can be modernized using b.Loop.." + println(i) + } +} + +func BenchmarkE(b *testing.B) { + b.Run("sub", func(b *testing.B) { + b.StopTimer() // not deleted + println("slow") + b.StartTimer() // not deleted + + // ... + }) + + for b.Loop() { // want "b.N can be modernized using b.Loop.." + println("no uses of i") + } + + b.StopTimer() + println("slow") +} + +func BenchmarkF(b *testing.B) { + var wg sync.WaitGroup + wg.Go(func() { + for i := 0; i < b.N; i++ { // nope: b.N accessed from a FuncLit + } + }) + wg.Wait() +} + +func BenchmarkG(b *testing.B) { + var wg sync.WaitGroup + poster := func() { + for i := 0; i < b.N; i++ { // nope: b.N accessed from a FuncLit + } + wg.Done() + } + wg.Add(2) + for range 2 { + go poster() + } + wg.Wait() +} + +func BenchmarkH(b *testing.B) { + var wg sync.WaitGroup + wg.Go(func() { + for range b.N { // nope: b.N accessed from a FuncLit + } + }) + wg.Wait() +} + +func BenchmarkI(b *testing.B) { + for i := 0; i < b.N; i++ { // nope: b.N accessed more than once in benchmark + } + for i := 0; i < b.N; i++ { // nope: b.N accessed more than once in benchmark + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/fieldsseq.go b/pkg/golinters/modernize/testdata/fix/out/fieldsseq.go new file mode 100644 index 000000000000..f91130f58c28 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/fieldsseq.go @@ -0,0 +1,42 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package fieldsseq + +import ( + "bytes" + "strings" +) + +func _() { + for line := range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient" + println(line) + } + for i, line := range strings.Fields("") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Fields("") { // nope: uses index var + println(i) + } + for i := range strings.Fields("") { // nope: uses index var + println(i) + } + for range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient" + } + for range strings.FieldsSeq("") { // want "Ranging over FieldsSeq is more efficient" + } + for range bytes.FieldsSeq(nil) { // want "Ranging over FieldsSeq is more efficient" + } + { + lines := strings.FieldsSeq("") // want "Ranging over FieldsSeq is more efficient" + for line := range lines { + println(line) + } + } + { + lines := strings.Fields("") // nope: lines is used not just by range + for _, line := range lines { + println(line) + } + println(lines) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/fmtappendf.go b/pkg/golinters/modernize/testdata/fix/out/fmtappendf.go new file mode 100644 index 000000000000..b3bc47d858c7 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/fmtappendf.go @@ -0,0 +1,44 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package fmtappendf + +import ( + "fmt" +) + +func two() string { + return "two" +} + +func bye() { + _ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func funcsandvars() { + one := "one" + _ = fmt.Appendf(nil, "bye %d %s %s", 1, two(), one) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func typealias() { + type b = byte + type bt = []byte + _ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf" + _ = fmt.Appendf(nil, "bye %d", 1) // want "Replace .*Sprintf.* with fmt.Appendf" +} + +func otherprints() { + _ = fmt.Append(nil, "bye %d", 1) // want "Replace .*Sprint.* with fmt.Append" + _ = fmt.Appendln(nil, "bye %d", 1) // want "Replace .*Sprintln.* with fmt.Appendln" +} + +func comma() { + type S struct{ Bytes []byte } + var _ = struct{ A S }{ + A: S{ + Bytes: // want "Replace .*Sprint.* with fmt.Appendf" + fmt.Appendf(nil, "%d", 0), + }, + } + _ = // want "Replace .*Sprint.* with fmt.Appendf" + fmt.Appendf(nil, "%d", 0) +} diff --git a/pkg/golinters/modernize/testdata/fix/out/forvar.go b/pkg/golinters/modernize/testdata/fix/out/forvar.go new file mode 100644 index 000000000000..b2837bab10ef --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/forvar.go @@ -0,0 +1,91 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package forvar + +func _(m map[int]int, s []int) { + // changed + for i := range s { + // want "copying variable is unneeded" + go f(i) + } + for _, v := range s { + // want "copying variable is unneeded" + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + go f(k) + go f(v) + } + for i := range s { + /* hi */ // want "copying variable is unneeded" + go f(i) + } + // nope + var i, k, v int + + for i = range s { // nope, scope change + i := i + go f(i) + } + for _, v = range s { // nope, scope change + v := v + go f(v) + } + for k = range m { // nope, scope change + k := k + go f(k) + } + for k, v = range m { // nope, scope change + k := k + v := v + go f(k) + go f(v) + } + for _, v = range m { // nope, scope change + v := v + go f(v) + } + for _, v = range m { // nope, not x := x + v := i + go f(v) + } + for k, v := range m { // nope, LHS and RHS differ + v, k := k, v + go f(k) + go f(v) + } + for k, v := range m { // nope, not a simple redecl + k, v, x := k, v, 1 + go f(k) + go f(v) + go f(x) + } + for i := range s { // nope, not a simple redecl + i := (i) + go f(i) + } + for i := range s { // nope, not a simple redecl + i := i + 1 + go f(i) + } +} + +func f(n int) {} diff --git a/pkg/golinters/modernize/testdata/fix/out/mapsloop.go b/pkg/golinters/modernize/testdata/fix/out/mapsloop.go new file mode 100644 index 000000000000..1351aacc9e98 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/mapsloop.go @@ -0,0 +1,201 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package mapsloop + +import ( + "iter" + "maps" +) + +var _ = maps.Clone[M] // force "maps" import so that each diagnostic doesn't add one + +type M map[int]string + +// -- src is map -- + +func useCopy(dst, src map[int]string) { + // Replace loop by maps.Copy. + // A + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) +} + +func useCopyGeneric[K comparable, V any, M ~map[K]V](dst, src M) { + // Replace loop by maps.Copy. + // A + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) +} + +func useCopyNotClone(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace make(...) by maps.Copy. + dst := make(map[int]string, len(src)) + // A + // B + // want "Replace m\\[k\\]=v loop with maps.Copy" + // C + maps.Copy(dst, src) + + // A + dst = map[int]string{} + // B + // C + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useCopyParen(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace (make)(...) by maps.Clone. + dst := (make)(map[int]string, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + + dst = (map[int]string{}) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useCopy_typesDiffer(src M) { + // Replace loop but not make(...) as maps.Copy(src) would return wrong type M. + dst := make(map[int]string, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useCopy_typesDiffer2(src map[int]string) { + // Replace loop but not make(...) as maps.Copy(src) would return wrong type map[int]string. + dst := make(M, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useClone_typesDiffer3(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) as maps.Clone(src) returns map[int]string + // which is assignable to M. + var dst M + dst = make(M, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useClone_typesDiffer4(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) as maps.Clone(src) returns map[int]string + // which is assignable to M. + var dst M + dst = make(M, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +func useClone_generic[Map ~map[K]V, K comparable, V any](src Map) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace loop and make(...) by maps.Clone + dst := make(Map, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + maps.Copy(dst, src) + println(dst) +} + +// -- src is iter.Seq2 -- + +func useInsert_assignableToSeq2(dst map[int]string, src func(yield func(int, string) bool)) { + // Replace loop by maps.Insert because src is assignable to iter.Seq2. + // want "Replace m\\[k\\]=v loop with maps.Insert" + maps.Insert(dst, src) +} + +func useCollect(src iter.Seq2[int, string]) { + // Replace loop and make(...) by maps.Collect. + var dst map[int]string + // A + // B + // C + // want "Replace m\\[k\\]=v loop with maps.Collect" + dst = maps.Collect(src) +} + +func useInsert_typesDifferAssign(src iter.Seq2[int, string]) { + // Replace loop and make(...): maps.Collect returns an unnamed map type + // that is assignable to M. + var dst M + // A + // B + // want "Replace m\\[k\\]=v loop with maps.Collect" + dst = maps.Collect(src) +} + +func useInsert_typesDifferDeclare(src iter.Seq2[int, string]) { + // Replace loop but not make(...) as maps.Collect would return an + // unnamed map type that would change the type of dst. + dst := make(M) + // want "Replace m\\[k\\]=v loop with maps.Insert" + maps.Insert(dst, src) +} + +// -- non-matches -- + +type isomerOfSeq2 func(yield func(int, string) bool) + +func nopeInsertRequiresAssignableToSeq2(dst map[int]string, src isomerOfSeq2) { + for k, v := range src { // nope: src is not assignable to maps.Insert's iter.Seq2 parameter + dst[k] = v + } +} + +func nopeSingleVarRange(dst map[int]bool, src map[int]string) { + for key := range src { // nope: must be "for k, v" + dst[key] = true + } +} + +func nopeBodyNotASingleton(src map[int]string) { + var dst map[int]string + for key, value := range src { + dst[key] = value + println() // nope: other things in the loop body + } +} + +// Regression test for https://github.com/golang/go/issues/70815#issuecomment-2581999787. +func nopeAssignmentHasIncrementOperator(src map[int]int) { + dst := make(map[int]int) + for k, v := range src { + dst[k] += v + } +} + +func nopeNotAMap(src map[int]string) { + var dst []string + for k, v := range src { + dst[k] = v + } +} + +func nopeNotAMapGeneric[E any, M ~map[int]E, S ~[]E](src M) { + var dst S + for k, v := range src { + dst[k] = v + } +} + +func nopeHasImplicitWidening(src map[string]int) { + dst := make(map[string]any) + for k, v := range src { + dst[k] = v + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/mapsloop_dot.go b/pkg/golinters/modernize/testdata/fix/out/mapsloop_dot.go new file mode 100644 index 000000000000..dc103b982233 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/mapsloop_dot.go @@ -0,0 +1,25 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package mapsloop + +import . "maps" + +var _ = Clone[M] // force "maps" import so that each diagnostic doesn't add one + +type M map[int]string + +func useCopyDot(dst, src map[int]string) { + // Replace loop by maps.Copy. + // want "Replace m\\[k\\]=v loop with maps.Copy" + Copy(dst, src) +} + +func useCloneDot(src map[int]string) { + // Clone is tempting but wrong when src may be nil; see #71844. + + // Replace make(...) by maps.Copy. + dst := make(map[int]string, len(src)) + // want "Replace m\\[k\\]=v loop with maps.Copy" + Copy(dst, src) + println(dst) +} diff --git a/pkg/golinters/modernize/testdata/fix/out/minmax.go b/pkg/golinters/modernize/testdata/fix/out/minmax.go new file mode 100644 index 000000000000..3409bc867ed8 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/minmax.go @@ -0,0 +1,172 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package minmax + +func ifmin(a, b int) { + x := max( + // A + // B + a, + // want "if statement can be modernized using max" + // C + // D + // E + b) + print(x) +} + +func ifmax(a, b int) { + x := min(a, + // want "if statement can be modernized using min" + b) + print(x) +} + +func ifminvariant(a, b int) { + x := min(a, + // want "if statement can be modernized using min" + b) + print(x) +} + +func ifmaxvariant(a, b int) { + x := min(a, + // want "if statement can be modernized using min" + b) + print(x) +} + +func ifelsemin(a, b int) { + var x int // A + // B + x = min( + // want "if/else statement can be modernized using min" + // C + // D + // E + a, + // F + // G + // H + b) + print(x) +} + +func ifelsemax(a, b int) { + // A + var x int // B + // C + x = max( + // want "if/else statement can be modernized using max" + // D + // E + // F + a, + // G + b) + print(x) +} + +func shadowed() int { + hour, min := 3600, 60 + + var time int + if hour < min { // silent: the built-in min function is shadowed here + time = hour + } else { + time = min + } + return time +} + +func nopeIfStmtHasInitStmt() { + x := 1 + if y := 2; y < x { // silent: IfStmt has an Init stmt + x = y + } + print(x) +} + +// Regression test for a bug: fix was "y := max(x, y)". +func oops() { + x := 1 + y := max(x, + // want "if statement can be modernized using max" + 2) + print(y) +} + +// Regression test for a bug: += is not a simple assignment. +func nopeAssignHasIncrementOperator() { + x := 1 + y := 0 + y += 2 + if x > y { + y = x + } + print(y) +} + +// Regression test for https://github.com/golang/go/issues/71721. +func nopeNotAMinimum(x, y int) int { + // A value of -1 or 0 will use a default value (30). + if x <= 0 { + y = 30 + } else { + y = x + } + return y +} + +// Regression test for https://github.com/golang/go/issues/71847#issuecomment-2673491596 +func nopeHasElseBlock(x int) int { + y := x + // Before, this was erroneously reduced to y = max(x, 0) + if y < 0 { + y = 0 + } else { + y += 2 + } + return y +} + +func fix72727(a, b int) { + o := max( + // some important comment. DO NOT REMOVE. + a-42, + // want "if statement can be modernized using max" + b) +} + +type myfloat float64 + +// The built-in min/max differ in their treatment of NaN, +// so reject floating-point numbers (#72829). +func nopeFloat(a, b myfloat) (res myfloat) { + if a < b { + res = a + } else { + res = b + } + return +} + +// Regression test for golang/go#72928. +func underscoreAssign(a, b int) { + if a > b { + _ = a + } +} + +// Regression test for https://github.com/golang/go/issues/73576. +func nopeIfElseIf(a int) int { + x := 0 + if a < 0 { + x = 0 + } else if a > 100 { + x = 100 + } else { + x = a + } + return x +} diff --git a/pkg/golinters/modernize/testdata/fix/out/rangeint.go b/pkg/golinters/modernize/testdata/fix/out/rangeint.go new file mode 100644 index 000000000000..f6be19a410db --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/rangeint.go @@ -0,0 +1,264 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package rangeint + +import ( + "os" + os1 "os" +) + +func _(i int, s struct{ i int }, slice []int) { + for i := range 10 { // want "for loop can be modernized using range over int" + println(i) + } + for j := range 10 { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int8(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int16(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int32(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int64(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range uint8(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range uint16(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range uint32(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range uint64(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int8(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range int8(10) { // want "for loop can be modernized using range over int" + println(j) + } + for j := range os1.FileMode(10) { // want "for loop can be modernized using range over int" + println(j) + } + + { + var i int + for i = range 10 { // want "for loop can be modernized using range over int" + } + // NB: no uses of i after loop. + } + for range 10 { // want "for loop can be modernized using range over int" + // i unused within loop + } + for i := range slice { // want "for loop can be modernized using range over int" + println(slice[i]) + } + for range len("") { // want "for loop can be modernized using range over int" + // NB: not simplified to range "" + } + + // nope + for j := .0; j < 10; j++ { // nope: j is a float type + println(j) + } + for j := float64(0); j < 10; j++ { // nope: j is a float type + println(j) + } + for i := 0; i < 10; { // nope: missing increment + } + for i := 0; i < 10; i-- { // nope: negative increment + } + for i := 0; ; i++ { // nope: missing comparison + } + for i := 0; i <= 10; i++ { // nope: wrong comparison + } + for ; i < 10; i++ { // nope: missing init + } + for s.i = 0; s.i < 10; s.i++ { // nope: not an ident + } + for i := 0; i < 10; i++ { // nope: takes address of i + println(&i) + } + for i := 0; i < 10; i++ { // nope: increments i + i++ + } + for i := 0; i < 10; i++ { // nope: assigns i + i = 8 + } + + // The limit expression must be loop invariant; + // see https://github.com/golang/go/issues/72917 + for i := 0; i < f(); i++ { // nope + } + { + var s struct{ limit int } + for i := 0; i < s.limit; i++ { // nope: limit is not a const or local var + } + } + { + const k = 10 + for range k { // want "for loop can be modernized using range over int" + } + } + { + var limit = 10 + for range limit { // want "for loop can be modernized using range over int" + } + } + { + var limit = 10 + for i := 0; i < limit; i++ { // nope: limit is address-taken + } + print(&limit) + } + { + limit := 10 + limit++ + for i := 0; i < limit; i++ { // nope: limit is assigned other than by its declaration + } + } + for i := 0; i < Global; i++ { // nope: limit is an exported global var; may be updated elsewhere + } + for range table { // want "for loop can be modernized using range over int" + } + { + s := []string{} + for i := 0; i < len(s); i++ { // nope: limit is not loop-invariant + s = s[1:] + } + } + for i := 0; i < len(slice); i++ { // nope: i is incremented within loop + i += 1 + } + for Global = 0; Global < 10; Global++ { // nope: loop index is a global variable. + } +} + +var Global int + +var table = []string{"hello", "world"} + +func f() int { return 0 } + +// Repro for part of #71847: ("for range n is invalid if the loop body contains i++"): +func _(s string) { + var i int // (this is necessary) + for i = 0; i < len(s); i++ { // nope: loop body increments i + if true { + i++ // nope + } + } +} + +// Repro for #71952: for and range loops have different final values +// on i (n and n-1, respectively) so we can't offer the fix if i is +// used after the loop. +func nopePostconditionDiffers() { + i := 0 + for i = 0; i < 5; i++ { + println(i) + } + println(i) // must print 5, not 4 +} + +// Non-integer untyped constants need to be explicitly converted to int. +func issue71847d() { + const limit = 1e3 // float + for range int(limit) { // want "for loop can be modernized using range over int" + } + for range int(limit) { // want "for loop can be modernized using range over int" + } + for range uint(limit) { // want "for loop can be modernized using range over int" + } + + const limit2 = 1 + 0i // complex + for range int(limit2) { // want "for loop can be modernized using range over int" + } +} + +func issue72726() { + var n, kd int + for i := range n { // want "for loop can be modernized using range over int" + // nope: j will be invisible once it's refactored to 'for j := range min(n-j, kd+1)' + for j := 0; j < min(n-j, kd+1); j++ { // nope + _, _ = i, j + } + } + + for i := 0; i < i; i++ { // nope + } + + var i int + for i = 0; i < i/2; i++ { // nope + } + + var arr []int + for i = 0; i < arr[i]; i++ { // nope + } +} + +func todo() { + for j := range os1.FileMode(10) { // want "for loop can be modernized using range over int" + println(j) + } +} + +type T uint +type TAlias = uint + +func Fn(a int) T { + return T(a) +} + +func issue73037() { + var q T + for a := range q { // want "for loop can be modernized using range over int" + println(a) + } + for a := Fn(0); a < q; a++ { + println(a) + } + var qa TAlias + for a := range qa { // want "for loop can be modernized using range over int" + println(a) + } + for a := range T(10) { // want "for loop can be modernized using range over int" + for b := range T(10) { // want "for loop can be modernized using range over int" + println(a, b) + } + } +} + +func issue75289() { + // A use of i within a defer may be textually before the loop but runs + // after, so it should cause the loop to be rejected as a candidate + // to avoid it observing a different final value of i. + { + var i int + defer func() { println(i) }() + for i = 0; i < 10; i++ { // nope: i is accessed after the loop (via defer) + } + } + + // A use of i within a defer within the loop is also a dealbreaker. + { + var i int + for i = 0; i < 10; i++ { // nope: i is accessed after the loop (via defer) + defer func() { println(i) }() + } + } + + // This (outer) defer is irrelevant. + defer func() { + var i int + for i = range 10 { // want "for loop can be modernized using range over int" + } + }() +} diff --git a/pkg/golinters/modernize/testdata/fix/out/reflecttypefor.go b/pkg/golinters/modernize/testdata/fix/out/reflecttypefor.go new file mode 100644 index 000000000000..9791d6841bcf --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/reflecttypefor.go @@ -0,0 +1,23 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package reflecttypefor + +import ( + "io" + "reflect" + "time" +) + +var ( + x any + _ = reflect.TypeOf(x) // nope (dynamic) + _ = reflect.TypeFor[int]() // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeFor[uint]() // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(error(nil)) // nope (likely a mistake) + _ = reflect.TypeFor[*error]() // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(io.Reader(nil)) // nope (likely a mistake) + _ = reflect.TypeFor[*io.Reader]() // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeOf(*new(time.Time)) // nope (false negative of noEffects) + _ = reflect.TypeFor[time.Time]() // want "reflect.TypeOf call can be simplified using TypeFor" + _ = reflect.TypeFor[time.Duration]() // want "reflect.TypeOf call can be simplified using TypeFor" +) diff --git a/pkg/golinters/modernize/testdata/fix/out/slicescontains.go b/pkg/golinters/modernize/testdata/fix/out/slicescontains.go new file mode 100644 index 000000000000..6165ced45014 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/slicescontains.go @@ -0,0 +1,157 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package slicescontains + +import "slices" + +var _ = slices.Contains[[]int] // force import of "slices" to avoid duplicate import edits + +func nopeNoBreak(slice []int, needle int) { + for i := range slice { + if slice[i] == needle { + println("found") + } + } +} + +func rangeIndex(slice []int, needle int) { + if slices.Contains(slice, needle) { + println("found") + } +} + +func rangeValue(slice []int, needle int) { + if slices.Contains(slice, needle) { + println("found") + } +} + +func returns(slice []int, needle int) { + if slices.Contains(slice, needle) { + println("found") + return + } +} + +func assignTrueBreak(slice []int, needle int) { + found := slices.Contains(slice, needle) + print(found) +} + +func assignFalseBreak(slice []int, needle int) { // TODO: treat this specially like booleanTrue + found := true + if slices.Contains(slice, needle) { + found = false + } + print(found) +} + +func assignFalseBreakInSelectSwitch(slice []int, needle int) { + // Exercise RangeStmt in CommClause, CaseClause. + select { + default: + found := slices.Contains(slice, needle) + print(found) + } + switch { + default: + found := slices.Contains(slice, needle) + print(found) + } +} + +func returnTrue(slice []int, needle int) bool { + return slices.Contains(slice, needle) +} + +func returnFalse(slice []int, needle int) bool { + return !slices.Contains(slice, needle) +} + +func containsFunc(slice []int, needle int) bool { + return slices.ContainsFunc(slice, predicate) +} + +func nopeLoopBodyHasFreeContinuation(slice []int, needle int) bool { + for _, elem := range slice { + if predicate(elem) { + if needle == 7 { + continue // this statement defeats loop elimination + } + return true + } + } + return false +} + +func generic[T any](slice []T, f func(T) bool) bool { + return slices.ContainsFunc(slice, f) +} + +func predicate(int) bool + +// Regression tests for bad fixes when needle +// and haystack have different types (#71313): + +func nopeNeedleHaystackDifferentTypes(x any, args []error) { + for _, arg := range args { + if arg == x { + return + } + } +} + +func nopeNeedleHaystackDifferentTypes2(x error, args []any) { + for _, arg := range args { + if arg == x { + return + } + } +} + +func nopeVariadicNamedContainsFunc(slice []int) bool { + for _, elem := range slice { + if variadicPredicate(elem) { + return true + } + } + return false +} + +func variadicPredicate(int, ...any) bool + +func nopeVariadicContainsFunc(slice []int) bool { + f := func(int, ...any) bool { + return true + } + for _, elem := range slice { + if f(elem) { + return true + } + } + return false +} + +// Negative test case for implicit C->I conversion +type I interface{ F() } +type C int + +func (C) F() {} + +func nopeImplicitConversionContainsFunc(slice []C, f func(I) bool) bool { + for _, elem := range slice { + if f(elem) { // implicit conversion from C to I + return true + } + } + return false +} + +func nopeTypeParamWidening[T any](slice []T, f func(any) bool) bool { + for _, elem := range slice { + if f(elem) { // implicit conversion from T to any + return true + } + } + return false +} diff --git a/pkg/golinters/modernize/testdata/fix/out/slicessort.go b/pkg/golinters/modernize/testdata/fix/out/slicessort.go new file mode 100644 index 000000000000..4b2914b86a6a --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/slicessort.go @@ -0,0 +1,39 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package slicessort + +import "slices" + +import "sort" + +type myint int + +func _(s []myint) { + slices.Sort(s) // want "sort.Slice can be modernized using slices.Sort" +} + +func _(x *struct{ s []int }) { + slices.Sort(x.s) // want "sort.Slice can be modernized using slices.Sort" +} + +func _(s []int) { + sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator +} + +func _(s []int) { + sort.Slice(s, func(i, j int) bool { return s[j] < s[i] }) // nope: wrong index var +} + +func _(sense bool, s2 []struct{ x int }) { + sort.Slice(s2, func(i, j int) bool { return s2[i].x < s2[j].x }) // nope: not a simple index operation + + // Regression test for a crash: the sole statement of a + // comparison func body is not necessarily a return! + sort.Slice(s2, func(i, j int) bool { + if sense { + return s2[i].x < s2[j].x + } else { + return s2[i].x > s2[j].x + } + }) +} diff --git a/pkg/golinters/modernize/testdata/fix/out/splitseq.go b/pkg/golinters/modernize/testdata/fix/out/splitseq.go new file mode 100644 index 000000000000..307b7a8d2455 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/splitseq.go @@ -0,0 +1,42 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package splitseq + +import ( + "bytes" + "strings" +) + +func _() { + for line := range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + println(line) + } + for i, line := range strings.Split("", "") { // nope: uses index var + println(i, line) + } + for i, _ := range strings.Split("", "") { // nope: uses index var + println(i) + } + for i := range strings.Split("", "") { // nope: uses index var + println(i) + } + for range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range strings.SplitSeq("", "") { // want "Ranging over SplitSeq is more efficient" + } + for range bytes.SplitSeq(nil, nil) { // want "Ranging over SplitSeq is more efficient" + } + { + lines := strings.SplitSeq("", "") // want "Ranging over SplitSeq is more efficient" + for line := range lines { + println(line) + } + } + { + lines := strings.Split("", "") // nope: lines is used not just by range + for _, line := range lines { + println(line) + } + println(lines) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/stditerators.go b/pkg/golinters/modernize/testdata/fix/out/stditerators.go new file mode 100644 index 000000000000..2859b265197d --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/stditerators.go @@ -0,0 +1,60 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stditerators + +import "go/types" + +func _(tuple *types.Tuple) { + for v := range tuple.Variables() { // want "Len/At loop can simplified using Tuple.Variables iteration" + print(v) + } +} + +func _(scope *types.Scope) { + for child := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + print(child) + } + { + const child = 0 // shadowing of preferred name at def + for child0 := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + print(child0) + } + } + { + for i := 0; i < scope.NumChildren(); i++ { + const child = 0 // nope: shadowing of fresh name at use + print(scope.Child(i)) + } + } + { + for elem := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + elem := elem // => preferred name = "elem" + print(elem) + } + } + { + for child := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration" + first := scope.Child(0) // the name heuristic should not be fooled by this + print(first, child) + } + } +} + +func _(union, union2 *types.Union) { + for term := range union.Terms() { // want "Len/Term loop can simplified using Union.Terms iteration" + print(term) + print(term) + } + for i := union.Len() - 1; i >= 0; i-- { // nope: wrong loop form + print(union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: wrong loop form + print(union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: use of i not in x.At(i) + print(i, union.Term(i)) + } + for i := 0; i <= union.Len(); i++ { // nope: x.At and x.Len have different receivers + print(i, union2.Term(i)) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/stringsbuilder.go b/pkg/golinters/modernize/testdata/fix/out/stringsbuilder.go new file mode 100644 index 000000000000..7cafa11eeabe --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/stringsbuilder.go @@ -0,0 +1,108 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stringsbuilder + +import "strings" + +// basic test +func _() { + var s strings.Builder + s.WriteString("before") + for range 10 { + s.WriteString("in") // want "using string \\+= string in a loop is inefficient" + s.WriteString("in2") + } + s.WriteString("after") + print(s.String()) +} + +// with initializer +func _() { + var s strings.Builder + s.WriteString("a") + for range 10 { + s.WriteString("b") // want "using string \\+= string in a loop is inefficient" + } + print(s.String()) +} + +// with empty initializer +func _() { + var s strings.Builder + for range 10 { + s.WriteString("b") // want "using string \\+= string in a loop is inefficient" + } + print(s.String()) +} + +// with short decl +func _() { + var s strings.Builder + s.WriteString("a") + for range 10 { + s.WriteString("b") // want "using string \\+= string in a loop is inefficient" + } + print(s.String()) +} + +// with short decl and empty initializer +func _() { + var s strings.Builder + for range 10 { + s.WriteString("b") // want "using string \\+= string in a loop is inefficient" + } + print(s.String()) +} + +// nope: += must appear at least once within a loop. +func _() { + var s string + s += "a" + s += "b" + s += "c" + print(s) +} + +// nope: the declaration of s is not in a block. +func _() { + if s := "a"; true { + for range 10 { + s += "x" + } + print(s) + } +} + +// in a switch (special case of "in a block" logic) +func _() { + switch { + default: + var s strings.Builder + s.WriteString("a") + for range 10 { + s.WriteString("b") // want "using string \\+= string in a loop is inefficient" + } + print(s.String()) + } +} + +// nope: don't handle direct assignments to the string (only +=). +func _(x string) string { + var s string + s = x + for range 3 { + s += "" + x + } + return s +} + +// Regression test for bug in a GenDecl with parens. +func issue75318(slice []string) string { + var ( + msg strings.Builder + ) + for _, s := range slice { + msg.WriteString(s) // want "using string \\+= string in a loop is inefficient" + } + return msg.String() +} diff --git a/pkg/golinters/modernize/testdata/fix/out/stringscutprefix.go b/pkg/golinters/modernize/testdata/fix/out/stringscutprefix.go new file mode 100644 index 000000000000..27b62ff7ef05 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/stringscutprefix.go @@ -0,0 +1,166 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package stringscutprefix + +import ( + "strings" +) + +var ( + s, pre, suf string +) + +// test supported cases of pattern 1 - CutPrefix +func _() { + if after, ok := strings.CutPrefix(s, pre); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a := after + _ = a + } + if after, ok := strings.CutPrefix("", ""); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a := after + _ = a + } + if after, ok := strings.CutPrefix(s, ""); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + println([]byte(after)) + } + if after, ok := strings.CutPrefix(s, ""); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b := "", after + _, _ = a, b + } + if after, ok := strings.CutPrefix(s, ""); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b := after, strings.TrimPrefix(s, "") // only replace the first occurrence + s = "123" + b = strings.TrimPrefix(s, "") // only replace the first occurrence + _, _ = a, b + } + + var a, b string + if after, ok := strings.CutPrefix(s, ""); ok { // want "HasPrefix \\+ TrimPrefix can be simplified to CutPrefix" + a, b = "", after + _, _ = a, b + } +} + +// test basic cases for CutSuffix - only covering the key differences +func _() { + if before, ok := strings.CutSuffix(s, suf); ok { // want "HasSuffix \\+ TrimSuffix can be simplified to CutSuffix" + a := before + _ = a + } + if before, ok := strings.CutSuffix(s, ""); ok { // want "HasSuffix \\+ TrimSuffix can be simplified to CutSuffix" + println([]byte(before)) + } +} + +// test cases that are not supported by pattern1 - CutPrefix +func _() { + ok := strings.HasPrefix("", "") + if ok { // noop, currently it doesn't track the result usage of HasPrefix + a := strings.TrimPrefix("", "") + _ = a + } + if strings.HasPrefix(s, pre) { + a := strings.TrimPrefix("", "") // noop, as the argument isn't the same + _ = a + } + if strings.HasPrefix(s, pre) { + var result string + result = strings.TrimPrefix("", "") // noop, as we believe define is more popular. + _ = result + } + if strings.HasPrefix("", "") { + a := strings.TrimPrefix(s, pre) // noop, as the argument isn't the same + _ = a + } + if s1 := s; strings.HasPrefix(s1, pre) { + a := strings.TrimPrefix(s1, pre) // noop, as IfStmt.Init is present + _ = a + } +} + +// test basic unsupported case for CutSuffix +func _() { + if strings.HasSuffix(s, suf) { + a := strings.TrimSuffix("", "") // noop, as the argument isn't the same + _ = a + } +} + +var value0 string + +// test supported cases of pattern2 - CutPrefix +func _() { + if after, ok := strings.CutPrefix(s, pre); ok { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + if after, ok := strings.CutPrefix(s, pre); ok { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + if after, ok := strings.CutPrefix(s, pre); ok { // want "TrimPrefix can be simplified to CutPrefix" + println(strings.TrimPrefix(s, pre)) // noop here + } + if after, ok := strings.CutPrefix(s, ""); ok { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + var ok bool // define an ok variable to test the fix won't shadow it for its if stmt body + _ = ok + if after, ok0 := strings.CutPrefix(s, pre); ok0 { // want "TrimPrefix can be simplified to CutPrefix" + println(after) + } + var predefined string + if predefined = strings.TrimPrefix(s, pre); s != predefined { // noop + println(predefined) + } + if predefined = strings.TrimPrefix(s, pre); s != predefined { // noop + println(&predefined) + } + var value string + if value = strings.TrimPrefix(s, pre); s != value { // noop + println(value) + } + lhsMap := make(map[string]string) + if lhsMap[""] = strings.TrimPrefix(s, pre); s != lhsMap[""] { // noop + println(lhsMap[""]) + } + arr := make([]string, 0) + if arr[0] = strings.TrimPrefix(s, pre); s != arr[0] { // noop + println(arr[0]) + } + type example struct { + field string + } + var e example + if e.field = strings.TrimPrefix(s, pre); s != e.field { // noop + println(e.field) + } +} + +// test basic cases for pattern2 - CutSuffix +func _() { + if before, ok := strings.CutSuffix(s, suf); ok { // want "TrimSuffix can be simplified to CutSuffix" + println(before) + } + if before, ok := strings.CutSuffix(s, suf); ok { // want "TrimSuffix can be simplified to CutSuffix" + println(before) + } +} + +// test cases that not supported by pattern2 - CutPrefix +func _() { + if after := strings.TrimPrefix(s, pre); s != pre { // noop + println(after) + } + if after := strings.TrimPrefix(s, pre); after != pre { // noop + println(after) + } + if strings.TrimPrefix(s, pre) != s { + println(strings.TrimPrefix(s, pre)) + } +} + +// test basic unsupported case for pattern2 - CutSuffix +func _() { + if before := strings.TrimSuffix(s, suf); s != suf { // noop + println(before) + } +} diff --git a/pkg/golinters/modernize/testdata/fix/out/testingcontext_test.go b/pkg/golinters/modernize/testdata/fix/out/testingcontext_test.go new file mode 100644 index 000000000000..06e72a3bb3c1 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/testingcontext_test.go @@ -0,0 +1,72 @@ +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package testingcontext + +import ( + "context" + "testing" +) + +func Test(t *testing.T) { + ctx := t.Context() + _ = ctx + + func() { + ctx, cancel := context.WithCancel(context.Background()) // Nope. scope of defer is not the testing func. + defer cancel() + _ = ctx + }() + + { + ctx := t.Context() + _ = ctx + var t int // not in scope of the call to WithCancel + _ = t + } + + { + ctx := context.Background() + ctx, cancel := context.WithCancel(context.Background()) // Nope. ctx is redeclared. + defer cancel() + _ = ctx + } + + { + var t int + ctx, cancel := context.WithCancel(context.Background()) // Nope. t is shadowed. + defer cancel() + _ = ctx + _ = t + } + + t.Run("subtest", func(t2 *testing.T) { + ctx := t2.Context() + _ = ctx + }) +} + +func TestAlt(t2 *testing.T) { + ctx := t2.Context() + _ = ctx +} + +func Testnot(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) // Nope. Not a test func. + defer cancel() + _ = ctx +} + +func Benchmark(b *testing.B) { + ctx := b.Context() + _ = ctx + + b.Run("subtest", func(b2 *testing.B) { + ctx := b2.Context() + _ = ctx + }) +} + +func Fuzz(f *testing.F) { + ctx := f.Context() + _ = ctx +} diff --git a/pkg/golinters/modernize/testdata/fix/out/waitgroup.go b/pkg/golinters/modernize/testdata/fix/out/waitgroup.go new file mode 100644 index 000000000000..216bd19222c4 --- /dev/null +++ b/pkg/golinters/modernize/testdata/fix/out/waitgroup.go @@ -0,0 +1,176 @@ +//go:build go1.25 + +//golangcitest:args -Emodernize +//golangcitest:expected_exitcode 0 +package waitgroup + +import ( + "fmt" + "sync" +) + +// supported case for pattern 1. +func _() { + var wg sync.WaitGroup + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + }) + + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + }) + + for range 10 { + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + }) + } +} + +// supported case for pattern 2. +func _() { + var wg sync.WaitGroup + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + }) + + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + }) + + for range 10 { + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + }) + } +} + +// this function puts some wrong usages but waitgroup modernizer will still offer fixes. +func _() { + var wg sync.WaitGroup + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + defer wg.Done() + fmt.Println() + }) + + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + wg.Done() + }) + + // want "Goroutine creation can be simplified using WaitGroup.Go" + wg.Go(func() { + fmt.Println() + wg.Done() + }) +} + +// this function puts the unsupported cases of pattern 1. +func _() { + var wg sync.WaitGroup + wg.Add(1) + go func() {}() + + wg.Add(1) + go func(i int) { + defer wg.Done() + fmt.Println(i) + }(1) + + wg.Add(1) + go func() { + fmt.Println() + defer wg.Done() + }() + + wg.Add(1) + go func() { // noop: no wg.Done call inside function body. + fmt.Println() + }() + + go func() { // noop: no Add call before this go stmt. + defer wg.Done() + fmt.Println() + }() + + wg.Add(2) // noop: only support Add(1). + go func() { + defer wg.Done() + }() + + var wg1 sync.WaitGroup + wg1.Add(1) // noop: Add and Done should be the same object. + go func() { + defer wg.Done() + fmt.Println() + }() + + wg.Add(1) // noop: Add and Done should be the same object. + go func() { + defer wg1.Done() + fmt.Println() + }() +} + +// this function puts the unsupported cases of pattern 2. +func _() { + var wg sync.WaitGroup + wg.Add(1) + go func() { + wg.Done() + fmt.Println() + }() + + go func() { // noop: no Add call before this go stmt. + fmt.Println() + wg.Done() + }() + + var wg1 sync.WaitGroup + wg1.Add(1) // noop: Add and Done should be the same object. + go func() { + fmt.Println() + wg.Done() + }() + + wg.Add(1) // noop: Add and Done should be the same object. + go func() { + fmt.Println() + wg1.Done() + }() +} + +type Server struct { + wg sync.WaitGroup +} + +type ServerContainer struct { + serv Server +} + +func _() { + var s Server + // want "Goroutine creation can be simplified using WaitGroup.Go" + s.wg.Go(func() { + print() + }) + + var sc ServerContainer + // want "Goroutine creation can be simplified using WaitGroup.Go" + sc.serv.wg.Go(func() { + print() + }) + + var wg sync.WaitGroup + arr := [1]*sync.WaitGroup{&wg} + // want "Goroutine creation can be simplified using WaitGroup.Go" + arr[0].Go(func() { + print() + }) +} diff --git a/pkg/golinters/modernize/testdata/modernize_any.go b/pkg/golinters/modernize/testdata/modernize_any.go new file mode 100644 index 000000000000..7802e6f4cbd1 --- /dev/null +++ b/pkg/golinters/modernize/testdata/modernize_any.go @@ -0,0 +1,11 @@ +//golangcitest:args -Emodernize +package testdata + +func _(x interface{}) {} // want "interface{} can be replaced by any" + +func _() { + var x interface{} // want "interface{} can be replaced by any" + const any = 1 + var y interface{} // nope: any is shadowed here + _, _ = x, y +} diff --git a/pkg/golinters/modernize/testdata/modernize_custom.go b/pkg/golinters/modernize/testdata/modernize_custom.go new file mode 100644 index 000000000000..2d386e713fa5 --- /dev/null +++ b/pkg/golinters/modernize/testdata/modernize_custom.go @@ -0,0 +1,13 @@ +//golangcitest:args -Emodernize +//golangcitest:config_path testdata/modernize_custom.yml +//golangcitest:expected_exitcode 0 +package testdata + +func _(x interface{}) {} + +func _() { + var x interface{} + const any = 1 + var y interface{} // nope: any is shadowed here + _, _ = x, y +} diff --git a/pkg/golinters/modernize/testdata/modernize_custom.yml b/pkg/golinters/modernize/testdata/modernize_custom.yml new file mode 100644 index 000000000000..1aeb999f29d3 --- /dev/null +++ b/pkg/golinters/modernize/testdata/modernize_custom.yml @@ -0,0 +1,7 @@ +version: "2" + +linters: + settings: + modernize: + disable: + - any diff --git a/pkg/lint/lintersdb/builder_linter.go b/pkg/lint/lintersdb/builder_linter.go index 72e02f7a6b3a..6e9cef1d1b36 100644 --- a/pkg/lint/lintersdb/builder_linter.go +++ b/pkg/lint/lintersdb/builder_linter.go @@ -73,6 +73,7 @@ import ( "github.com/golangci/golangci-lint/v2/pkg/golinters/mirror" "github.com/golangci/golangci-lint/v2/pkg/golinters/misspell" "github.com/golangci/golangci-lint/v2/pkg/golinters/mnd" + "github.com/golangci/golangci-lint/v2/pkg/golinters/modernize" "github.com/golangci/golangci-lint/v2/pkg/golinters/musttag" "github.com/golangci/golangci-lint/v2/pkg/golinters/nakedret" "github.com/golangci/golangci-lint/v2/pkg/golinters/nestif" @@ -382,6 +383,11 @@ func (LinterBuilder) Build(cfg *config.Config) ([]*linter.Config, error) { WithSince("v1.22.0"). WithURL("https://github.com/tommy-muehle/go-mnd"), + linter.NewConfig(modernize.New(&cfg.Linters.Settings.Modernize)). + WithSince("v2.6.0"). + WithLoadForGoAnalysis(). + WithURL("https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/modernize"), + linter.NewConfig(gomoddirectives.New(&cfg.Linters.Settings.GoModDirectives)). WithSince("v1.39.0"). WithURL("https://github.com/ldez/gomoddirectives"),