checkers: add sortSlice checker (#929)

Find suspicious sort slice calls.

Sorting `slice` argument with a callback that uses
another slice is usually incorrect, as only one
slice will be modified by Slice call inplace.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>

checkers/sortSlice_checker.go

@@ -0,0 +1,135 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/go-critic/go-critic/checkers/internal/lintutil"
+	"github.com/go-lintpack/lintpack"
+	"github.com/go-lintpack/lintpack/astwalk"
+	"github.com/go-toolsmith/astcast"
+	"github.com/go-toolsmith/astequal"
+	"github.com/go-toolsmith/typep"
+	"golang.org/x/tools/go/ast/astutil"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "sortSlice"
+	info.Tags = []string{"diagnostic", "experimental"}
+	info.Summary = "Detects suspicious sort.Slice calls"
+	info.Before = `sort.Slice(xs, func(i, j) bool { return keys[i] < keys[j] })`
+	info.After = `sort.Slice(kv, func(i, j) bool { return kv[i].key < kv[j].key })`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForExpr(&sortSliceChecker{ctx: ctx})
+	})
+}
+
+type sortSliceChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *sortSliceChecker) VisitExpr(expr ast.Expr) {
+	call := astcast.ToCallExpr(expr)
+	if len(call.Args) != 2 {
+		return
+	}
+	switch qualifiedName(call.Fun) {
+	case "sort.Slice", "sort.SliceStable":
+		// OK.
+	default:
+		return
+	}
+
+	slice := c.unwrapSlice(call.Args[0])
+	lessFunc, ok := call.Args[1].(*ast.FuncLit)
+	if !ok {
+		return
+	}
+	if !typep.SideEffectFree(c.ctx.TypesInfo, slice) {
+		return // Don't check unpredictable slice values
+	}
+
+	ivar, jvar := c.paramIdents(lessFunc.Type)
+	if ivar == nil || jvar == nil {
+		return
+	}
+
+	if len(lessFunc.Body.List) != 1 {
+		return
+	}
+	ret, ok := lessFunc.Body.List[0].(*ast.ReturnStmt)
+	if !ok {
+		return
+	}
+	cmp := astcast.ToBinaryExpr(astutil.Unparen(ret.Results[0]))
+	if !typep.SideEffectFree(c.ctx.TypesInfo, cmp) {
+		return
+	}
+	switch cmp.Op {
+	case token.LSS, token.LEQ, token.GTR, token.GEQ:
+		// Both cmp.X and cmp.Y are expected to be some expressions
+		// over the `slice` expression. In the simplest case,
+		// it's a `slice[i] <op> slice[j]`.
+		if !c.containsSlice(cmp.X, slice) && !c.containsSlice(cmp.Y, slice) {
+			c.warnSlice(cmp, slice)
+		}
+
+		// This one is more about the style, but can reveal potential issue
+		// or misprint in sorting condition.
+		// We give a warn if X contains indexing with `i` index and Y
+		// contains indexing with `j`.
+		if c.containsIndex(cmp.X, jvar) && c.containsIndex(cmp.Y, ivar) {
+			c.warnIndex(cmp, ivar, jvar)
+		}
+	}
+}
+
+func (c *sortSliceChecker) paramIdents(e *ast.FuncType) (*ast.Ident, *ast.Ident) {
+	// Covers both `i, j int` and `i int, j int`.
+	idents := make([]*ast.Ident, 0, 2)
+	for _, field := range e.Params.List {
+		idents = append(idents, field.Names...)
+	}
+	if len(idents) == 2 {
+		return idents[0], idents[1]
+	}
+	return nil, nil
+}
+
+func (c *sortSliceChecker) unwrapSlice(e ast.Expr) ast.Expr {
+	switch e := e.(type) {
+	case *ast.ParenExpr:
+		return c.unwrapSlice(e.X)
+	case *ast.SliceExpr:
+		return e.X
+	default:
+		return e
+	}
+}
+
+func (c *sortSliceChecker) containsIndex(e, index ast.Expr) bool {
+	return lintutil.ContainsNode(e, func(n ast.Node) bool {
+		indexing, ok := n.(*ast.IndexExpr)
+		if !ok {
+			return false
+		}
+		return astequal.Expr(indexing.Index, index)
+	})
+}
+
+func (c *sortSliceChecker) containsSlice(e, slice ast.Expr) bool {
+	return lintutil.ContainsNode(e, func(n ast.Node) bool {
+		return astequal.Node(n, slice)
+	})
+}
+
+func (c *sortSliceChecker) warnSlice(cause ast.Node, slice ast.Expr) {
+	c.ctx.Warn(cause, "cmp func must use %s slice in comparison", slice)
+}
+
+func (c *sortSliceChecker) warnIndex(cause ast.Node, ivar, jvar *ast.Ident) {
+	c.ctx.Warn(cause, "unusual order of {%s,%s} params in comparison", ivar, jvar)
+}

