-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 <adonovan@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Alan Donovan <adonovan@google.com>
- Loading branch information
Showing
5 changed files
with
174 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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") | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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()) })()) | ||
} |