lint: add indexAlloc checker

Performance check.

Suggests to replace strings.Index with bytes.Index
where it can make code more efficient.

See golang/go#25864

Author: quasilyte

docs/overview.md

@@ -180,6 +180,10 @@ This page describes checks supported by [go-critic](https://github.com/go-critic
         <td><a href="#importShadow-ref">importShadow</a></td>
         <td>Detects when imported package names shadowed in assignments</td>
       </tr>
+      <tr>
+        <td><a href="#indexAlloc-ref">indexAlloc</a></td>
+        <td>Detects strings.Index calls that may cause unwanted allocs</td>
+      </tr>
       <tr>
         <td><a href="#indexOnlyLoop-ref">indexOnlyLoop</a></td>
         <td>Detects for loops that can benefit from rewrite to range loop</td>
@@ -910,6 +914,24 @@ func myFunc(filename string) {
 
 
 
+<a name="indexAlloc-ref"></a>
+## indexAlloc
+Detects strings.Index calls that may cause unwanted allocs.
+
+
+
+**Before:**
+```go
+strings.Index(string(x), y)
+```
+
+**After:**
+```go
+bytes.Index(x, []byte(y))
+```
+
+
+
 <a name="indexOnlyLoop-ref"></a>
 ## indexOnlyLoop
 Detects for loops that can benefit from rewrite to range loop.

lint/assignOp_checker.go

@@ -28,7 +28,7 @@ func (c *assignOpChecker) VisitStmt(stmt ast.Stmt) {
 		assign.Tok == token.ASSIGN &&
 		len(assign.Lhs) == 1 &&
 		len(assign.Rhs) == 1 &&
-		isSafeExpr(assign.Lhs[0])
+		isSafeExpr(c.ctx.typesInfo, assign.Lhs[0])
 	if !cond {
 		return
 	}

lint/boolExprSimplify_checker.go

@@ -113,7 +113,7 @@ func (c *boolExprSimplifyChecker) combineChecks(cur *astutil.Cursor) bool {
 	if !astequal.Expr(lhs.X, rhs.X) || !astequal.Expr(lhs.Y, rhs.Y) {
 		return false
 	}
-	if !isSafeExpr(lhs.X) || !isSafeExpr(lhs.Y) {
+	if !isSafeExpr(c.ctx.typesInfo, lhs.X) || !isSafeExpr(c.ctx.typesInfo, lhs.Y) {
 		return false
 	}
 

lint/dupSubExpr_checker.go

@@ -80,7 +80,7 @@ func (c *dupSubExprChecker) checkBinaryExpr(expr *ast.BinaryExpr) {
 	if c.resultIsFloat(expr.X) && c.floatOpsSet[expr.Op] {
 		return
 	}
-	if isSafeExpr(expr) && c.opSet[expr.Op] && astequal.Expr(expr.X, expr.Y) {
+	if isSafeExpr(c.ctx.typesInfo, expr) && c.opSet[expr.Op] && astequal.Expr(expr.X, expr.Y) {
 		c.warn(expr)
 	}
 }

lint/indexAlloc_checker.go

@@ -0,0 +1,43 @@
+package lint
+
+import (
+	"go/ast"
+
+	"github.com/go-critic/go-critic/lint/internal/lintutil"
+)
+
+func init() {
+	addChecker(&indexAllocChecker{}, attrExperimental, attrPerformance)
+}
+
+type indexAllocChecker struct {
+	checkerBase
+}
+
+func (c *indexAllocChecker) InitDocumentation(d *Documentation) {
+	d.Summary = "Detects strings.Index calls that may cause unwanted allocs"
+	d.Before = `strings.Index(string(x), y)`
+	d.After = `bytes.Index(x, []byte(y))`
+}
+
+func (c *indexAllocChecker) VisitExpr(e ast.Expr) {
+	call := lintutil.AsCallExpr(e)
+	if qualifiedName(call.Fun) != "strings.Index" {
+		return
+	}
+	stringConv := lintutil.AsCallExpr(call.Args[0])
+	if qualifiedName(stringConv.Fun) != "string" {
+		return
+	}
+	x := stringConv.Args[0]
+	y := call.Args[1]
+	if !isSafeExpr(c.ctx.typesInfo, x) || !isSafeExpr(c.ctx.typesInfo, y) {
+		return
+	}
+	c.warn(e, x, y)
+}
+
+func (c *indexAllocChecker) warn(cause ast.Node, x, y ast.Expr) {
+	c.ctx.Warn(cause, "consider replacing %s with bytes.Index(%s, []byte(%s))",
+		cause, x, y)
+}

lint/nilValReturn_checker.go

@@ -44,7 +44,7 @@ func (c *nilValReturnChecker) VisitStmt(stmt ast.Stmt) {
 	expr, ok := ifStmt.Cond.(*ast.BinaryExpr)
 	cond := ok &&
 		expr.Op == token.EQL &&
-		isSafeExpr(expr.X) &&
+		isSafeExpr(c.ctx.typesInfo, expr.X) &&
 		qualifiedName(expr.Y) == "nil" &&
 		astequal.Expr(expr.X, ret.Results[0])
 	if cond {

lint/testdata/indexAlloc/negative_tests.go

@@ -0,0 +1,23 @@
+package checker_tests
+
+import "bytes"
+
+func fixedCode(x []byte, y string) {
+	_ = bytes.Index(x, []byte(y))
+	_ = bytes.Index([]byte("12"), []byte(y))
+	_ = bytes.Index([]byte{'1', '2'}, []byte(y))
+	_ = bytes.Index(x, []byte("a"+y))
+}
+
+func getBytes() []byte  { return nil }
+func getString() string { return "" }
+
+func unsafeArgs(x []byte, y string) {
+	_ = bytes.Index(getBytes(), []byte(y))
+	_ = bytes.Index(getBytes(), []byte("a"+y))
+
+	_ = bytes.Index(x, []byte(getString()))
+	_ = bytes.Index([]byte("12"), []byte(getString()))
+	_ = bytes.Index([]byte{'1', '2'}, []byte(getString()))
+	_ = bytes.Index(x, []byte("a"+getString()))
+}

lint/testdata/indexAlloc/positive_tests.go

@@ -0,0 +1,17 @@
+package checker_tests
+
+import "strings"
+
+func positiveTests(x []byte, y string) {
+	/// consider replacing strings.Index(string(x), y) with bytes.Index(x, []byte(y))
+	_ = strings.Index(string(x), y)
+
+	/// consider replacing strings.Index(string([]byte("12")), y) with bytes.Index([]byte("12"), []byte(y))
+	_ = strings.Index(string([]byte("12")), y)
+
+	/// consider replacing strings.Index(string([]byte{'1', '2'}), y) with bytes.Index([]byte{'1', '2'}, []byte(y))
+	_ = strings.Index(string([]byte{'1', '2'}), y)
+
+	/// consider replacing strings.Index(string(x), "a"+y) with bytes.Index(x, []byte("a" + y))
+	_ = strings.Index(string(x), "a"+y)
+}

lint/util.go

@@ -6,6 +6,7 @@ import (
 	"go/types"
 	"strings"
 
+	"github.com/go-critic/go-critic/lint/internal/lintutil"
 	"golang.org/x/tools/go/ast/astutil"
 )
 
@@ -101,7 +102,10 @@ func typeIsPointer(typ types.Type) bool {
 // no significant side-effects. As opposed to strictly safe expressions,
 // soft safe expressions permit some forms of side-effects, like
 // panic possibility during indexing or nil pointer dereference.
-func isSafeExpr(expr ast.Expr) bool {
+//
+// Uses types info to determine type conversion expressions that
+// are the only permitted kinds of call expressions.
+func isSafeExpr(info *types.Info, expr ast.Expr) bool {
 	// This list switch is not comprehensive and uses
 	// whitelist to be on the conservative side.
 	// Can be extended as needed.
@@ -111,19 +115,29 @@ func isSafeExpr(expr ast.Expr) bool {
 	// may cause panics.
 	switch expr := expr.(type) {
 	case *ast.StarExpr:
-		return isSafeExpr(expr.X)
+		return isSafeExpr(info, expr.X)
 	case *ast.BinaryExpr:
-		return isSafeExpr(expr.X) && isSafeExpr(expr.Y)
+		return isSafeExpr(info, expr.X) && isSafeExpr(info, expr.Y)
 	case *ast.UnaryExpr:
-		return expr.Op != token.ARROW && isSafeExpr(expr.X)
+		return expr.Op != token.ARROW && isSafeExpr(info, expr.X)
 	case *ast.BasicLit, *ast.Ident:
 		return true
 	case *ast.IndexExpr:
-		return isSafeExpr(expr.X) && isSafeExpr(expr.Index)
+		return isSafeExpr(info, expr.X) && isSafeExpr(info, expr.Index)
 	case *ast.SelectorExpr:
-		return isSafeExpr(expr.X)
+		return isSafeExpr(info, expr.X)
 	case *ast.ParenExpr:
-		return isSafeExpr(expr.X)
+		return isSafeExpr(info, expr.X)
+	case *ast.CompositeLit:
+		for _, x := range expr.Elts {
+			if !isSafeExpr(info, x) {
+				return false
+			}
+		}
+		return true
+	case *ast.CallExpr:
+		return lintutil.IsTypeExpr(info, expr.Fun)
+
 	default:
 		return false
 	}