checkers: add evalOrder checker (#829)

Checks for return statements that may have unwanted
dependency on the evaluation order.

See:
	golang/go#27098
	golang/go#23188

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

checkers/evalOrder_checker.go

@@ -0,0 +1,87 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"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"
+)
+
+func init() {
+	var info lintpack.CheckerInfo
+	info.Name = "evalOrder"
+	info.Tags = []string{"diagnostic", "experimental"}
+	info.Summary = "Detects unwanted dependencies on the evaluation order"
+	info.Before = `return x, f(&x)`
+	info.After = `
+err := f(&x)
+return x, err
+`
+
+	collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
+		return astwalk.WalkerForStmt(&evalOrderChecker{ctx: ctx})
+	})
+}
+
+type evalOrderChecker struct {
+	astwalk.WalkHandler
+	ctx *lintpack.CheckerContext
+}
+
+func (c *evalOrderChecker) VisitStmt(stmt ast.Stmt) {
+	ret := astcast.ToReturnStmt(stmt)
+	if len(ret.Results) < 2 {
+		return
+	}
+
+	// TODO(quasilyte): handle selector expressions like o.val in addition
+	// to bare identifiers.
+	addrTake := &ast.UnaryExpr{Op: token.AND}
+	for _, res := range ret.Results {
+		id, ok := res.(*ast.Ident)
+		if !ok {
+			continue
+		}
+		addrTake.X = id // addrTake is &id now
+		for _, res := range ret.Results {
+			call, ok := res.(*ast.CallExpr)
+			if !ok {
+				continue
+			}
+
+			// 1. Check if there is a call in form of id.method() where
+			// method takes id by a pointer.
+			if sel, ok := call.Fun.(*ast.SelectorExpr); ok {
+				if astequal.Node(sel.X, id) && c.hasPtrRecv(sel.Sel) {
+					c.warn(call)
+				}
+			}
+
+			// 2. Check that there is no call that uses &id as an argument.
+			dependency := lintutil.ContainsNode(call, func(n ast.Node) bool {
+				return astequal.Node(addrTake, n)
+			})
+			if dependency {
+				c.warn(call)
+			}
+		}
+	}
+}
+
+func (c *evalOrderChecker) hasPtrRecv(fn *ast.Ident) bool {
+	sig, ok := c.ctx.TypesInfo.TypeOf(fn).(*types.Signature)
+	if !ok {
+		return false
+	}
+	return typep.IsPointer(sig.Recv().Type())
+}
+
+func (c *evalOrderChecker) warn(call *ast.CallExpr) {
+	c.ctx.Warn(call, "may want to evaluate %s before the return statement", call)
+}

checkers/testdata/evalOrder/negative_tests.go

@@ -0,0 +1,34 @@
+package checker_test
+
+func noReturnDependency() {
+	var x int
+	var y int
+
+	_ = func() (int, int) {
+		v := mutateArg(&x)
+		return x, v
+	}
+
+	_ = func() (int, int, int, int) {
+		yv := mutateArg(&y)
+		xv := mutateArg(&x)
+		return yv, xv, x, y
+	}
+
+	_ = func() (int, int, int) {
+		v1 := mutateArg(&x)
+		v2 := mutateArg(&x)
+		return v1, v2, x
+	}
+
+	var o object
+
+	_ = func() (object, int) {
+		v := o.mutate()
+		return o, v
+	}
+
+	_ = func() (object, int) {
+		return o, o.cantMutate()
+	}
+}

checkers/testdata/evalOrder/positive_tests.go

@@ -0,0 +1,51 @@
+package checker_test
+
+func mutateArg(x *int) int {
+	*x += 10
+	return *x
+}
+
+func identity(x int) int {
+	return x
+}
+
+type object struct {
+	val int
+}
+
+func (o *object) mutate() int {
+	o.val++
+	return o.val
+}
+
+func (o object) cantMutate() int {
+	return o.val
+}
+
+func returnDepencency() {
+	var x int
+	var y int
+
+	_ = func() (int, int) {
+		/*! may want to evaluate mutateArg(&x) before the return statement */
+		return x, mutateArg(&x)
+	}
+
+	_ = func() (int, int, int, int) {
+		/*! may want to evaluate mutateArg(&x) before the return statement */
+		/*! may want to evaluate mutateArg(&y) before the return statement */
+		return mutateArg(&y), mutateArg(&x), x, y
+	}
+
+	_ = func() (int, int, int) {
+		/*! may want to evaluate mutateArg(&x) before the return statement */
+		return identity(x), mutateArg(&x), x
+	}
+
+	var o object
+
+	_ = func() (object, int) {
+		/*! may want to evaluate o.mutate() before the return statement */
+		return o, o.mutate()
+	}
+}