Skip to content

Commit 2a84b3d

Browse files
thepuddsthepudds
committed
go/analysis/passes/loopclosure: add filtering to allow certain well-understood statements to trail problematic go and defer statements
1 parent 9f2680c commit 2a84b3d

File tree

4 files changed

+704
-12
lines changed

4 files changed

+704
-12
lines changed

‎go/analysis/passes/loopclosure/lastfilter.go

Lines changed: 289 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,289 @@
1+
// Copyright 2022 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package loopclosure
6+
7+
import (
8+
"go/ast"
9+
"go/token"
10+
"go/types"
11+
12+
"golang.org/x/tools/go/types/typeutil"
13+
)
14+
15+
// filter provides filtering for statements and expressions that are
16+
// understood well enough to conclude that they cannot stop the flow of execution
17+
// within a function body, such as by calling into an unknown function,
18+
// waiting, or panicking. It is effectively an "allow list" approach,
19+
// conservatively giving up on statements and expressions it does not understand.
20+
type filter struct {
21+
info *types.Info
22+
23+
// skipStmts tracks if we have already determined whether to skip a statement.
24+
// This is an optimization we only use for compound statements in order
25+
// to avoid recursively descending into the same compound statement multiple times
26+
// during filtering. The map value indicates whether to skip (that is, this is not a set).
27+
skipStmts map[ast.Stmt]bool
28+
}
29+
30+
func newFilter(info *types.Info) *filter {
31+
return &filter{
32+
info: info,
33+
skipStmts: make(map[ast.Stmt]bool),
34+
}
35+
}
36+
37+
// skipStmt reports that a statement can be skipped if the statement is unable to
38+
// stop the flow of execution within a function body, such as by calling
39+
// into an unknown function, waiting, or panicking.
40+
//
41+
// The current primary use case is to skip certain well-understood statements
42+
// that are unable to render a captured loop variable reference benign when
43+
// the statement follows a go, defer, or errgroup.Group.Go statement.
44+
//
45+
// For compound statements such as switch and if statements, it
46+
// recursively checks whether all statements within the bodies are skippable,
47+
// and if so, reports the parent statement as skippable.
48+
// Similarly, expressions within statements are recursively checked.
49+
// Statements it does not understand are conservatively reported as not skippable.
50+
// It understands certain builtins, such as len and append.
51+
// It does not skip select statements, which can cause a wait on a go statement.
52+
func (f *filter) skipStmt(v visitor, stmt ast.Stmt) bool {
53+
// TODO: consider differentiating what we skip for defer vs. go.
54+
// TODO: consider parameterizing, such as whether to allow panic, select, ...
55+
// TODO: more precise description of intent and nature of statements we skip.
56+
// TODO: allow *ast.DeclStmt (e.g., 'var a int').
57+
58+
// Check if we have already determined an answer for this statement.
59+
if skip, ok := f.skipStmts[stmt]; ok {
60+
return skip
61+
}
62+
63+
switch s := stmt.(type) {
64+
case nil:
65+
return true
66+
case *ast.AssignStmt:
67+
switch s.Tok {
68+
case token.QUO_ASSIGN, token.REM_ASSIGN, token.SHL_ASSIGN, token.SHR_ASSIGN:
69+
// TODO: consider allowing division and shift, which can panic
70+
return false
71+
}
72+
for _, e := range s.Rhs {
73+
if !f.skipExpr(e) {
74+
return false
75+
}
76+
}
77+
for _, e := range s.Lhs {
78+
if !f.skipExpr(e) {
79+
return false
80+
}
81+
}
82+
return true
83+
case *ast.BranchStmt:
84+
switch s.Tok {
85+
case token.CONTINUE, token.FALLTHROUGH:
86+
return true
87+
}
88+
case *ast.ExprStmt:
89+
if call, ok := s.X.(*ast.CallExpr); ok {
90+
return f.skipExpr(call)
91+
}
92+
case *ast.IncDecStmt:
93+
return f.skipExpr(s.X)
94+
case *ast.IfStmt:
95+
if !f.skipExpr(s.Cond) {
96+
f.skipStmts[stmt] = false // memoize
97+
return false
98+
}
99+
loop:
100+
for {
101+
for i := range s.Body.List {
102+
if !f.skipStmt(v, s.Body.List[i]) {
103+
f.skipStmts[stmt] = false
104+
return false
105+
}
106+
}
107+
switch e := s.Else.(type) {
108+
case *ast.BlockStmt:
109+
for i := range e.List {
110+
if !f.skipStmt(v, e.List[i]) {
111+
f.skipStmts[stmt] = false
112+
return false
113+
}
114+
}
115+
break loop
116+
case *ast.IfStmt:
117+
s = e
118+
case nil:
119+
break loop
120+
}
121+
}
122+
f.skipStmts[stmt] = true
123+
return true
124+
case *ast.ForStmt:
125+
if !f.skipStmt(v, s.Init) || !f.skipExpr(s.Cond) || !f.skipStmt(v, s.Post) {
126+
f.skipStmts[stmt] = false // memoize
127+
return false
128+
}
129+
for i := range s.Body.List {
130+
if !f.skipStmt(v, s.Body.List[i]) {
131+
f.skipStmts[stmt] = false
132+
return false
133+
}
134+
}
135+
f.skipStmts[stmt] = true
136+
return true
137+
case *ast.RangeStmt:
138+
// TODO: we might not need to check s.Key or s.Value?
139+
if !f.skipExpr(s.Key) || !f.skipExpr(s.Value) {
140+
f.skipStmts[stmt] = false // memoize
141+
return false
142+
}
143+
for i := range s.Body.List {
144+
if !f.skipStmt(v, s.Body.List[i]) {
145+
f.skipStmts[stmt] = false
146+
return false
147+
}
148+
}
149+
f.skipStmts[stmt] = true
150+
return true
151+
case *ast.SwitchStmt:
152+
if !f.skipExpr(s.Tag) || !f.skipStmt(v, s.Init) {
153+
f.skipStmts[stmt] = false // memoize
154+
return false
155+
}
156+
for i := range s.Body.List {
157+
cc := s.Body.List[i].(*ast.CaseClause)
158+
for _, x := range cc.List {
159+
if !f.skipExpr(x) {
160+
f.skipStmts[stmt] = false
161+
return false
162+
}
163+
}
164+
for _, ccStmt := range cc.Body {
165+
if !f.skipStmt(v, ccStmt) {
166+
f.skipStmts[stmt] = false
167+
return false
168+
}
169+
}
170+
}
171+
f.skipStmts[stmt] = true
172+
return true
173+
case *ast.TypeSwitchStmt:
174+
// TODO: confirm we don't need to check s.Assign
175+
if !f.skipStmt(v, s.Init) {
176+
f.skipStmts[stmt] = false // memoize
177+
return false
178+
}
179+
for i := range s.Body.List {
180+
cc := s.Body.List[i].(*ast.CaseClause)
181+
for _, x := range cc.List {
182+
if !f.skipExpr(x) {
183+
f.skipStmts[stmt] = false
184+
return false
185+
}
186+
}
187+
for _, ccStmt := range cc.Body {
188+
if !f.skipStmt(v, ccStmt) {
189+
f.skipStmts[stmt] = false
190+
return false
191+
}
192+
}
193+
}
194+
f.skipStmts[stmt] = true
195+
return true
196+
}
197+
198+
// We default to false if we don't have specific knowledge of a statement.
199+
return false
200+
}
201+
202+
// skipExpr is like skipStmt, but for expressions.
203+
func (f *filter) skipExpr(expr ast.Expr) bool {
204+
switch x := expr.(type) {
205+
case nil:
206+
return true
207+
case *ast.BasicLit:
208+
return true
209+
case *ast.BinaryExpr:
210+
switch x.Op {
211+
case token.QUO, token.REM, token.SHL, token.SHR:
212+
// TODO: consider allowing division and shift, which can panic
213+
return false
214+
}
215+
return f.skipExpr(x.X) && f.skipExpr(x.Y)
216+
case *ast.CallExpr:
217+
// TODO: consider allowing conversions like float64(i)
218+
fn := typeutil.Callee(f.info, x)
219+
if b, ok := fn.(*types.Builtin); ok {
220+
switch b.Name() {
221+
case "append", "cap", "copy", "delete", "len", "new":
222+
// These builtins do not panic, and cannot wait on the execution of a defer or go statement.
223+
// TODO: consider a possibly large "allow list" of stdlib funcs, such as strings.Contains.
224+
// TODO: consider allowing println, print, although this would require updating tests that use print now.
225+
// TODO: consider allowing fmt.Println and similar when arguments are all basic types, or otherwise
226+
// restricted from having String, Error, Format, [...] methods.
227+
for _, arg := range x.Args {
228+
if !f.skipExpr(arg) {
229+
return false
230+
}
231+
}
232+
return true
233+
}
234+
}
235+
case *ast.CompositeLit:
236+
// We handle things like pair{a: i, b: i}, where 'pair' is the *ast.Ident.
237+
// TODO: handle *ast.CompositeLit for slices and maps
238+
if ident, ok := x.Type.(*ast.Ident); ok {
239+
if obj := f.info.Uses[ident]; obj != nil {
240+
if _, ok := obj.Type().Underlying().(*types.Struct); ok {
241+
for _, elt := range x.Elts {
242+
kv, ok := elt.(*ast.KeyValueExpr)
243+
if !ok {
244+
return false
245+
}
246+
if !f.skipExpr(kv.Value) {
247+
return false
248+
}
249+
}
250+
return true
251+
}
252+
}
253+
}
254+
case *ast.Ident:
255+
return true
256+
case *ast.ParenExpr:
257+
return f.skipExpr(x.X)
258+
case *ast.SelectorExpr:
259+
// We only allow basic cases for selector expressions such as foo.bar,
260+
// where foo is an *ast.Ident with a struct object and not a pointer type.
261+
// TODO: we do not yet handle x.y.z = 1
262+
// TODO: probably add test for Call().bar and foo.Call().bar
263+
if !f.skipExpr(x.X) {
264+
return false
265+
}
266+
if ident, ok := x.X.(*ast.Ident); ok {
267+
obj := f.info.Uses[ident]
268+
if obj != nil {
269+
if _, ok := obj.Type().Underlying().(*types.Struct); ok {
270+
// We do not (currently) want to allow pointers here, given
271+
// a pointer dereference could panic.
272+
// TODO: consider allowing pointer types in selector expression.
273+
return true
274+
}
275+
}
276+
}
277+
case *ast.UnaryExpr:
278+
switch x.Op {
279+
// See https://go.dev/ref/spec#UnaryExpr
280+
case token.ADD, token.SUB, token.NOT, token.XOR, token.AND:
281+
// We disallow token.MUL because we currently do not want to allow dereference.
282+
// TODO: review this UnaryExpr list -- is it complete? are any of these issues?
283+
// TODO: confirm token.AND is not allowing more address operations than we expect.
284+
return f.skipExpr(x.X)
285+
}
286+
}
287+
// We default to false if we don't have specific knowledge of an expression.
288+
return false
289+
}

