[tools] go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel
4 views
Skip to first unread message
Robert Findley (Gerrit)
Sep 19, 2022, 1:39:43 PM9/19/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Robert Findley uploaded patch set #6 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Robert Findley, TryBot-Result-1 by Gopher Robot, gopls-CI-1 by kokoro
go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel
Add experimental new logic to the loopclosure analyzer that checks for
access to loop variables from parallel subtests. For now, this is gated
behind an internal variable so that we may experiment with it without
yet affecting cmd/vet.
Add an internal/loopclosure command that enables the experimental new
check.
Change-Id: Ibf7e388a58a2b3e712e66e095d06612cf3ea918c
---
M go/analysis/passes/loopclosure/loopclosure.go
M go/analysis/passes/loopclosure/loopclosure_test.go
M go/analysis/passes/loopclosure/testdata/src/a/a.go
A go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
M gopls/doc/analyzers.md
M gopls/internal/lsp/source/api_json.go
M internal/analysisinternal/analysis.go
A internal/loopclosure/main.go
8 files changed, 325 insertions(+), 82 deletions(-)
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Sep 19, 2022, 1:50:30 PM9/19/22
to Robert Findley, goph...@pubsubhelper.golang.org, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/b93b45a0-a747-405c-bdce-3e9877b05d52
Patch set 6:gopls-CI +1
Alan Donovan (Gerrit)
Sep 19, 2022, 3:03:37 PM9/19/22
to Robert Findley, goph...@pubsubhelper.golang.org, David Chase, kokoro, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Patch set 6:Code-Review +2
2 comments:
File go/analysis/passes/loopclosure/loopclosure.go:
whole
program"can't ... without whole program analysis" isn't really accurate (my bad).
Also, active voice, and s/we/the analysis/. How about:
// The analyzer only considers references in the last statement of the
// loop body as it is not deep enough to understand the effects of
// subsequent statements which might render the reference benign.Patch Set #6, Line 143:
if v.Obj == id.Obj {(Preexisting)
The use of the old Ident.Obj resolution is a relic of the pre-go/types age, and has at least one interesting bug: without type information, it can't tell whether m{k: 0} is a map literal (in which case k is a reference) or a struct literal (in which case k is a field name). I think this would mean that the old mechanism either treats it as a struct always, or as a map always---which is why the Type==nil check above exists... but it itself appears to be based on a possible bug in go/types, namely that field name ids in struct literals do not have an entry in Info.Types.
It would be better if this code used Defs and Uses to connect the refs with their vars. And perhaps we should ensure that go/types always reports a Types entry for struct literal fields.
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Sep 19, 2022, 3:40:13 PM9/19/22
to Robert Findley, goph...@pubsubhelper.golang.org, Alan Donovan, kokoro, Gopher Robot, golang-co...@googlegroups.com
1 comment:
File internal/analysisinternal/analysis.go:
Patch Set #6, Line 23:
var LoopclosureParallelSubtests = falseDoes this mean the test is not run by default?
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Sep 19, 2022, 4:01:46 PM9/19/22
to goph...@pubsubhelper.golang.org, David Chase, Alan Donovan, kokoro, Gopher Robot, golang-co...@googlegroups.com
1 comment:
File internal/analysisinternal/analysis.go:
Patch Set #6, Line 23:
var LoopclosureParallelSubtests = falseDoes this mean the test is not run by default?
No, this var is set in the test runner (see loopclosure_test.go).
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Sep 20, 2022, 4:59:45 PM9/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Robert Findley uploaded patch set #7 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Robert Findley, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel
Add experimental new logic to the loopclosure analyzer that checks for
access to loop variables from parallel subtests. For now, this is gated
behind an internal variable so that we may experiment with it without
yet affecting cmd/vet.
Add an internal/loopclosure command that enables the experimental new
check.
Change-Id: Ibf7e388a58a2b3e712e66e095d06612cf3ea918c
---
M go/analysis/passes/loopclosure/loopclosure.go
M go/analysis/passes/loopclosure/loopclosure_test.go
M go/analysis/passes/loopclosure/testdata/src/a/a.go
A go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
M gopls/doc/analyzers.md
M gopls/internal/lsp/source/api_json.go
M internal/analysisinternal/analysis.go
A internal/loopclosure/main.go
8 files changed, 326 insertions(+), 83 deletions(-)
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Sep 20, 2022, 5:00:42 PM9/20/22
to goph...@pubsubhelper.golang.org, David Chase, Alan Donovan, kokoro, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Patch set 6:Run-TryBot +1
2 comments:
File go/analysis/passes/loopclosure/loopclosure.go:
whole
program
"can't ... without whole program analysis" isn't really accurate (my bad). […]
Took your suggestion verbatim. Thanks!
Patch Set #6, Line 143:
if v.Obj == id.Obj {
(Preexisting) […]
Agreed, let's do this in a follow-up CL.
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Sep 20, 2022, 5:10:27 PM9/20/22
to Robert Findley, goph...@pubsubhelper.golang.org, Gopher Robot, David Chase, Alan Donovan, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/6d787742-9feb-40e0-b6b3-b24893803416
Patch set 7:gopls-CI -1
Robert Findley (Gerrit)
Sep 20, 2022, 5:50:34 PM9/20/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Robert Findley uploaded patch set #8 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Robert Findley, TryBot-Result-1 by Gopher Robot, gopls-CI-1 by kokoro
go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel
Add experimental new logic to the loopclosure analyzer that checks for
access to loop variables from parallel subtests. For now, this is gated
behind an internal variable so that we may experiment with it without
yet affecting cmd/vet.
Add an internal/loopclosure command that enables the experimental new
check.
Change-Id: Ibf7e388a58a2b3e712e66e095d06612cf3ea918c
---
M go/analysis/passes/loopclosure/loopclosure.go
M go/analysis/passes/loopclosure/loopclosure_test.go
M go/analysis/passes/loopclosure/testdata/src/a/a.go
A go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
M gopls/doc/analyzers.md
M gopls/internal/lsp/source/api_json.go
M internal/analysisinternal/analysis.go
A internal/loopclosure/main.go
8 files changed, 327 insertions(+), 84 deletions(-)
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Sep 20, 2022, 6:02:22 PM9/20/22
to Robert Findley, goph...@pubsubhelper.golang.org, Gopher Robot, David Chase, Alan Donovan, golang-co...@googlegroups.com
Attention is currently required from: Robert Findley.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/85a1f0f7-af20-416e-8c5f-87cc9ce0a1f4
Patch set 8:gopls-CI +1
Robert Findley (Gerrit)
Sep 20, 2022, 6:09:38 PM9/20/22
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, kokoro, Gopher Robot, David Chase, Alan Donovan, golang-co...@googlegroups.com
Robert Findley submitted this change.
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: gopls/internal/lsp/source/api_json.go
Insertions: 2, Deletions: 2.
@@ -300,7 +300,7 @@
},
{
Name: "\"loopclosure\"",
- Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nIn these cases only the last statement of the loop body is considered as we\ncan't know whether the loop variable access escapes the iteration without whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+ Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
Default: "true",
},
{
@@ -926,7 +926,7 @@
},
{
Name: "loopclosure",
- Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nIn these cases only the last statement of the loop body is considered as we\ncan't know whether the loop variable access escapes the iteration without whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+ Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
Default: true,
},
{
```
```
The name of the file: gopls/doc/analyzers.md
Insertions: 3, Deletions: 3.
@@ -224,9 +224,9 @@
1. a call to go or defer at the end of the loop body
2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
-In these cases only the last statement of the loop body is considered as we
-can't know whether the loop variable access escapes the iteration without whole
-program analysis.
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
For example:
```
```
The name of the file: go/analysis/passes/loopclosure/loopclosure.go
Insertions: 3, Deletions: 3.
@@ -25,9 +25,9 @@
1. a call to go or defer at the end of the loop body
2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
-In these cases only the last statement of the loop body is considered as we
-can't know whether the loop variable access escapes the iteration without whole
-program analysis.
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
For example:
```
Approvals:
Alan Donovan: Looks good to me, approved
kokoro: gopls CI succeeded
David Chase: Looks good to me, approved
Gopher Robot: TryBots succeeded
Robert Findley: Run TryBots
go/analysis/passes/loopclosure: experiment with checking t.Run+Parallel
Add experimental new logic to the loopclosure analyzer that checks for
access to loop variables from parallel subtests. For now, this is gated
behind an internal variable so that we may experiment with it without
yet affecting cmd/vet.
Add an internal/loopclosure command that enables the experimental new
check.
Change-Id: Ibf7e388a58a2b3e712e66e095d06612cf3ea918c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/430916
Reviewed-by: David Chase <drc...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Robert Findley <rfin...@google.com>
gopls-CI: kokoro <noreply...@google.com>
Reviewed-by: Alan Donovan <adon...@google.com>
---
M go/analysis/passes/loopclosure/loopclosure.go
M go/analysis/passes/loopclosure/loopclosure_test.go
M go/analysis/passes/loopclosure/testdata/src/a/a.go
A go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
M gopls/doc/analyzers.md
M gopls/internal/lsp/source/api_json.go
M internal/analysisinternal/analysis.go
A internal/loopclosure/main.go
8 files changed, 333 insertions(+), 84 deletions(-)
diff --git a/go/analysis/passes/loopclosure/loopclosure.go b/go/analysis/passes/loopclosure/loopclosure.go
index 98de9a9..aed3e43 100644
--- a/go/analysis/passes/loopclosure/loopclosure.go
+++ b/go/analysis/passes/loopclosure/loopclosure.go
@@ -14,15 +14,20 @@
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/go/types/typeutil"
+ "golang.org/x/tools/internal/analysisinternal"
)
const Doc = `check references to loop variables from within nested functions
-This analyzer checks for references to loop variables from within a
-function literal inside the loop body. It checks only instances where
-the function literal is called in a defer or go statement that is the
-last statement in the loop body, as otherwise we would need whole
-program analysis.
+This analyzer checks for references to loop variables from within a function
+literal inside the loop body. It checks for patterns where access to a loop
+variable is known to escape the current loop iteration:
+ 1. a call to go or defer at the end of the loop body
+ 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
+
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
For example:
@@ -34,6 +39,10 @@
See: https://golang.org/doc/go_faq.html#closures_and_goroutines`
+// TODO(rfindley): enable support for checking parallel subtests, pending
+// investigation, adding:
+// 3. a call testing.T.Run where the subtest body invokes t.Parallel()
+
var Analyzer = &analysis.Analyzer{
Name: "loopclosure",
Doc: Doc,
@@ -79,52 +88,71 @@
return
}
- // Inspect a go or defer statement
- // if it's the last one in the loop body.
- // (We give up if there are following statements,
- // because it's hard to prove go isn't followed by wait,
- // or defer by return.)
- if len(body.List) == 0 {
- return
- }
- // The function invoked in the last return statement.
- var fun ast.Expr
- switch s := body.List[len(body.List)-1].(type) {
- case *ast.GoStmt:
- fun = s.Call.Fun
- case *ast.DeferStmt:
- fun = s.Call.Fun
- case *ast.ExprStmt: // check for errgroup.Group.Go()
- if call, ok := s.X.(*ast.CallExpr); ok {
- fun = goInvokes(pass.TypesInfo, call)
- }
- }
- lit, ok := fun.(*ast.FuncLit)
- if !ok {
- return
- }
- ast.Inspect(lit.Body, func(n ast.Node) bool {
- id, ok := n.(*ast.Ident)
- if !ok || id.Obj == nil {
- return true
- }
- if pass.TypesInfo.Types[id].Type == nil {
- // Not referring to a variable (e.g. struct field name)
- return true
- }
- for _, v := range vars {
- if v.Obj == id.Obj {
- pass.ReportRangef(id, "loop variable %s captured by func literal",
- id.Name)
+ // Inspect statements to find function literals that may be run outside of
+ // the current loop iteration.
+ //
+ // For go, defer, and errgroup.Group.Go, we ignore all but the last
+ // statement, because it's hard to prove go isn't followed by wait, or
+ // defer by return.
+ //
+ // We consider every t.Run statement in the loop body, because there is
+ // no such commonly used mechanism for synchronizing parallel subtests.
+ // It is of course theoretically possible to synchronize parallel subtests,
+ // though such a pattern is likely to be exceedingly rare as it would be
+ // fighting against the test runner.
+ lastStmt := len(body.List) - 1
+ for i, s := range body.List {
+ var fun ast.Expr // if non-nil, a function that escapes the loop iteration
+ switch s := s.(type) {
+ case *ast.GoStmt:
+ if i == lastStmt {
+ fun = s.Call.Fun
+ }
+
+ case *ast.DeferStmt:
+ if i == lastStmt {
+ fun = s.Call.Fun
+ }
+
+ case *ast.ExprStmt: // check for errgroup.Group.Go and testing.T.Run (with T.Parallel)
+ if call, ok := s.X.(*ast.CallExpr); ok {
+ if i == lastStmt {
+ fun = goInvoke(pass.TypesInfo, call)
+ }
+ if fun == nil && analysisinternal.LoopclosureParallelSubtests {
+ fun = parallelSubtest(pass.TypesInfo, call)
+ }
}
}
- return true
- })
+
+ lit, ok := fun.(*ast.FuncLit)
+ if !ok {
+ continue
+ }
+
+ ast.Inspect(lit.Body, func(n ast.Node) bool {
+ id, ok := n.(*ast.Ident)
+ if !ok || id.Obj == nil {
+ return true
+ }
+ if pass.TypesInfo.Types[id].Type == nil {
+ // Not referring to a variable (e.g. struct field name)
+ return true
+ }
+ for _, v := range vars {
+ if v.Obj == id.Obj {
+ pass.ReportRangef(id, "loop variable %s captured by func literal",
+ id.Name)
+ }
+ }
+ return true
+ })
+ }
})
return nil, nil
}
-// goInvokes returns a function expression that would be called asynchronously
+// goInvoke returns a function expression that would be called asynchronously
// (but not awaited) in another goroutine as a consequence of the call.
// For example, given the g.Go call below, it returns the function literal expression.
//
@@ -133,33 +161,89 @@
// g.Go(func() error { ... })
//
// Currently only "golang.org/x/sync/errgroup.Group()" is considered.
-func goInvokes(info *types.Info, call *ast.CallExpr) ast.Expr {
- f := typeutil.StaticCallee(info, call)
- // Note: Currently only supports: golang.org/x/sync/errgroup.Go.
- if f == nil || f.Name() != "Go" {
- return nil
- }
- recv := f.Type().(*types.Signature).Recv()
- if recv == nil {
- return nil
- }
- rtype, ok := recv.Type().(*types.Pointer)
- if !ok {
- return nil
- }
- named, ok := rtype.Elem().(*types.Named)
- if !ok {
- return nil
- }
- if named.Obj().Name() != "Group" {
- return nil
- }
- pkg := f.Pkg()
- if pkg == nil {
- return nil
- }
- if pkg.Path() != "golang.org/x/sync/errgroup" {
+func goInvoke(info *types.Info, call *ast.CallExpr) ast.Expr {
+ if !isMethodCall(info, call, "golang.org/x/sync/errgroup", "Group", "Go") {
return nil
}
return call.Args[0]
}
+
+// parallelSubtest returns a function expression that would be called
+// asynchronously via the go test runner, as t.Run has been invoked with a
+// function literal that calls t.Parallel.
+//
+// import "testing"
+//
+// func TestFoo(t *testing.T) {
+// tests := []int{0, 1, 2}
+// for i, t := range tests {
+// t.Run("subtest", func(t *testing.T) {
+// t.Parallel()
+// println(i, t)
+// })
+// }
+// }
+func parallelSubtest(info *types.Info, call *ast.CallExpr) ast.Expr {
+ if !isMethodCall(info, call, "testing", "T", "Run") {
+ return nil
+ }
+
+ lit, ok := call.Args[1].(*ast.FuncLit)
+ if !ok {
+ return nil
+ }
+
+ for _, stmt := range lit.Body.List {
+ exprStmt, ok := stmt.(*ast.ExprStmt)
+ if !ok {
+ continue
+ }
+ if isMethodCall(info, exprStmt.X, "testing", "T", "Parallel") {
+ return lit
+ }
+ }
+
+ return nil
+}
+
+// isMethodCall reports whether expr is a method call of
+// <pkgPath>.<typeName>.<method>.
+func isMethodCall(info *types.Info, expr ast.Expr, pkgPath, typeName, method string) bool {
+ call, ok := expr.(*ast.CallExpr)
+ if !ok {
+ return false
+ }
+
+ // Check that we are calling a method <method>
+ f := typeutil.StaticCallee(info, call)
+ if f == nil || f.Name() != method {
+ return false
+ }
+ recv := f.Type().(*types.Signature).Recv()
+ if recv == nil {
+ return false
+ }
+
+ // Check that the receiver is a <pkgPath>.<typeName> or
+ // *<pkgPath>.<typeName>.
+ rtype := recv.Type()
+ if ptr, ok := recv.Type().(*types.Pointer); ok {
+ rtype = ptr.Elem()
+ }
+ named, ok := rtype.(*types.Named)
+ if !ok {
+ return false
+ }
+ if named.Obj().Name() != typeName {
+ return false
+ }
+ pkg := f.Pkg()
+ if pkg == nil {
+ return false
+ }
+ if pkg.Path() != pkgPath {
+ return false
+ }
+
+ return true
+}
diff --git a/go/analysis/passes/loopclosure/loopclosure_test.go b/go/analysis/passes/loopclosure/loopclosure_test.go
index 1498838..e00ae21 100644
--- a/go/analysis/passes/loopclosure/loopclosure_test.go
+++ b/go/analysis/passes/loopclosure/loopclosure_test.go
@@ -5,9 +5,11 @@
package loopclosure_test
import (
- "golang.org/x/tools/internal/typeparams"
"testing"
+ "golang.org/x/tools/internal/analysisinternal"
+ "golang.org/x/tools/internal/typeparams"
+
"golang.org/x/tools/go/analysis/analysistest"
"golang.org/x/tools/go/analysis/passes/loopclosure"
)
@@ -19,4 +21,12 @@
tests = append(tests, "typeparams")
}
analysistest.Run(t, testdata, loopclosure.Analyzer, tests...)
+
+ // Enable checking of parallel subtests.
+ defer func(parallelSubtest bool) {
+ analysisinternal.LoopclosureParallelSubtests = parallelSubtest
+ }(analysisinternal.LoopclosureParallelSubtests)
+ analysisinternal.LoopclosureParallelSubtests = true
+
+ analysistest.Run(t, testdata, loopclosure.Analyzer, "subtests")
}
diff --git a/go/analysis/passes/loopclosure/testdata/src/a/a.go b/go/analysis/passes/loopclosure/testdata/src/a/a.go
index 2c8e2e6..2fc3f21 100644
--- a/go/analysis/passes/loopclosure/testdata/src/a/a.go
+++ b/go/analysis/passes/loopclosure/testdata/src/a/a.go
@@ -6,7 +6,9 @@
package testdata
-import "golang.org/x/sync/errgroup"
+import (
+ "golang.org/x/sync/errgroup"
+)
func _() {
var s []int
@@ -91,9 +93,9 @@
}
}
-// Group is used to test that loopclosure does not match on any type named "Group".
-// The checker only matches on methods "(*...errgroup.Group).Go".
-type Group struct{};
+// Group is used to test that loopclosure only matches Group.Go when Group is
+// from the golang.org/x/sync/errgroup package.
+type Group struct{}
func (g *Group) Go(func() error) {}
diff --git a/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go b/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
new file mode 100644
index 0000000..4bcd495
--- /dev/null
+++ b/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go
@@ -0,0 +1,99 @@
+// Copyright 2022 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 that the loopclosure analyzer detects leaked
+// references via parallel subtests.
+
+package subtests
+
+import (
+ "testing"
+)
+
+// T is used to test that loopclosure only matches T.Run when T is from the
+// testing package.
+type T struct{}
+
+// Run should not match testing.T.Run. Note that the second argument is
+// intentionally a *testing.T, not a *T, so that we can check both
+// testing.T.Parallel inside a T.Run, and a T.Parallel inside a testing.T.Run.
+func (t *T) Run(string, func(*testing.T)) { // The second argument here is testing.T
+}
+
+func (t *T) Parallel() {}
+
+func _(t *testing.T) {
+ for i, test := range []int{1, 2, 3} {
+ // Check that parallel subtests are identified.
+ t.Run("", func(t *testing.T) {
+ t.Parallel()
+ println(i) // want "loop variable i captured by func literal"
+ println(test) // want "loop variable test captured by func literal"
+ })
+
+ // Check that serial tests are OK.
+ t.Run("", func(t *testing.T) {
+ println(i)
+ println(test)
+ })
+
+ // Check that the location of t.Parallel does not matter.
+ t.Run("", func(t *testing.T) {
+ println(i) // want "loop variable i captured by func literal"
+ println(test) // want "loop variable test captured by func literal"
+ t.Parallel()
+ })
+
+ // Check uses in nested blocks.
+ t.Run("", func(t *testing.T) {
+ t.Parallel()
+ {
+ println(i) // want "loop variable i captured by func literal"
+ println(test) // want "loop variable test captured by func literal"
+ }
+ })
+
+ // Check that we catch uses in nested subtests.
+ t.Run("", func(t *testing.T) {
+ t.Parallel()
+ t.Run("", func(t *testing.T) {
+ println(i) // want "loop variable i captured by func literal"
+ println(test) // want "loop variable test captured by func literal"
+ })
+ })
+
+ // Check that there is no diagnostic if t is not a *testing.T.
+ t.Run("", func(_ *testing.T) {
+ t := &T{}
+ t.Parallel()
+ println(i)
+ println(test)
+ })
+ }
+}
+
+// Check that there is no diagnostic when loop variables are shadowed within
+// the loop body.
+func _(t *testing.T) {
+ for i, test := range []int{1, 2, 3} {
+ i := i
+ test := test
+ t.Run("", func(t *testing.T) {
+ t.Parallel()
+ println(i)
+ println(test)
+ })
+ }
+}
+
+// Check that t.Run must be *testing.T.Run.
+func _(t *T) {
+ for i, test := range []int{1, 2, 3} {
+ t.Run("", func(t *testing.T) {
+ t.Parallel()
+ println(i)
+ println(test)
+ })
+ }
+}
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index 90a5118..dfe2a43 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -218,11 +218,15 @@
check references to loop variables from within nested functions
-This analyzer checks for references to loop variables from within a
-function literal inside the loop body. It checks only instances where
-the function literal is called in a defer or go statement that is the
-last statement in the loop body, as otherwise we would need whole
-program analysis.
+This analyzer checks for references to loop variables from within a function
+literal inside the loop body. It checks for patterns where access to a loop
+variable is known to escape the current loop iteration:
+ 1. a call to go or defer at the end of the loop body
+ 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body
+
+The analyzer only considers references in the last statement of the loop body
+as it is not deep enough to understand the effects of subsequent statements
+which might render the reference benign.
For example:
diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go
index 6231507..957319a 100755
--- a/gopls/internal/lsp/source/api_json.go
+++ b/gopls/internal/lsp/source/api_json.go
@@ -300,7 +300,7 @@
},
{
Name: "\"loopclosure\"",
- Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+ Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
Default: "true",
},
{
@@ -926,7 +926,7 @@
},
{
Name: "loopclosure",
- Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a\nfunction literal inside the loop body. It checks only instances where\nthe function literal is called in a defer or go statement that is the\nlast statement in the loop body, as otherwise we would need whole\nprogram analysis.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
+ Doc: "check references to loop variables from within nested functions\n\nThis analyzer checks for references to loop variables from within a function\nliteral inside the loop body. It checks for patterns where access to a loop\nvariable is known to escape the current loop iteration:\n 1. a call to go or defer at the end of the loop body\n 2. a call to golang.org/x/sync/errgroup.Group.Go at the end of the loop body\n\nThe analyzer only considers references in the last statement of the loop body\nas it is not deep enough to understand the effects of subsequent statements\nwhich might render the reference benign.\n\nFor example:\n\n\tfor i, v := range s {\n\t\tgo func() {\n\t\t\tprintln(i, v) // not what you might expect\n\t\t}()\n\t}\n\nSee: https://golang.org/doc/go_faq.html#closures_and_goroutines",
Default: true,
},
{
diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go
index e32152a..d538e07 100644
--- a/internal/analysisinternal/analysis.go
+++ b/internal/analysisinternal/analysis.go
@@ -14,9 +14,14 @@
"strconv"
)
-// Flag to gate diagnostics for fuzz tests in 1.18.
+// DiagnoseFuzzTests controls whether the 'tests' analyzer diagnoses fuzz tests
+// in Go 1.18+.
var DiagnoseFuzzTests bool = false
+// LoopclosureParallelSubtests controls whether the 'loopclosure' analyzer
+// diagnoses loop variables references in parallel subtests.
+var LoopclosureParallelSubtests = false
+
var (
GetTypeErrors func(p interface{}) []types.Error
SetTypeErrors func(p interface{}, errors []types.Error)
diff --git a/internal/loopclosure/main.go b/internal/loopclosure/main.go
new file mode 100644
index 0000000..03238ed
--- /dev/null
+++ b/internal/loopclosure/main.go
@@ -0,0 +1,22 @@
+// Copyright 2022 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 loopclosure command applies the golang.org/x/tools/go/analysis/passes/loopclosure
+// analysis to the specified packages of Go source code. It enables
+// experimental checking of parallel subtests.
+//
+// TODO: Once the parallel subtest experiment is complete, this can be made
+// public at go/analysis/passes/loopclosure/cmd, or deleted.
+package main
+
+import (
+ "golang.org/x/tools/go/analysis/passes/loopclosure"
+ "golang.org/x/tools/go/analysis/singlechecker"
+ "golang.org/x/tools/internal/analysisinternal"
+)
+
+func main() {
+ analysisinternal.LoopclosureParallelSubtests = true
+ singlechecker.Main(loopclosure.Analyzer)
+}
To view, visit change 430916. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages