From 374bb098b41f27c9a3cfbbe7c2bdfa661c398dff Mon Sep 17 00:00:00 2001 From: Clifton Kaznocha Date: Sun, 8 Sep 2024 21:29:30 -0700 Subject: [PATCH] Warn about loops using `range len(b)` with arrays/slices (#28) --- go.mod | 2 +- go.work | 2 +- intrange.go | 287 ++++++++++++++++++++++++-------------- intrange_test.go | 2 +- testdata/go.mod | 2 +- testdata/main.go | 71 ++++++++++ testdata/main.go.golden | 299 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 559 insertions(+), 106 deletions(-) create mode 100644 testdata/main.go.golden diff --git a/go.mod b/go.mod index 44f8a25..94b1079 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/ckaznocha/intrange -go 1.21 +go 1.22 require ( github.com/gostaticanalysis/testutil v0.4.0 diff --git a/go.work b/go.work index f41a04a..3814c99 100644 --- a/go.work +++ b/go.work @@ -1,4 +1,4 @@ -go 1.22.0 +go 1.22 use ( . diff --git a/intrange.go b/intrange.go index fac4e3d..33cddf3 100644 --- a/intrange.go +++ b/intrange.go @@ -5,6 +5,7 @@ import ( "fmt" "go/ast" "go/token" + "go/types" "strconv" "golang.org/x/tools/go/analysis" @@ -23,7 +24,10 @@ var ( errFailedAnalysis = errors.New("failed analysis") ) -const msg = "for loop can be changed to use an integer range (Go 1.22+)" +const ( + msg = "for loop can be changed to use an integer range (Go 1.22+)" + msgLenRange = "for loop can be changed to `i := range %s`" +) func run(pass *analysis.Pass) (any, error) { result, ok := pass.ResultOf[inspect.Analyzer] @@ -44,90 +48,134 @@ func run(pass *analysis.Pass) (any, error) { ) } - resultInspector.Preorder([]ast.Node{(*ast.ForStmt)(nil)}, check(pass)) + resultInspector.Preorder([]ast.Node{(*ast.ForStmt)(nil), (*ast.RangeStmt)(nil)}, check(pass)) return nil, nil } func check(pass *analysis.Pass) func(node ast.Node) { return func(node ast.Node) { - forStmt, ok := node.(*ast.ForStmt) - if !ok { + switch stmt := node.(type) { + case *ast.ForStmt: + checkForStmt(pass, stmt) + case *ast.RangeStmt: + checkRangeStmt(pass, stmt) + default: return } + } +} + +func checkForStmt(pass *analysis.Pass, forStmt *ast.ForStmt) { + // Existing checks for other patterns + if forStmt.Init == nil || forStmt.Cond == nil || forStmt.Post == nil { + return + } + + // i := 0;; + init, ok := forStmt.Init.(*ast.AssignStmt) + if !ok { + return + } + + if len(init.Lhs) != 1 || len(init.Rhs) != 1 { + return + } + + initIdent, ok := init.Lhs[0].(*ast.Ident) + if !ok { + return + } + + if !compareNumberLit(init.Rhs[0], 0) { + return + } + + cond, ok := forStmt.Cond.(*ast.BinaryExpr) + if !ok { + return + } - if forStmt.Init == nil || forStmt.Cond == nil || forStmt.Post == nil { + var nExpr ast.Expr + + switch cond.Op { + case token.LSS: // ;i < n; + if isBenchmark(cond.Y) { return } - // i := 0;; - init, ok := forStmt.Init.(*ast.AssignStmt) + nExpr = findNExpr(cond.Y) + + x, ok := cond.X.(*ast.Ident) if !ok { return } - if len(init.Lhs) != 1 || len(init.Rhs) != 1 { + if x.Name != initIdent.Name { + return + } + case token.GTR: // ;n > i; + if isBenchmark(cond.X) { return } - initIdent, ok := init.Lhs[0].(*ast.Ident) + nExpr = findNExpr(cond.X) + + y, ok := cond.Y.(*ast.Ident) if !ok { return } - if !compareNumberLit(init.Rhs[0], 0) { + if y.Name != initIdent.Name { return } + default: + return + } - cond, ok := forStmt.Cond.(*ast.BinaryExpr) - if !ok { + switch post := forStmt.Post.(type) { + case *ast.IncDecStmt: // ;;i++ + if post.Tok != token.INC { return } - var nExpr ast.Expr + ident, ok := post.X.(*ast.Ident) + if !ok { + return + } - switch cond.Op { - case token.LSS: // ;i < n; - if isBenchmark(cond.Y) { + if ident.Name != initIdent.Name { + return + } + case *ast.AssignStmt: + switch post.Tok { + case token.ADD_ASSIGN: // ;;i += 1 + if len(post.Lhs) != 1 { return } - nExpr = findNExpr(cond.Y) - - x, ok := cond.X.(*ast.Ident) + ident, ok := post.Lhs[0].(*ast.Ident) if !ok { return } - if x.Name != initIdent.Name { - return - } - case token.GTR: // ;n > i; - if isBenchmark(cond.X) { + if ident.Name != initIdent.Name { return } - nExpr = findNExpr(cond.X) - - y, ok := cond.Y.(*ast.Ident) - if !ok { + if len(post.Rhs) != 1 { return } - if y.Name != initIdent.Name { + if !compareNumberLit(post.Rhs[0], 1) { return } - default: - return - } - - switch post := forStmt.Post.(type) { - case *ast.IncDecStmt: // ;;i++ - if post.Tok != token.INC { + case token.ASSIGN: // ;;i = i + 1 && ;;i = 1 + i + if len(post.Lhs) != 1 || len(post.Rhs) != 1 { return } - ident, ok := post.X.(*ast.Ident) + ident, ok := post.Lhs[0].(*ast.Ident) if !ok { return } @@ -135,35 +183,31 @@ func check(pass *analysis.Pass) func(node ast.Node) { if ident.Name != initIdent.Name { return } - case *ast.AssignStmt: - switch post.Tok { - case token.ADD_ASSIGN: // ;;i += 1 - if len(post.Lhs) != 1 { - return - } - ident, ok := post.Lhs[0].(*ast.Ident) - if !ok { - return - } + bin, ok := post.Rhs[0].(*ast.BinaryExpr) + if !ok { + return + } - if ident.Name != initIdent.Name { - return - } + if bin.Op != token.ADD { + return + } - if len(post.Rhs) != 1 { + switch x := bin.X.(type) { + case *ast.Ident: // ;;i = i + 1 + if x.Name != initIdent.Name { return } - if !compareNumberLit(post.Rhs[0], 1) { + if !compareNumberLit(bin.Y, 1) { return } - case token.ASSIGN: // ;;i = i + 1 && ;;i = 1 + i - if len(post.Lhs) != 1 || len(post.Rhs) != 1 { + case *ast.BasicLit: // ;;i = 1 + i + if !compareNumberLit(x, 1) { return } - ident, ok := post.Lhs[0].(*ast.Ident) + ident, ok := bin.Y.(*ast.Ident) if !ok { return } @@ -171,64 +215,103 @@ func check(pass *analysis.Pass) func(node ast.Node) { if ident.Name != initIdent.Name { return } - - bin, ok := post.Rhs[0].(*ast.BinaryExpr) - if !ok { - return - } - - if bin.Op != token.ADD { - return - } - - switch x := bin.X.(type) { - case *ast.Ident: // ;;i = i + 1 - if x.Name != initIdent.Name { - return - } - - if !compareNumberLit(bin.Y, 1) { - return - } - case *ast.BasicLit: // ;;i = 1 + i - if !compareNumberLit(x, 1) { - return - } - - ident, ok := bin.Y.(*ast.Ident) - if !ok { - return - } - - if ident.Name != initIdent.Name { - return - } - default: - return - } default: return } default: return } + default: + return + } - bc := &bodyChecker{ - initIdent: initIdent, - nExpr: nExpr, - } + bc := &bodyChecker{ + initIdent: initIdent, + nExpr: nExpr, + } - ast.Inspect(forStmt.Body, bc.check) + ast.Inspect(forStmt.Body, bc.check) - if bc.modified { - return - } + if bc.modified { + return + } - pass.Report(analysis.Diagnostic{ - Pos: forStmt.Pos(), - Message: msg, - }) + pass.Report(analysis.Diagnostic{ + Pos: forStmt.Pos(), + Message: msg, + }) +} + +func checkRangeStmt(pass *analysis.Pass, rangeStmt *ast.RangeStmt) { + if rangeStmt.Key == nil { + return } + + ident, ok := rangeStmt.Key.(*ast.Ident) + if !ok { + return + } + + if ident.Name == "_" { + return + } + + if rangeStmt.Value != nil { + return + } + + if rangeStmt.X == nil { + return + } + + x, ok := rangeStmt.X.(*ast.CallExpr) + if !ok { + return + } + + fn, ok := x.Fun.(*ast.Ident) + if !ok { + return + } + + if fn.Name != "len" || len(x.Args) != 1 { + return + } + + arg, ok := x.Args[0].(*ast.Ident) + if !ok { + return + } + + // make sure arg is a slice or array + obj := pass.TypesInfo.ObjectOf(arg) + if obj == nil { + return + } + + switch obj.Type().Underlying().(type) { + case *types.Slice, *types.Array: + default: + return + } + + pass.Report(analysis.Diagnostic{ + Pos: ident.Pos(), + End: x.End(), + Message: fmt.Sprintf(msgLenRange, arg.Name), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("Replace `len(%s)` with `%s`", arg.Name, arg.Name), + TextEdits: []analysis.TextEdit{ + { + Pos: x.Pos(), + End: x.End(), + NewText: []byte(arg.Name), + }, + }, + }, + }, + }) } func findNExpr(expr ast.Expr) ast.Expr { diff --git a/intrange_test.go b/intrange_test.go index 3deab94..11b9529 100644 --- a/intrange_test.go +++ b/intrange_test.go @@ -11,5 +11,5 @@ import ( func TestAnalyzer(t *testing.T) { testdata := testutil.WithModules(t, analysistest.TestData(), nil) - analysistest.Run(t, testdata, intrange.Analyzer) + analysistest.RunWithSuggestedFixes(t, testdata, intrange.Analyzer) } diff --git a/testdata/go.mod b/testdata/go.mod index 19c5166..1260ac4 100644 --- a/testdata/go.mod +++ b/testdata/go.mod @@ -1,5 +1,5 @@ module github.com/ckaznocha/intrange/testdata -go 1.21 +go 1.22 require google.golang.org/protobuf v1.33.0 diff --git a/testdata/main.go b/testdata/main.go index fac909a..1130c8e 100644 --- a/testdata/main.go +++ b/testdata/main.go @@ -226,3 +226,74 @@ func opReEval_IndexExpressions_Map(n map[string]int) { n["N"] = 5 } } + +func issue27() { + var someSlice []interface{} + + for i := range len(someSlice) { // want "for loop can be changed to `i := range someSlice`" + print(i) + } + + for i := range notLen(someSlice) { + print(i) + } + + for i := range len(someSlice) / 2 { + print(i) + } + + for i := range someSlice { + print(i) + } + + for i, _ := range someSlice { + print(i) + } + + var someArray [2]interface{} + + for i := range len(someArray) { // want "for loop can be changed to `i := range someArray`" + print(i) + } + + for i := range notLen(someArray) { + print(i) + } + + for i := range len(someArray) / 2 { + print(i) + } + + for i := range someArray { + print(i) + } + + for i, _ := range someArray { + print(i) + } + + var someMap map[int]interface{} + for i := range len(someMap) { + print(i) + } + + for i := range notLen(someMap) { + print(i) + } + + for i := range len(someMap) / 2 { + print(i) + } + + for i := range someMap { + print(i) + } + + for i, _ := range someMap { + print(i) + } +} + +func notLen(any) int { + return 0 +} diff --git a/testdata/main.go.golden b/testdata/main.go.golden new file mode 100644 index 0000000..e4e7ad5 --- /dev/null +++ b/testdata/main.go.golden @@ -0,0 +1,299 @@ +package main + +import ( + "testing" + + "google.golang.org/protobuf/reflect/protoreflect" +) + +func main() { + for i := 2; i < 10; i++ { + } + + for i := 0; i < 10; i += 2 { + } + + for i := 0; i < 10; i++ { + i += 1 + } + + for i := 0; i < 10; i++ { + i++ + } + + for i := 0; i < 10; i++ { + i = i + 1 + } + + for i := 0; i < 10; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := uint32(0); i < 10; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0x0; i < 10; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i += 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i += 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i = i + 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i = i + 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i = 1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < 10; i = 0x1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0x0; 10 > i; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i += 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i += 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i = i + 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i = i + 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i = 1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; 10 > i; i = 0x1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + const x = 10 + + for i := 2; i < x; i++ { + } + + for i := 0; i < x; i += 2 { + } + + for i := 0; i < x; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := uint32(0); i < uint32(x); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0x0; i < x; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i += 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i += 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i = i + 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i = i + 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i = 1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < x; i = 0x1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i += 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i += 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i = i + 1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i = i + 0x1 { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i = 1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; x > i; i = 0x1 + i { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + var b *testing.B + for i := 0; i < b.N; i++ { + } + + for i := 0; b.N >= i; i++ { + } + + var n int + for i := 0; i < n; i++ { + n-- + } + + for i := 0; i < n; i++ { + n++ + } + + // Example from https://github.com/ckaznocha/intrange/issues/12 + var what string + for i := 0; i < len(what); i++ { + if what[i] == 'v' && i+1 < len(what) && what[i+1] >= '0' && what[i+1] <= '9' { + what = what[:i] + what[i+1:] + } + } + + for i := 0; i < len(what); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + var t struct{ n int } + for i := 0; i < t.n; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < t.n; i++ { + t.n++ + } + + var s []int + for i := 0; i < len(s); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < len(s); i++ { + s = append(s, 4) + } + + var m map[int]int + for i := 0; i < len(m); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < len(m); i++ { + m[4] = 4 + } + + var t2 struct{ m map[int]int } + for i := 0; i < len(t2.m); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + } + + for i := 0; i < len(t2.m); i++ { + t2.m[4] = 4 + } +} + +// https://github.com/ckaznocha/intrange/issues/16 +func issue16(service protoreflect.ServiceDescriptor) { + for i := 0; i < service.Methods().Len(); i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + print(i) + } +} + +func opReEval_IndexExpressions_ArrayLike(n []int) { + for i := 0; i < n[0]; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + print(i) + } + + for i := 0; i < n[0]; i++ { + n[0]++ + } + + for i := 0; i < n[0]; i++ { + n[0] = 5 + } +} + +func opReEval_IndexExpressions_Map(n map[string]int) { + for i := 0; i < n["N"]; i++ { // want `for loop can be changed to use an integer range \(Go 1\.22\+\)` + print(i) + } + + for i := 0; i < n["N"]; i++ { + n["N"]++ + } + + for i := 0; i < n["N"]; i++ { + n["N"] = 5 + } +} + +func issue27() { + var someSlice []interface{} + + for i := range someSlice { // want "for loop can be changed to `i := range someSlice`" + print(i) + } + + for i := range notLen(someSlice) { + print(i) + } + + for i := range len(someSlice) / 2 { + print(i) + } + + for i := range someSlice { + print(i) + } + + for i, _ := range someSlice { + print(i) + } + + var someArray [2]interface{} + + for i := range someArray { // want "for loop can be changed to `i := range someArray`" + print(i) + } + + for i := range notLen(someArray) { + print(i) + } + + for i := range len(someArray) / 2 { + print(i) + } + + for i := range someArray { + print(i) + } + + for i, _ := range someArray { + print(i) + } + + var someMap map[int]interface{} + for i := range len(someMap) { + print(i) + } + + for i := range notLen(someMap) { + print(i) + } + + for i := range len(someMap) / 2 { + print(i) + } + + for i := range someMap { + print(i) + } + + for i, _ := range someMap { + print(i) + } +} + +func notLen(any) int { + return 0 +}