checkers/testdata/sortSlice/negative_tests.go

@@ -0,0 +1,114 @@
+package checker_test
+
+import (
+	"sort"
+)
+
+func goodSorting() {
+	{
+		var xs []int
+		var ys []int
+		sort.Slice(xs, func(i, j int) bool {
+			return (xs[i] < xs[j])
+		})
+		sort.Slice(ys, func(i, j int) bool {
+			return (ys[i]) >= (ys[j])
+		})
+	}
+
+	{
+		var xs *[][]int
+		sort.Slice((*xs), func(i, j int) bool {
+			return ((*xs)[i][2] < (*xs)[j][2])
+		})
+	}
+
+	{
+		var xs [][]int
+		sort.Slice(xs[0], func(i, j int) bool {
+			return xs[0][i] <= xs[0][j]
+		})
+	}
+
+	{
+		type elem struct {
+			val string
+			key string
+		}
+		type object struct {
+			elems []elem
+		}
+		var o object
+		sort.Slice(o.elems, func(i, j int) bool {
+			return o.elems[i].key < o.elems[j].key
+		})
+		var iface interface{} = o
+		sort.Slice(iface.(object).elems, func(i, j int) bool {
+			return iface.(object).elems[i].val < iface.(object).elems[j].val
+		})
+	}
+
+	{
+		// It's OK to sort a part of the slice.
+		var xs []int
+		var n int
+		sort.Slice(xs[:n], func(i, j int) bool {
+			return xs[i] < xs[j]
+		})
+	}
+
+	{
+		// OK to use type conversions.
+		var xs [][]byte
+		sort.Slice(xs, func(i, j int) bool {
+			return string(xs[i]) > string(xs[j])
+		})
+	}
+
+	{
+		// OK to use len() func.
+		var xs [][]byte
+		sort.Slice(xs, func(i, j int) bool {
+			return len(xs[i]) <= len(xs[j])
+		})
+	}
+}
+
+var globalSlice = []int{1, 2, 3}
+
+func getSlice() []int { return globalSlice }
+
+func getElem(i int) int { return globalSlice[i] }
+
+func elemKey(i int) string { return "" }
+
+func ignore() {
+	// Skip due to the getSlice() being a function call.
+	sort.Slice(getSlice(), func(i, j int) bool {
+		return globalSlice[i] < globalSlice[j]
+	})
+
+	// This is also suspicious and should probably be reported,
+	// but we don't analyze this case yet.
+	var keys []int
+	sort.Slice(globalSlice, func(i, j int) bool {
+		if globalSlice[i] < globalSlice[j] {
+			return true
+		}
+		return keys[i] < keys[j]
+	})
+
+	sort.Slice(globalSlice, func(i, j int) bool {
+		return getElem(i) > getElem(j)
+	})
+
+	sort.Slice(globalSlice, func(i, j int) bool {
+		return elemKey(globalSlice[i]) > elemKey(globalSlice[j])
+	})
+	sort.Slice(globalSlice, func(i, j int) bool {
+		return elemKey(keys[i]) > elemKey(keys[j])
+	})
+	sort.SliceStable(globalSlice, func(i, j int) bool {
+		return globalSlice[i]+globalSlice[j] > 0
+	})
+}

checkers/testdata/sortSlice/positive_tests.go

