From c6cfb51e64c922d08e805d8b9fceabbec0268b85 Mon Sep 17 00:00:00 2001 From: Aaron Delaney Date: Thu, 1 Jun 2023 15:13:23 +0100 Subject: [PATCH] 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 --- .../passes/defermistake/defermistake.go | 87 +++++++++++++++++++ .../passes/defermistake/defermistake_test.go | 17 ++++ .../passes/defermistake/testdata/src/a/a.go | 49 +++++++++++ 3 files changed, 153 insertions(+) create mode 100644 go/analysis/passes/defermistake/defermistake.go create mode 100644 go/analysis/passes/defermistake/defermistake_test.go create mode 100644 go/analysis/passes/defermistake/testdata/src/a/a.go diff --git a/go/analysis/passes/defermistake/defermistake.go b/go/analysis/passes/defermistake/defermistake.go new file mode 100644 index 00000000000..cbb93fa4b2f --- /dev/null +++ b/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 +} diff --git a/go/analysis/passes/defermistake/defermistake_test.go b/go/analysis/passes/defermistake/defermistake_test.go new file mode 100644 index 00000000000..b4c8d2ba400 --- /dev/null +++ b/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") +} diff --git a/go/analysis/passes/defermistake/testdata/src/a/a.go b/go/analysis/passes/defermistake/testdata/src/a/a.go new file mode 100644 index 00000000000..513e5e0b900 --- /dev/null +++ b/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" +}