[tools] go/analysis: add runtime.Cleanup pass
6 views
Skip to first unread message
Daniel Morsing (Gerrit)
Aug 17, 2025, 9:23:06 AMAug 17
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Daniel Morsing has uploaded the change for review![]( "Open in Gerrit")
Commit message
go/analysis: add runtime.Cleanup pass
This pass checks for various pitfalls when using runtime cleanups.
Mostly, it makes sure that the pointer isn't closed over by the function
or the argument to the function, effectively disabling the mechanism.
Change-Id: If645b66d1ad91b0c9ec478d03b751caa18366087
Change diff
diff --git a/go/analysis/passes/cleanup/cleanup.go b/go/analysis/passes/cleanup/cleanup.go
new file mode 100644
index 0000000..9144949
--- /dev/null
+++ b/go/analysis/passes/cleanup/cleanup.go
@@ -0,0 +1,110 @@
+// Copyright 2025 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 cleanup defines an Analyzer that detects common mistakes
+// involving runtime cleanups.
+package cleanup
+
+import (
+ "go/ast"
+ "go/token"
+ "go/types"
+
+ "golang.org/x/tools/go/analysis"
+ typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex"
+ "golang.org/x/tools/internal/typesinternal/typeindex"
+)
+
+const Doc = "check for common mistakes involving runtime cleanups"
+
+var Analyzer = &analysis.Analyzer{
+ Name: "cleanup",
+ Doc: Doc,
+ URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/cleanup",
+ Requires: []*analysis.Analyzer{typeindexanalyzer.Analyzer},
+ Run: run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+ index := pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index)
+
+ cleanup := index.Object("runtime", "AddCleanup")
+ if !index.Used(cleanup) {
+ return nil, nil
+ }
+
+ for c := range index.Calls(cleanup) {
+ call := c.Node().(*ast.CallExpr)
+ ptr := ast.Unparen(call.Args[0])
+ // Calls and receives yield a value that is unrelated to its identifier
+ // construction, so ignore their identifiers
+ // TODO: should we disallow these rather than just ignore them?
+ switch node := ptr.(type) {
+ case *ast.CallExpr:
+ continue
+ case *ast.UnaryExpr:
+ if node.Op == token.ARROW {
+ continue
+ }
+ default:
+ }
+ ptrVariables := make(map[types.Object]bool)
+ for id := range c.Child(call.Args[0]).Preorder((*ast.Ident)(nil)) {
+ // walk every identifier in the ptr expression and build a list of variables
+ // that shouldn't be closed over by any function literal.
+ // We're walking over an expression with no block statements, so any object found will
+ // be to variables in this scope.
+ obj := pass.TypesInfo.Uses[id.Node().(*ast.Ident)]
+ if _, ok := obj.(*types.PkgName); ok {
+ continue
+ }
+ // field or method selection
+ scope := obj.Parent()
+ if scope == nil {
+ continue
+ }
+ ptrVariables[obj] = true
+ }
+ cur := c.Child(call.Args[2])
+ cursor:
+ for id := range cur.Preorder((*ast.Ident)(nil)) {
+ ident := id.Node().(*ast.Ident)
+ obj := pass.TypesInfo.Uses[ident]
+ if obj == nil {
+ continue
+ }
+ if ptrVariables[obj] {
+ for id := range cur.Enclosing((*ast.SelectorExpr)(nil)) {
+ sel := id.Node().(*ast.SelectorExpr)
+ if pass.TypesInfo.Selections[sel].Indirect() {
+ // we referenced a identifier from ptr, but it is a selection
+ // that dereferences, so the variable isn't closed over
+ continue cursor
+ }
+ // we only care about the innermost selection
+ break
+ }
+ pass.ReportRangef(ident, "argument closes over %s, cleanup will never run or might panic", ident.Name)
+ break
+ }
+ }
+ cur = c.Child(call.Args[1])
+ if fn, ok := ast.Unparen(call.Args[1]).(*ast.FuncLit); ok {
+ // don't look at the parameters for function literals
+ cur, _ = cur.FindNode(fn.Body)
+ }
+ for id := range cur.Preorder((*ast.Ident)(nil)) {
+ ident := id.Node().(*ast.Ident)
+ obj := pass.TypesInfo.Uses[ident]
+ if obj == nil {
+ continue
+ }
+ if ptrVariables[obj] {
+ pass.ReportRangef(ident, "function closes over %s, cleanup will never run", ident.Name)
+ break
+ }
+ }
+ }
+ return nil, nil
+}
diff --git a/go/analysis/passes/cleanup/cleanup_test.go b/go/analysis/passes/cleanup/cleanup_test.go
new file mode 100644
index 0000000..e40f63f
--- /dev/null
+++ b/go/analysis/passes/cleanup/cleanup_test.go
@@ -0,0 +1,17 @@
+// Copyright 2025 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 cleanup_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/cleanup"
+)
+
+func Test(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, cleanup.Analyzer, "a")
+}
diff --git a/go/analysis/passes/cleanup/testdata/src/a/a.go b/go/analysis/passes/cleanup/testdata/src/a/a.go
new file mode 100644
index 0000000..b74aae9
--- /dev/null
+++ b/go/analysis/passes/cleanup/testdata/src/a/a.go
@@ -0,0 +1,47 @@
+// Copyright 2025 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.
+
+// This file contains tests for the cleanup checker.
+
+package a
+
+import "runtime"
+
+func PointerAsArg(i *int) {
+ runtime.AddCleanup(i, func(*int) {}, i) // want `argument closes over i`
+}
+
+type netFD struct {
+ fd int
+}
+
+func nfdclose(fd int) {
+}
+
+func (nfd *netFD) close(fd int) {
+}
+
+func NormalUse(nfd *netFD) {
+ runtime.AddCleanup(nfd, func(fd int) { // No error, this is the intended usage
+ }, nfd.fd)
+}
+
+func NormalUseFunc(nfd *netFD) {
+ runtime.AddCleanup(nfd, nfdclose, nfd.fd) // Using a function defined elsewhere is fine
+}
+
+func Address(nfd *netFD) {
+ runtime.AddCleanup(nfd, func(*int) {
+ }, &nfd.fd) // want `argument closes over nfd`
+}
+
+func ClosureMethod(nfd *netFD) {
+ runtime.AddCleanup(nfd, nfd.close, 0) // want `function closes over nfd, cleanup will never run`
+}
+
+func Closure(nfd *netFD) {
+ runtime.AddCleanup(nfd, func(fd int) {
+ nfd.close(nfd.fd) // want `function closes over nfd, cleanup will never run`
+ }, 0)
+}
Change information
Files:
- A go/analysis/passes/cleanup/cleanup.go
- A go/analysis/passes/cleanup/cleanup_test.go
- A go/analysis/passes/cleanup/testdata/src/a/a.go
Change size: M
Delta: 3 files changed, 174 insertions(+), 0 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Aug 17, 2025, 9:32:08 AMAug 17
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Daniel Morsing added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Daniel Morsing . resolved
Couple of notes on this:
This would have been trivial to implement with SSA, where I could look the free variables up when the closure is made, but I couldn't justify building an entire program for every function on the off-chance that it would use `runtime.AddCleanup`. Is there a way to selectively build SSA for a package under analysis?
I'm unsure what needs to happen to get this to run in vet. Where exactly does this need importing?
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Aug 18, 2025, 6:42:17 AMAug 18
to Daniel Morsing, goph...@pubsubhelper.golang.org, Michael Matloob, Robert Findley, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Daniel Morsing
Alan Donovan added 4 comments![]( "Open in Gerrit")
Commit Message
Line 7, Patchset 1 (Latest):
go/analysis: add runtime.Cleanup pass
This pass checks for various pitfalls when using runtime cleanups.
Mostly, it makes sure that the pointer isn't closed over by the function
or the argument to the function, effectively disabling the mechanism.Alan Donovan . unresolved
If you plan for this analyzer to be incorporated into cmd/vet, then it will require an approved proposal to create the new public API of its package. If this is just for gopls, it can be moved into a subdirectory of x/tools/gopls/internal/analysis without a proposal. However, even in that case, a tracking issue is preferred since it allows for public discussion and prioritization and ties together all the related code changes.
File go/analysis/passes/cleanup/cleanup.go
Line 19, Patchset 1 (Latest):
const Doc = "check for common mistakes involving runtime cleanups"Alan Donovan . unresolved
The documentation should go into more detail. See adjacent analyzers for examples.
I'd rather not review the complex logic below until I know exactly what it is supposed to do.
Line 33, Patchset 1 (Latest):
if !index.Used(cleanup) {
return nil, nil
}Alan Donovan . unresolved
This isn't needed: the index.Calls sequence will be empty in this case.
Related details
Attention is currently required from:
- Daniel Morsing
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Aug 18, 2025, 7:59:42 AMAug 18
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Daniel Morsing
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Daniel Morsing uploaded patch set #2 to this change.
Related details
Attention is currently required from:
- Daniel Morsing
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Aug 18, 2025, 8:05:24 AMAug 18
to goph...@pubsubhelper.golang.org, Alan Donovan, Michael Matloob, Robert Findley, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Daniel Morsing added 4 comments![]( "Open in Gerrit")
Commit Message
Line 7, Patchset 1:
go/analysis: add runtime.Cleanup pass
This pass checks for various pitfalls when using runtime cleanups.
Mostly, it makes sure that the pointer isn't closed over by the function
or the argument to the function, effectively disabling the mechanism.Alan Donovan . resolved
If you plan for this analyzer to be incorporated into cmd/vet, then it will require an approved proposal to create the new public API of its package. If this is just for gopls, it can be moved into a subdirectory of x/tools/gopls/internal/analysis without a proposal. However, even in that case, a tracking issue is preferred since it allows for public discussion and prioritization and ties together all the related code changes.
Daniel Morsing
I've moved the logic into gopls for now and will create an issue for vet inclusion.
File go/analysis/passes/cleanup/cleanup.go
Line 19, Patchset 1:
const Doc = "check for common mistakes involving runtime cleanups"Alan Donovan . resolved
The documentation should go into more detail. See adjacent analyzers for examples.
I'd rather not review the complex logic below until I know exactly what it is supposed to do.
Daniel Morsing
Have added a doc.go file, hopefully that explains the intent here a bit.
This isn't needed: the index.Calls sequence will be empty in this case.
Daniel Morsing
Done
Line 37, Patchset 1:
for c := range index.Calls(cleanup) {Alan Donovan . resolved
Daniel MorsingcurCall
Done
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Aug 18, 2025, 10:35:40 AMAug 18
to goph...@pubsubhelper.golang.org, Alan Donovan, Michael Matloob, Robert Findley, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Daniel Morsing added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Aug 18, 2025, 12:42:54 PMAug 18
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Daniel Morsing uploaded patch set #3 to this change.
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Aug 24, 2025, 6:54:44 PMAug 24
to Daniel Morsing, goph...@pubsubhelper.golang.org, Michael Matloob, Robert Findley, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Daniel Morsing
Alan Donovan added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Put a proposal up at https://go.dev/issues/75066
Alan Donovan
Thanks. I'll wait for the discussion to conclude the question of whether the vet check is warranted or the dynamic check is sufficient. Proposal issues FTW!
Related details
Attention is currently required from:
- Daniel Morsing
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Nov 27, 2025, 9:04:47 AM (3 days ago) Nov 27
to goph...@pubsubhelper.golang.org, Alan Donovan, Michael Matloob, Robert Findley, Gopher Robot, golang-co...@googlegroups.com
Daniel Morsing abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages