Merge upstream changes

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

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.

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

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)
+	}
+}

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)
+}