diff --git a/checkers/argOrder_checker.go b/checkers/argOrder_checker.go new file mode 100644 index 000000000..9e6a4fa70 --- /dev/null +++ b/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) +} diff --git a/checkers/testdata/_integration/check_main_only/linttest.golden b/checkers/testdata/_integration/check_main_only/linttest.golden index 214b733c9..1090f94c9 100644 --- a/checkers/testdata/_integration/check_main_only/linttest.golden +++ b/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` diff --git a/checkers/testdata/_integration/check_main_only/main.go b/checkers/testdata/_integration/check_main_only/main.go index 5f4289f92..2427e3f83 100644 --- a/checkers/testdata/_integration/check_main_only/main.go +++ b/checkers/testdata/_integration/check_main_only/main.go @@ -264,5 +264,9 @@ func typeAssertChain() { } } +func argOrder(s string) { + _ = strings.HasPrefix("$", s) +} + func main() { } diff --git a/checkers/testdata/argOrder/negative_tests.go b/checkers/testdata/argOrder/negative_tests.go new file mode 100644 index 000000000..9eb55caff --- /dev/null +++ b/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(":")) +} diff --git a/checkers/testdata/argOrder/positive_tests.go b/checkers/testdata/argOrder/positive_tests.go new file mode 100644 index 000000000..1f47c6543 --- /dev/null +++ b/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) +}