Skip to content

Commit

Permalink
go/analysis/passes/loopclosure: disable checker after go1.22.
Browse files Browse the repository at this point in the history
Uses the (*types.Info).FileVersion to disable the loopclosure checker
when in an *ast.File that uses GoVersion >= 1.22.

Updates golang/go#62605
Updates golang/go#63888

Change-Id: I2ebe974bc2ee2323eafb0f02d455ab76b3b9268d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/539016
Run-TryBot: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
timothy-king committed Nov 15, 2023
1 parent a1ca4fc commit cd62b43
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 10 deletions.
13 changes: 10 additions & 3 deletions go/analysis/passes/loopclosure/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@
// in such a way (e.g. with go or defer) that it may outlive the loop
// iteration and possibly observe the wrong value of the variable.
//
// Note: An iteration variable can only outlive a loop iteration in Go versions <=1.21.
// In Go 1.22 and later, the loop variable lifetimes changed to create a new
// iteration variable per loop iteration. (See go.dev/issue/60078.)
//
// In this example, all the deferred functions run after the loop has
// completed, so all observe the final value of v.
// completed, so all observe the final value of v [<go1.22].
//
// for _, v := range list {
// defer func() {
Expand All @@ -32,7 +36,10 @@
// }()
// }
//
// The next example uses a go statement and has a similar problem.
// After Go version 1.22, the previous two for loops are equivalent
// and both are correct.
//
// The next example uses a go statement and has a similar problem [<go1.22].
// In addition, it has a data race because the loop updates v
// concurrent with the goroutines accessing it.
//
Expand All @@ -56,7 +63,7 @@
// }
//
// The t.Parallel() call causes the rest of the function to execute
// concurrent with the loop.
// concurrent with the loop [<go1.22].
//
// The analyzer reports references only in the last statement,
// as it is not deep enough to understand the effects of subsequent
Expand Down
16 changes: 14 additions & 2 deletions go/analysis/passes/loopclosure/loopclosure.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
"golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/go/types/typeutil"
"golang.org/x/tools/internal/versions"
)

//go:embed doc.go
Expand All @@ -31,10 +32,15 @@ func run(pass *analysis.Pass) (interface{}, error) {
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)

nodeFilter := []ast.Node{
(*ast.File)(nil),
(*ast.RangeStmt)(nil),
(*ast.ForStmt)(nil),
}
inspect.Preorder(nodeFilter, func(n ast.Node) {
inspect.Nodes(nodeFilter, func(n ast.Node, push bool) bool {
if !push {
// inspect.Nodes is slightly suboptimal as we only use push=true.
return true
}
// Find the variables updated by the loop statement.
var vars []types.Object
addVar := func(expr ast.Expr) {
Expand All @@ -46,6 +52,11 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
var body *ast.BlockStmt
switch n := n.(type) {
case *ast.File:
// Only traverse the file if its goversion is strictly before go1.22.
goversion := versions.Lang(versions.FileVersions(pass.TypesInfo, n))
// goversion is empty for older go versions (or the version is invalid).
return goversion == "" || versions.Compare(goversion, "go1.22") < 0
case *ast.RangeStmt:
body = n.Body
addVar(n.Key)
Expand All @@ -64,7 +75,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
}
if vars == nil {
return
return true
}

// Inspect statements to find function literals that may be run outside of
Expand Down Expand Up @@ -113,6 +124,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
}
}
}
return true
})
return nil, nil
}
Expand Down
45 changes: 45 additions & 0 deletions go/analysis/passes/loopclosure/loopclosure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@
package loopclosure_test

import (
"os"
"path/filepath"
"testing"

"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/analysistest"
"golang.org/x/tools/go/analysis/passes/loopclosure"
"golang.org/x/tools/internal/testenv"
"golang.org/x/tools/internal/typeparams"
"golang.org/x/tools/txtar"
)

func Test(t *testing.T) {
Expand All @@ -20,3 +25,43 @@ func Test(t *testing.T) {
}
analysistest.Run(t, testdata, loopclosure.Analyzer, tests...)
}

func TestVersions22(t *testing.T) {
testenv.NeedsGo1Point(t, 22)

testfile := filepath.Join(analysistest.TestData(), "src", "versions", "go22.txtar")
runTxtarFile(t, testfile, loopclosure.Analyzer, "golang.org/fake/versions")
}

func TestVersions18(t *testing.T) {
testenv.NeedsGo1Point(t, 18)

testfile := filepath.Join(analysistest.TestData(), "src", "versions", "go18.txtar")
runTxtarFile(t, testfile, loopclosure.Analyzer, "golang.org/fake/versions")
}

// runTxtarFile unpacks a txtar archive to a directory, and runs
// analyzer on the given patterns.
//
// This is compatible with a go.mod file.
//
// TODO(taking): Consider unifying with analysistest.
func runTxtarFile(t *testing.T, path string, analyzer *analysis.Analyzer, patterns ...string) {
ar, err := txtar.ParseFile(path)
if err != nil {
t.Fatal(err)
}

dir := t.TempDir()
for _, file := range ar.Files {
name, content := file.Name, file.Data

filename := filepath.Join(dir, name)
os.MkdirAll(filepath.Dir(filename), 0777) // ignore error
if err := os.WriteFile(filename, content, 0666); err != nil {
t.Fatal(err)
}
}

analysistest.Run(t, dir, analyzer, patterns...)
}
43 changes: 43 additions & 0 deletions go/analysis/passes/loopclosure/testdata/src/versions/go18.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
Test loopclosure at go version go1.18.

-- go.mod --
module golang.org/fake/versions

go 1.18
-- pre.go --
//go:build go1.18

package versions

func InGo18(l []int) {
for i, v := range l {
go func() {
print(i) // want "loop variable i captured by func literal"
print(v) // want "loop variable v captured by func literal"
}()
}
}
-- go22.go --
//go:build go1.22

package versions

func InGo22(l []int) {
for i, v := range l {
go func() {
print(i) // Not reported due to file's GoVersion.
print(v) // Not reported due to file's GoVersion.
}()
}
}
-- modver.go --
package versions

func At18FromModuleVersion(l []int) {
for i, v := range l {
go func() {
print(i) // want "loop variable i captured by func literal"
print(v) // want "loop variable v captured by func literal"
}()
}
}
43 changes: 43 additions & 0 deletions go/analysis/passes/loopclosure/testdata/src/versions/go22.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
Test loopclosure at go version go1.22.
-- go.mod --
module golang.org/fake/versions

go 1.22
-- pre.go --
//go:build go1.19

package versions

func Bad(l []int) {
for i, v := range l {
go func() {
print(i) // want "loop variable i captured by func literal"
print(v) // want "loop variable v captured by func literal"
}()
}
}
-- go22.go --
//go:build go1.22

package versions

func InGo22(l []int) {
for i, v := range l {
go func() {
print(i) // Not reported due to file's GoVersion.
print(v) // Not reported due to file's GoVersion.
}()
}
}

-- modver.go --
package versions

func At22FromTheModuleVersion(l []int) {
for i, v := range l {
go func() {
print(i) // Not reported due to module's GoVersion.
print(v) // Not reported due to module's GoVersion.
}()
}
}
13 changes: 10 additions & 3 deletions gopls/doc/analyzers.md
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,12 @@ iteration variable of an enclosing loop, and the loop calls the function
in such a way (e.g. with go or defer) that it may outlive the loop
iteration and possibly observe the wrong value of the variable.

Note: An iteration variable can only outlive a loop iteration in Go versions <=1.21.
In Go 1.22 and later, the loop variable lifetimes changed to create a new
iteration variable per loop iteration. (See go.dev/issue/60078.)

In this example, all the deferred functions run after the loop has
completed, so all observe the final value of v.
completed, so all observe the final value of v [<go1.22].

for _, v := range list {
defer func() {
Expand All @@ -294,7 +298,10 @@ One fix is to create a new variable for each iteration of the loop:
}()
}

The next example uses a go statement and has a similar problem.
After Go version 1.22, the previous two for loops are equivalent
and both are correct.

The next example uses a go statement and has a similar problem [<go1.22].
In addition, it has a data race because the loop updates v
concurrent with the goroutines accessing it.

Expand All @@ -318,7 +325,7 @@ A hard-to-spot variant of this form is common in parallel tests:
}

The t.Parallel() call causes the rest of the function to execute
concurrent with the loop.
concurrent with the loop [<go1.22].

The analyzer reports references only in the last statement,
as it is not deep enough to understand the effects of subsequent
Expand Down
4 changes: 2 additions & 2 deletions gopls/internal/lsp/source/api_json.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit cd62b43

Please sign in to comment.