go/analysis/passes/defermistake: add analyser for defer mistake

This is adding an analysis pass to catch defer statements where people
intend to invoke a defer arguments when the defer is ran; not when it is
first invoked.

In order to acheive this, the current analyasis implementation first uses
the inspect.Preorder tool to look for defer nodes. It then walks the
defer node expression tree. This solution means that we don't catch
function literals, and maybe it's slightly unoptimized because it
doesn't use the Inspect fast node filtering once we find the defer
nodes.

Updates golang/go#60048.

Change-Id: I50ec60c7fc4a5ced858f42cb8db8e9ea37a7038f

go/analysis/passes/defermistake/defermistake.go

@@ -0,0 +1,87 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package defermistake defines an Analyzer that reports common errors using defer.
+// Specifically, this package is concerned with reporting errors when a defer statement
+// is made with a function expression that is evaluated before the defer statements runs.
+//
+// For example, doing: defer observe(time.Since(time.Now())) will result in the observe
+// function receiving a time.Duration that is near zero. In order to have the intended
+// output, you can write: defer func() { observe(time.Since(now)) }()
+//
+// Currently the only function that is reported by this analysis pass is time.Since().
+package defermistake
+
+import (
+	"go/ast"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/types/typeutil"
+)
+
+const Doc = `report common mistakes evaluating functions at defer invocation
+
+The defermistake analysis reports if a function is evaluated when the defer is invoked,
+but it most likely that it is intended to be evaluated when the defer is executed.
+
+time.Since is currently the only function that is checked in this way.`
+
+// Analyzer is the defermistake analyzer.
+var Analyzer = &analysis.Analyzer{
+	Name:     "defermistake",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Doc:      Doc,
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	// checkDeferCall walks an expression tree and reports if any expressions are time.Since.
+	// FIXME: it does not walk into an ast.CallExpr, because FuncLit.Body is a BlockStmt.
+	// This means we do not catch a case where a function is invoked in a function literal:
+	//    defer x(func() time.Duration {time.Since(x)}())
+	checkDeferCall := func(node ast.Node) bool {
+		switch v := node.(type) {
+		case *ast.CallExpr:
+			// Useful print if you need to debug:
+			//   pos := pass.Fset.Position(call.Pos())
+			//   fmt.Printf("%v:%v\t| =>> fun=%T \n", pos.Line, pos.Column, call.Fun)
+			fn, ok := typeutil.Callee(pass.TypesInfo, v).(*types.Func)
+			if ok && isTimeSince(fn) {
+				pass.Reportf(v.Pos(), "defer func should not evaluate time.Since")
+				return true
+			}
+
+			return true
+		case ast.Expr:
+			return true
+		}
+		return false
+	}
+
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	// FIXME: ast.Inspect is called on the defer call. This could instead
+	// be constructed instead by creating a different function on inspect
+	// that allowed you to traverse nodes by matching a pattern. I don't know
+	// if this preferred to this more simplistic implementation.
+	nodeFilter := []ast.Node{
+		(*ast.DeferStmt)(nil),
+	}
+	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		d := n.(*ast.DeferStmt)
+		ast.Inspect(d.Call, checkDeferCall)
+	})
+
+	return nil, nil
+}
+
+func isTimeSince(f *types.Func) bool {
+	if f.Name() == "Since" && f.Pkg().Path() == "time" {
+		return true
+	}
+	return false
+}

go/analysis/passes/defermistake/defermistake_test.go

@@ -0,0 +1,17 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package defermistake_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/defermistake"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, defermistake.Analyzer, "a")
+}

go/analysis/passes/defermistake/testdata/src/a/a.go

@@ -0,0 +1,49 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+import (
+	"fmt"
+	"time"
+)
+
+func x(time.Duration) {}
+func x2(float64)      {}
+
+func _() { // code that doesnt trigger a report.
+	// The following are OK because func is not evaluated in defer invocation.
+	now := time.Now()
+	defer func() {
+		fmt.Println(time.Since(now)) // OK because time.Since is not evaluated in defer
+	}()
+	evalBefore := time.Since(now)
+	defer fmt.Println(evalBefore)
+	do := func(f func()) {}
+	defer do(func() { time.Since(now) })
+
+	// FIXME: The following are okay even though technically time.Since is evaluated here.
+	// We don't walk into literal functions.
+	defer x((func() time.Duration { return time.Since(now) })())
+}
+
+type y struct{}
+
+func (y) A(float64)        {}
+func (*y) B(float64)       {}
+func (y) C(time.Duration)  {}
+func (*y) D(time.Duration) {}
+
+func _() { // code that triggers a report.
+	now := time.Now()
+	defer time.Since(now)                     // want "defer func should not evaluate time.Since"
+	defer fmt.Println(time.Since(now))        // want "defer func should not evaluate time.Since"
+	defer fmt.Println(time.Since(time.Now())) // want "defer func should not evaluate time.Since"
+	defer x(time.Since(now))                  // want "defer func should not evaluate time.Since"
+	defer x2(time.Since(now).Seconds())       // want "defer func should not evaluate time.Since"
+	defer y{}.A(time.Since(now).Seconds())    // want "defer func should not evaluate time.Since"
+	defer (&y{}).B(time.Since(now).Seconds()) // want "defer func should not evaluate time.Since"
+	defer y{}.C(time.Since(now))              // want "defer func should not evaluate time.Since"
+	defer (&y{}).D(time.Since(now))           // want "defer func should not evaluate time.Since"
+}