diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml new file mode 100644 index 0000000..db23318 --- /dev/null +++ b/.github/workflows/build-test.yml @@ -0,0 +1,17 @@ +name: build-test +on: + push: + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/setup-go@v1 + with: + go-version: 1.13.x + - uses: actions/checkout@v2 + - name: Build + run: go build . + - name: Test + run: go test -v . diff --git a/README.md b/README.md index 162ede3..d5a7bb6 100644 --- a/README.md +++ b/README.md @@ -177,7 +177,26 @@ for feature := range optionalSet { } ``` - +Note: If performance is absolutely critical, it may be more efficient to use `copy` instead of `append` for larger slices. For reference, see the following benchmark: +```Go +func BenchmarkSize200PreallocateCopy(b *testing.B) { + existing := make([]int64, 200, 200) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Preallocate our initial slice + init := make([]int64, len(existing)) + copy(init, existing) + } +} +``` +``` +$ go test -bench=. -benchmem +goos: linux +goarch: amd64 +BenchmarkSize200NoPreallocate-4 500000 3080 ns/op 4088 B/op 9 allocs/op +BenchmarkSize200Preallocate-4 1000000 1163 ns/op 1792 B/op 1 allocs/op +BenchmarkSize200PreallocateCopy-4 2000000 807 ns/op 1792 B/op 1 allocs/op +``` ## TODO @@ -193,6 +212,6 @@ Pull requests welcome! ## Other static analysis tools -If you've enjoyed prealloc, take a look at my other static anaylsis tools! +If you've enjoyed prealloc, take a look at my other static analysis tools! - [nakedret](https://github.com/alexkohler/nakedret) - Finds naked returns. - [unimport](https://github.com/alexkohler/unimport) - Finds unnecessary import aliases. diff --git a/prealloc.go b/prealloc.go index 1235ad3..528aff5 100644 --- a/prealloc.go +++ b/prealloc.go @@ -1,7 +1,6 @@ package prealloc import ( - "errors" "flag" "fmt" "go/ast" @@ -84,18 +83,6 @@ func checkForPreallocations(args []string, simple, includeRangeLoops *bool, incl return nil, fmt.Errorf("could not parse input %v", err) } - if simple == nil { - return nil, errors.New("simple nil") - } - - if includeRangeLoops == nil { - return nil, errors.New("includeRangeLoops nil") - } - - if includeForLoops == nil { - return nil, errors.New("includeForLoops nil") - } - hints := []Hint{} for _, f := range files { retVis := &returnsVisitor{ @@ -305,6 +292,16 @@ func (v *returnsVisitor) Visit(node ast.Node) ast.Visitor { if len(v.sliceDeclarations) == 0 { continue } + // Check the value being ranged over and ensure it's not a channel (we cannot offer any recommendations on channel ranges). + rangeIdent, ok := s.X.(*ast.Ident) + if ok && rangeIdent.Obj != nil { + valueSpec, ok := rangeIdent.Obj.Decl.(*ast.ValueSpec) + if ok { + if _, rangeTargetIsChannel := valueSpec.Type.(*ast.ChanType); rangeTargetIsChannel { + continue + } + } + } if s.Body != nil { v.handleLoops(s.Body) } @@ -335,40 +332,70 @@ func (v *returnsVisitor) handleLoops(blockStmt *ast.BlockStmt) { switch bodyStmt := stmt.(type) { case *ast.AssignStmt: asgnStmt := bodyStmt - for _, expr := range asgnStmt.Rhs { + for index, expr := range asgnStmt.Rhs { + if index >= len(asgnStmt.Lhs) { + continue + } + + lhsIdent, ok := asgnStmt.Lhs[index].(*ast.Ident) + if !ok { + continue + } + callExpr, ok := expr.(*ast.CallExpr) if !ok { - continue // should this be break? comes back to multi-call support I think + continue } - ident, ok := callExpr.Fun.(*ast.Ident) + + rhsFuncIdent, ok := callExpr.Fun.(*ast.Ident) if !ok { continue } - if ident.Name == "append" { - // see if this append is appending the slice we found - for _, lhsExpr := range asgnStmt.Lhs { - lhsIdent, ok := lhsExpr.(*ast.Ident) + + if rhsFuncIdent.Name != "append" { + continue + } + + // e.g., `x = append(x)` + // Pointless, but pre-allocation will not help. + if len(callExpr.Args) < 2 { + continue + } + + rhsIdent, ok := callExpr.Args[0].(*ast.Ident) + if !ok { + continue + } + + // e.g., `x = append(y, a)` + // This is weird (and maybe a logic error), + // but we cannot recommend pre-allocation. + if lhsIdent.Name != rhsIdent.Name { + continue + } + + // e.g., `x = append(x, y...)` + // we should ignore this. Pre-allocating in this case + // is confusing, and is not possible in general. + if callExpr.Ellipsis.IsValid() { + continue + } + + for _, sliceDecl := range v.sliceDeclarations { + if sliceDecl.name == lhsIdent.Name { + // This is a potential mark, we just need to make sure there are no returns/continues in the + // range loop. + // now we just need to grab whatever we're ranging over + /*sxIdent, ok := s.X.(*ast.Ident) if !ok { continue - } - for _, sliceDecl := range v.sliceDeclarations { - if sliceDecl.name == lhsIdent.Name { - // This is a potential mark, we just need to make sure there are no returns/continues in the - // range loop. - // now we just need to grab whatever we're ranging over - /*sxIdent, ok := s.X.(*ast.Ident) - if !ok { - continue - }*/ + */ - v.preallocHints = append(v.preallocHints, Hint{ - Pos: sliceDecl.genD.Pos(), - DeclaredSliceName: sliceDecl.name, - }) - } - } + v.preallocHints = append(v.preallocHints, Hint{ + Pos: sliceDecl.genD.Pos(), + DeclaredSliceName: sliceDecl.name, + }) } - } } case *ast.IfStmt: @@ -391,7 +418,7 @@ func (v *returnsVisitor) handleLoops(blockStmt *ast.BlockStmt) { } -// Hint stores the information about an occurence of a slice that could be +// Hint stores the information about an occurrence of a slice that could be // preallocated. type Hint struct { Pos token.Pos diff --git a/prealloc_test.go b/prealloc_test.go index ac6f65b..1f22dca 100644 --- a/prealloc_test.go +++ b/prealloc_test.go @@ -1,8 +1,67 @@ package prealloc -import "testing" +import ( + "go/parser" + "go/token" + "testing" +) -func BenchmarkNoPreallocate(b *testing.B) { +func Test_checkForPreallocations(t *testing.T) { + const filename = "testdata/sample.go" + + trueVal := true + got, err := checkForPreallocations([]string{filename}, &trueVal, &trueVal, &trueVal) + if err != nil { + t.Fatal(err) + } + + want := []struct{ + Hint + LineNumber int + }{ + { + LineNumber: 5, + Hint: Hint{DeclaredSliceName: "y"}, + }, + { + LineNumber: 6, + Hint: Hint{DeclaredSliceName: "z"}, + }, + { + LineNumber: 7, + Hint: Hint{DeclaredSliceName: "t"}, + }, + } + + fset := token.NewFileSet() + if _, err := parser.ParseFile(fset, filename, nil, 0); err != nil { + t.Fatalf("could not parse file %q: %s", filename, err) + } + + if len(got) != len(want) { + t.Fatalf("expected %d hints, but got %d: %+v", len(want), len(got), got) + } + + for i := range got { + act, exp := got[i], want[i] + actFilename := fset.Position(act.Pos).Filename + actLineNumber := fset.Position(act.Pos).Line + + if actFilename != filename { + t.Errorf("wrong hints[%d].Filename: %q (expected: %q)", i, actFilename, filename) + } + + if actLineNumber != exp.LineNumber { + t.Errorf("wrong hints[%d].LineNumber: %d (expected: %d)", i, actLineNumber, exp.LineNumber) + } + + if act.DeclaredSliceName != exp.DeclaredSliceName { + t.Errorf("wrong hints[%d].DeclaredSliceName: %q (expected: %q)", i, act.DeclaredSliceName, exp.DeclaredSliceName) + } + } +} + +func BenchmarkSize10NoPreallocate(b *testing.B) { existing := make([]int64, 10, 10) b.ResetTimer() for i := 0; i < b.N; i++ { @@ -14,7 +73,7 @@ func BenchmarkNoPreallocate(b *testing.B) { } } -func BenchmarkPreallocate(b *testing.B) { +func BenchmarkSize10Preallocate(b *testing.B) { existing := make([]int64, 10, 10) b.ResetTimer() for i := 0; i < b.N; i++ { @@ -25,3 +84,47 @@ func BenchmarkPreallocate(b *testing.B) { } } } + +func BenchmarkSize10PreallocateCopy(b *testing.B) { + existing := make([]int64, 10, 10) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Preallocate our initial slice + init := make([]int64, len(existing)) + copy(init, existing) + } +} + +func BenchmarkSize200NoPreallocate(b *testing.B) { + existing := make([]int64, 200, 200) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Don't preallocate our initial slice + var init []int64 + for _, element := range existing { + init = append(init, element) + } + } +} + +func BenchmarkSize200Preallocate(b *testing.B) { + existing := make([]int64, 200, 200) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Preallocate our initial slice + init := make([]int64, 0, len(existing)) + for _, element := range existing { + init = append(init, element) + } + } +} + +func BenchmarkSize200PreallocateCopy(b *testing.B) { + existing := make([]int64, 200, 200) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Preallocate our initial slice + init := make([]int64, len(existing)) + copy(init, existing) + } +} diff --git a/testdata/sample.go b/testdata/sample.go new file mode 100644 index 0000000..8cf72ff --- /dev/null +++ b/testdata/sample.go @@ -0,0 +1,41 @@ +package main + +func main() { + x := make([]rune, len("Hello")) + var y []rune + var z, w, v, u, s []int + var t [][]int + var intChan chan int + + for i, r := range "Hello" { + // x is already pre-allocated + // y is a candidate for pre-allocation + x[i], y = r, append(y, r) + + // w is not a candidate for pre-allocation due to `...` + w = append(w, foo(i)...) + + // v is not a candidate for pre-allocation since this appends to u + v = append(u, i) + + // u is not a candidate for pre-allocation since nothing was actually appended + u = append(u) + + // z is a candidate for pre-allocation + z = append(z, i) + + // t is a candidate for pre-allocation + t = append(t, foo(i)) + } + + for i := range intChan { + // s is not a candidate for pre-allocation since the range target is a channel + s = append(s, i) + } + + _ = v +} + +func foo(n int) []int { + return make([]int, n) +}