@@ -0,0 +1,101 @@
+package checker_test
+
+import (
+	"sort"
+)
+
+func badSorting() {
+	{
+		var xs []int
+		var ys []int
+		sort.Slice(xs, func(i, j int) bool {
+			/*! cmp func must use xs slice in comparison */
+			return (ys[i] < ys[j])
+		})
+		sort.Slice(ys, func(i, j int) bool {
+			/*! cmp func must use ys slice in comparison */
+			return (xs[i]) >= (xs[j])
+		})
+
+		// Same as above, but for SliceStable func.
+		sort.SliceStable(xs, func(i, j int) bool {
+			/*! cmp func must use xs slice in comparison */
+			return (ys[i] < ys[j])
+		})
+		sort.SliceStable(ys, func(i, j int) bool {
+			/*! cmp func must use ys slice in comparison */
+			return (xs[i]) >= (xs[j])
+		})
+	}
+
+	{
+		var xs *[][]int
+		var ys []int
+		sort.Slice((*xs), func(i, j int) bool {
+			/*! cmp func must use *xs slice in comparison */
+			return (ys[i] < ys[j])
+		})
+	}
+
+	{
+		var xs [][]int
+		var ys []int
+		sort.Slice(xs[0], func(i, j int) bool {
+			/*! cmp func must use xs[0] slice in comparison */
+			return ys[i] <= ys[j]
+		})
+	}
+
+	{
+		type elem struct {
+			val string
+			key string
+		}
+		type object struct {
+			elems []elem
+		}
+		var o object
+		var ys []string
+		sort.Slice(o.elems, func(i, j int) bool {
+			/*! cmp func must use o.elems slice in comparison */
+			return ys[i] < ys[j]
+		})
+		var iface interface{} = o
+		sort.Slice(iface.(object).elems, func(i, j int) bool {
+			/*! cmp func must use iface.(object).elems slice in comparison */
+			return ys[i] < ys[j]
+		})
+	}
+}
+
+func swappedIndex() {
+	{
+		{
+			var xs []int
+			sort.Slice(xs, func(i, j int) bool {
+				/*! unusual order of {i,j} params in comparison */
+				return (xs[j]) >= (xs[i])
+			})
+		}
+	}
+
+	{
+		type elem struct {
+			val string
+			key string
+		}
+		type object struct {
+			elems []elem
+		}
+		var o object
+		sort.Slice(o.elems, func(i int, j int) bool {
+			/*! unusual order of {i,j} params in comparison */
+			return o.elems[j].key < o.elems[i].key
+		})
+		var iface interface{} = o
+		sort.Slice(iface.(object).elems, func(a, b int) bool {
+			/*! unusual order of {a,b} params in comparison */
+			return iface.(object).elems[b].val < iface.(object).elems[a].val
+		})
+	}
+}

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/go-toolsmith/astfmt v1.0.0
 	github.com/go-toolsmith/astp v1.0.0
 	github.com/go-toolsmith/strparse v1.0.0
-	github.com/go-toolsmith/typep v1.0.0
+	github.com/go-toolsmith/typep v1.0.2
 	github.com/mattn/goveralls v0.0.2
 	github.com/pborman/uuid v1.2.0 // indirect
 	github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c

go.sum

@@ -22,6 +22,8 @@ github.com/go-toolsmith/strparse v1.0.0 h1:Vcw78DnpCAKlM20kSbAyO4mPfJn/lyYA4BJUD
 github.com/go-toolsmith/strparse v1.0.0/go.mod h1:YI2nUKP9YGZnL/L1/DLFBfixrcjslWct4wyljWhSRy8=
 github.com/go-toolsmith/typep v1.0.0 h1:zKymWyA1TRYvqYrYDrfEMZULyrhcnGY3x7LDKU2XQaA=
 github.com/go-toolsmith/typep v1.0.0/go.mod h1:JSQCQMUPdRlMZFswiq3TGpNp1GMktqkR2Ns5AIQkATU=
+github.com/go-toolsmith/typep v1.0.2 h1:8xdsa1+FSIH/RhEkgnD1j2CJOy5mNllW1Q9tRiYwvlk=
+github.com/go-toolsmith/typep v1.0.2/go.mod h1:JSQCQMUPdRlMZFswiq3TGpNp1GMktqkR2Ns5AIQkATU=
 github.com/google/go-cmp v0.2.0 h1:+dTQ8DZQJz0Mb/HjFlkptS1FeQ4cWSnN941F8aEG4SQ=
 github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
 github.com/google/uuid v1.0.0 h1:b4Gk+7WdP/d3HZH8EJsZpvV7EtDOgaZLtnaNGIu1adA=