checkers: add argOrder checker (go-critic#790)

Signed-off-by: Iskander Sharipov <[email protected]>

checkers/argOrder_checker.go

@@ -0,0 +1,98 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/types"
+
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+	"github.com/go-toolsmith/astcast"
+	"github.com/go-toolsmith/astcopy"
+	"github.com/go-toolsmith/astp"
+	"github.com/go-toolsmith/typep"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "argOrder"
+	info.Tags = []string{"diagnostic", "experimental"}
+	info.Summary = "Detects suspicious arguments order"
+	info.Before = `strings.HasPrefix("#", userpass)`
+	info.After = `strings.HasPrefix(userpass, "#")`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForExpr(&argOrderChecker{ctx: ctx})
+	})
+}
+
+type argOrderChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *argOrderChecker) VisitExpr(expr ast.Expr) {
+	call := astcast.ToCallExpr(expr)
+
+	// For now only handle functions of 2 args.
+	// TODO(Quasilyte): generalize the algorithm and add more patterns.
+	if len(call.Args) != 2 {
+		return
+	}
+
+	calledExpr := astcast.ToSelectorExpr(call.Fun)
+	obj, ok := c.ctx.TypesInfo.ObjectOf(astcast.ToIdent(calledExpr.X)).(*types.PkgName)
+	if !ok || !isStdlibPkg(obj.Imported()) {
+		return
+	}
+
+	x := call.Args[0]
+	y := call.Args[1]
+	switch calledExpr.Sel.Name {
+	case "HasPrefix", "HasSuffix", "Contains", "TrimPrefix", "TrimSuffix":
+		if obj.Name() != "bytes" && obj.Name() != "strings" {
+			return
+		}
+		if c.isConstLiteral(x) && !c.isConstLiteral(y) {
+			c.warn(call)
+		}
+	}
+}
+
+func (c *argOrderChecker) isConstLiteral(x ast.Expr) bool {
+	if c.ctx.TypesInfo.Types[x].Value != nil {
+		return true
+	}
+
+	// Also permit byte slices.
+	switch x := x.(type) {
+	case *ast.CallExpr:
+		// Handle `[]byte("abc")` as well.
+		if len(x.Args) != 1 || !astp.IsBasicLit(x.Args[0]) {
+			return false
+		}
+		typ, ok := c.ctx.TypesInfo.TypeOf(x.Fun).(*types.Slice)
+		return ok && typep.HasUint8Kind(typ.Elem())
+
+	case *ast.CompositeLit:
+		// Check if it's a const byte slice.
+		typ, ok := c.ctx.TypesInfo.TypeOf(x).(*types.Slice)
+		if !ok || !typep.HasUint8Kind(typ.Elem()) {
+			return false
+		}
+		for _, elt := range x.Elts {
+			if !astp.IsBasicLit(elt) {
+				return false
+			}
+		}
+		return true
+
+	default:
+		return false
+	}
+}
+
+func (c *argOrderChecker) warn(call *ast.CallExpr) {
+	fixed := astcopy.CallExpr(call)
+	fixed.Args[0], fixed.Args[1] = fixed.Args[1], fixed.Args[0]
+	c.ctx.Warn(call, "probably meant `%s`", fixed)
+}

checkers/testdata/_integration/check_main_only/linttest.golden

@@ -1,6 +1,7 @@
 exit status 1
 ./main.go:14:7: appendAssign: append result not assigned to the same slice
 ./main.go:19:2: appendCombine: can combine chain of 2 appends into one
+./main.go:268:6: argOrder: probably meant `strings.HasPrefix(s, "$")`
 ./main.go:24:2: assignOp: replace `x = x + 2` with `x += 2`
 ./main.go:246:6: badCond: `x < 100 && x > 200` condition is always false
 ./main.go:28:9: boolExprSimplify: can simplify `!(x == y+1)` to `x != y+1`

checkers/testdata/_integration/check_main_only/main.go

@@ -264,5 +264,9 @@ func typeAssertChain() {
 	}
 }
 
+func argOrder(s string) {
+	_ = strings.HasPrefix("$", s)
+}
+
 func main() {
 }

checkers/testdata/argOrder/negative_tests.go

@@ -0,0 +1,23 @@
+package checker_test
+
+import (
+	"bytes"
+	"strings"
+)
+
+func nonConstArgs(s1, s2 string, b1, b2 []byte) {
+	_ = strings.HasPrefix(s1, s2)
+	_ = bytes.HasPrefix(b1, b2)
+
+	x := byte('x')
+	_ = bytes.HasPrefix([]byte{x}, b1)
+	_ = bytes.HasPrefix([]byte(s1), b1)
+}
+
+func properArgsOrder(s string, b []byte) {
+	_ = strings.HasPrefix(s, "http://")
+	_ = bytes.HasPrefix(b, []byte("http://"))
+	_ = bytes.HasPrefix(b, []byte{'h', 't', 't', 'p', ':', '/', '/'})
+	_ = strings.Contains(s, ":")
+	_ = bytes.Contains(b, []byte(":"))
+}

checkers/testdata/argOrder/positive_tests.go

@@ -0,0 +1,48 @@
+package checker_test
+
+import (
+	"bytes"
+	"strings"
+)
+
+func badArgOrder(s string, b []byte) {
+	/*! probably meant `strings.HasPrefix(s, "http://")` */
+	_ = strings.HasPrefix("http://", s)
+
+	/*! probably meant `bytes.HasPrefix(b, []byte("http://"))` */
+	_ = bytes.HasPrefix([]byte("http://"), b)
+	/*! probably meant `bytes.HasPrefix(b, []byte{'h', 't', 't', 'p', ':', '/', '/'})` */
+	_ = bytes.HasPrefix([]byte{'h', 't', 't', 'p', ':', '/', '/'}, b)
+
+	/*! probably meant `strings.Contains(s, ":")` */
+	_ = strings.Contains(":", s)
+	/*! probably meant `bytes.Contains(b, []byte(":"))` */
+	_ = bytes.Contains([]byte(":"), b)
+
+	/*! probably meant `strings.TrimPrefix(s, ":")` */
+	_ = strings.TrimPrefix(":", s)
+	/*! probably meant `bytes.TrimPrefix(b, []byte(":"))` */
+	_ = bytes.TrimPrefix([]byte(":"), b)
+
+	/*! probably meant `strings.TrimSuffix(s, ":")` */
+	_ = strings.TrimSuffix(":", s)
+	/*! probably meant `bytes.TrimSuffix(b, []byte(":"))` */
+	_ = bytes.TrimSuffix([]byte(":"), b)
+}
+
+func argubleCases(s string) {
+	// It's possible to use strings.Contains as a "check element presents in a set".
+	// But that usage is somewhat rare and can be implemented via sorted []string
+	// search or a map[string]bool/map[string]struct{}.
+
+	/*! probably meant `strings.Contains(s, "uint uint8 uint16 uint32")` */
+	_ = strings.Contains("uint uint8 uint16 uint32", s)
+
+	// Code below removes "optional " prefix if s="optional ".
+	// But this is not the most clear way to do it.
+	// We accept this false positive as it can be fixed
+	// by assigning a string literal to a variable, for example.
+
+	/*! probably meant `strings.TrimPrefix(s, "optional foo bar")` */
+	_ = strings.TrimPrefix("optional foo bar", s)
+}