From 85be8882c905a9b24abcec8363e48a14ad5cd4e2 Mon Sep 17 00:00:00 2001 From: Aaron Delaney Date: Thu, 1 Jun 2023 15:13:23 +0100 Subject: [PATCH] go/analysis/passes/defers: 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 achieve 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 Reviewed-on: https://go-review.googlesource.com/c/tools/+/499875 TryBot-Bypass: Alan Donovan Reviewed-by: Alan Donovan Reviewed-by: Robert Findley Auto-Submit: Alan Donovan gopls-CI: kokoro Run-TryBot: Alan Donovan --- go/analysis/passes/defers/cmd/defers/main.go | 13 ++++ go/analysis/passes/defers/defer.go | 60 +++++++++++++++++++ go/analysis/passes/defers/defer_test.go | 17 ++++++ go/analysis/passes/defers/doc.go | 25 ++++++++ go/analysis/passes/defers/testdata/src/a/a.go | 59 ++++++++++++++++++ 5 files changed, 174 insertions(+) create mode 100644 go/analysis/passes/defers/cmd/defers/main.go create mode 100644 go/analysis/passes/defers/defer.go create mode 100644 go/analysis/passes/defers/defer_test.go create mode 100644 go/analysis/passes/defers/doc.go create mode 100644 go/analysis/passes/defers/testdata/src/a/a.go diff --git a/go/analysis/passes/defers/cmd/defers/main.go b/go/analysis/passes/defers/cmd/defers/main.go new file mode 100644 index 00000000000..b3dc8b94eca --- /dev/null +++ b/go/analysis/passes/defers/cmd/defers/main.go @@ -0,0 +1,13 @@ +// 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. + +// The defers command runs the defers analyzer. +package main + +import ( + "golang.org/x/tools/go/analysis/passes/defers" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { singlechecker.Main(defers.Analyzer) } diff --git a/go/analysis/passes/defers/defer.go b/go/analysis/passes/defers/defer.go new file mode 100644 index 00000000000..19474bcc4e8 --- /dev/null +++ b/go/analysis/passes/defers/defer.go @@ -0,0 +1,60 @@ +// 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 defers + +import ( + _ "embed" + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" +) + +//go:embed doc.go +var doc string + +// Analyzer is the defer analyzer. +var Analyzer = &analysis.Analyzer{ + Name: "defer", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Doc: analysisutil.MustExtractDoc(doc, "defer"), + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + if !analysisutil.Imports(pass.Pkg, "time") { + return nil, nil + } + + checkDeferCall := func(node ast.Node) bool { + switch v := node.(type) { + case *ast.CallExpr: + fn, ok := typeutil.Callee(pass.TypesInfo, v).(*types.Func) + if ok && fn.Name() == "Since" && fn.Pkg().Path() == "time" { + pass.Reportf(v.Pos(), "call to time.Since is not deferred") + } + case *ast.FuncLit: + return false // prune + } + return true + } + + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + 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 +} diff --git a/go/analysis/passes/defers/defer_test.go b/go/analysis/passes/defers/defer_test.go new file mode 100644 index 00000000000..57881f022d4 --- /dev/null +++ b/go/analysis/passes/defers/defer_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 defers_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/defers" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, defers.Analyzer, "a") +} diff --git a/go/analysis/passes/defers/doc.go b/go/analysis/passes/defers/doc.go new file mode 100644 index 00000000000..ec9f7664062 --- /dev/null +++ b/go/analysis/passes/defers/doc.go @@ -0,0 +1,25 @@ +// 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 defers defines an Analyzer that checks for common mistakes in defer +// statements. +// +// # Analyzer defer +// +// defer: report common mistakes in defer statements +// +// The defer analyzer reports a diagnostic when a defer statement would +// result in a non-deferred call to time.Since, as experience has shown +// that this is nearly always a mistake. +// +// For example: +// +// start := time.Now() +// ... +// defer recordLatency(time.Since(start)) // error: call to time.Since is not deferred +// +// The correct code is: +// +// defer func() { recordLatency(time.Since(start)) }()` +package defers diff --git a/go/analysis/passes/defers/testdata/src/a/a.go b/go/analysis/passes/defers/testdata/src/a/a.go new file mode 100644 index 00000000000..e8bc8cde3ba --- /dev/null +++ b/go/analysis/passes/defers/testdata/src/a/a.go @@ -0,0 +1,59 @@ +// 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 Since() (t time.Duration) { + return +} + +func x(time.Duration) {} +func x2(float64) {} + +func good() { + // 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) }) + defer fmt.Println(Since()) // OK because Since function is not in module time + +} + +type y struct{} + +func (y) A(float64) {} +func (*y) B(float64) {} +func (y) C(time.Duration) {} +func (*y) D(time.Duration) {} + +func bad() { + var zero time.Time + now := time.Now() + defer time.Since(zero) // want "call to time.Since is not deferred" + defer time.Since(now) // want "call to time.Since is not deferred" + defer fmt.Println(time.Since(now)) // want "call to time.Since is not deferred" + defer fmt.Println(time.Since(time.Now())) // want "call to time.Since is not deferred" + defer x(time.Since(now)) // want "call to time.Since is not deferred" + defer x2(time.Since(now).Seconds()) // want "call to time.Since is not deferred" + defer y{}.A(time.Since(now).Seconds()) // want "call to time.Since is not deferred" + defer (&y{}).B(time.Since(now).Seconds()) // want "call to time.Since is not deferred" + defer y{}.C(time.Since(now)) // want "call to time.Since is not deferred" + defer (&y{}).D(time.Since(now)) // want "call to time.Since is not deferred" +} + +func ugly() { + // The following is ok even though time.Since is evaluated. We don't + // walk into function literals or check what function definitions are doing. + defer x((func() time.Duration { return time.Since(time.Now()) })()) +}