‎go/analysis/passes/loopclosure/loopclosure.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -159,19 +159,18 @@ func run(pass *analysis.Pass) (interface{}, error) {
159159
// Inspect statements to find function literals that may be run outside of
160160
// the current loop iteration.
161161
//
162-
// For go, defer, and errgroup.Group.Go, we ignore all but the last
163-
// statement, because it's hard to prove go isn't followed by wait, or
164-
// defer by return. "Last" is defined recursively.
165-
//
166-
// TODO: consider allowing the "last" go/defer/Go statement to be followed by
167-
// N "trivial" statements, possibly under a recursive definition of "trivial"
168-
// so that that checker could, for example, conclude that a go statement is
169-
// followed by an if statement made of only trivial statements and trivial expressions,
170-
// and hence the go statement could still be checked.
162+
// For go, defer, and errgroup.Group.Go, by default we ignore all but the last
163+
// statement, where "last" is defined recursively.
164+
// In addition, if a potentially problematic go, defer, or errgroup.Group.Go
165+
// statement is followed by one or more "last" statements that we can prove
166+
// do not cause a wait or otherwise derail the flow of execution sufficiently, then
167+
// we still examine the function literal within the potentially problematic statement.
168+
// TODO: consider differentiating between go vs. defer for what we can prove.
171169
gdv := goDeferVisitor{pass: pass, vars: vars}
172170
v := visitor{
173-
last: gdv.last,
174-
all: gdv.all,
171+
last: gdv.last,
172+
all: gdv.all,
173+
skipLast: newFilter(pass.TypesInfo).skipStmt,
175174
}
176175
v.visit(body.List)
177176

‎go/analysis/passes/loopclosure/loopclosure_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414

1515
func Test(t *testing.T) {
1616
testdata := analysistest.TestData()
17-
tests := []string{"a", "golang.org/...", "subtests"}
17+
tests := []string{"a", "golang.org/...", "subtests", "go121"}
1818
if typeparams.Enabled {
1919
tests = append(tests, "typeparams")
2020
}

0 commit comments

Comments
 (0)