From 160d3bef43b386e6a3db8990ad78c3177c56fc3a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 9 Mar 2024 18:37:00 +0100 Subject: go.mod: Upgrade to golang.org/x/tools v0.15.0 The version previously declared caused a panic when running under Go 1.22: $ go1.22.0 test -v === RUN Test panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102ffd84c] goroutine 92 [running]: go/types.(*Checker).handleBailout(0x140002e0000, 0x140002ddb68) /tmp/go/src/go/types/check.go:339 +0x9c panic({0x103110f80?, 0x1032d4670?}) /tmp/go/src/runtime/panic.go:765 +0x124 go/types.(*StdSizes).Sizeof(0x0, {0x10315b768?, 0x1032d56a0}) /tmp/go/src/go/types/sizes.go:228 +0x2fc go/types.(*Config).sizeof(...) /tmp/go/src/go/types/sizes.go:331 go/types.representableConst.func1({0x10315b768?, 0x1032d56a0?}) /tmp/go/src/go/types/const.go:76 +0x9c go/types.representableConst({0x10315ca48, 0x1032cbe00}, 0x140002e0000, 0x1032d56a0, 0x0) /tmp/go/src/go/types/const.go:92 +0x138 go/types.(*Checker).arrayLength(0x140002e0000, {0x10315bb80?, 0x140002b2160?}) /tmp/go/src/go/types/typexpr.go:504 +0x23c go/types.(*Checker).typInternal(0x140002e0000, {0x10315bb50?, 0x140002ac090?}, 0x1033453c8?) /tmp/go/src/go/types/typexpr.go:299 +0x934 go/types.(*Checker).definedType(0x140002b23e0?, {0x10315bb50?, 0x140002ac090}, 0x102ff9fd8?) /tmp/go/src/go/types/typexpr.go:180 +0x30 go/types.(*Checker).varType(0x140002e0000, {0x10315bb50?, 0x140002ac090}) /tmp/go/src/go/types/typexpr.go:145 +0x30 go/types.(*Checker).structType(0x140002e0000, 0x140002ac3c0, 0x140002ac3c0?) /tmp/go/src/go/types/struct.go:113 +0x128 go/types.(*Checker).typInternal(0x140002e0000, {0x10315bdc0?, 0x14000290468?}, 0x102e87278?) /tmp/go/src/go/types/typexpr.go:316 +0xb14 go/types.(*Checker).definedType(0x0?, {0x10315bdc0?, 0x14000290468}, 0x102e3c294?) /tmp/go/src/go/types/typexpr.go:180 +0x30 go/types.(*Checker).typeDecl(0x140002e0000, 0x140002d4370, 0x140002c2040, 0x778?) /tmp/go/src/go/types/decl.go:595 +0x544 go/types.(*Checker).objDecl(0x140002e0000, {0x10315df58, 0x140002d4370}, 0x102fc77b8?) /tmp/go/src/go/types/decl.go:197 +0x7d0 go/types.(*Checker).packageObjects(0x140002e0000) /tmp/go/src/go/types/resolver.go:675 +0x350 go/types.(*Checker).checkFiles(0x140002e0000, {0x140002ae000, 0x1, 0x1}) /tmp/go/src/go/types/check.go:387 +0x1d8 go/types.(*Checker).Files(...) /tmp/go/src/go/types/check.go:344 golang.org/x/tools/go/packages.(*loader).loadPackage(0x140001501c0, 0x1400016b1a0) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:1052 +0x97c golang.org/x/tools/go/packages.(*loader).loadRecursive.func1() ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:851 +0x178 sync.(*Once).doSlow(0x0?, 0x0?) /tmp/go/src/sync/once.go:74 +0x100 sync.(*Once).Do(...) /tmp/go/src/sync/once.go:65 golang.org/x/tools/go/packages.(*loader).loadRecursive(0x0?, 0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:839 +0x50 golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:846 +0x30 created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1 in goroutine 40 ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:845 +0x84 panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102ffd84c] goroutine 65 [running]: go/types.(*Checker).handleBailout(0x140001c2000, 0x14000171b68) /tmp/go/src/go/types/check.go:339 +0x9c panic({0x103110f80?, 0x1032d4670?}) /tmp/go/src/runtime/panic.go:765 +0x124 go/types.(*StdSizes).Sizeof(0x0, {0x10315b768?, 0x1032d56a0}) /tmp/go/src/go/types/sizes.go:228 +0x2fc go/types.(*Config).sizeof(...) /tmp/go/src/go/types/sizes.go:331 go/types.representableConst.func1({0x10315b768?, 0x1032d56a0?}) /tmp/go/src/go/types/const.go:76 +0x9c go/types.representableConst({0x10315ca48, 0x1032cbd80}, 0x140001c2000, 0x1032d56a0, 0x1400016fec8) /tmp/go/src/go/types/const.go:92 +0x138 go/types.(*Checker).representation(0x10315b768?, 0x140001b0ec0, 0x1400016ff48?) /tmp/go/src/go/types/const.go:256 +0x68 go/types.(*Checker).implicitTypeAndValue(0x140001c2000, 0x140001b0ec0, {0x10315b768?, 0x1032d56a0?}) /tmp/go/src/go/types/expr.go:375 +0x2e8 go/types.(*Checker).convertUntyped(0x140001c2000, 0x140001b0ec0, {0x10315b768, 0x1032d56a0}) /tmp/go/src/go/types/const.go:289 +0x30 go/types.(*Checker).matchTypes(0x14000170698?, 0x140001b0e80, 0x140001b0ec0) /tmp/go/src/go/types/expr.go:926 +0x7c go/types.(*Checker).binary(0x14000170818?, 0x140001b0e80, {0x10315bf40?, 0x140001a20f0}, {0x10315ba90?, 0x140001a8100}, {0x10315bb80?, 0x140001a8120}, 0x28, 0x102e6da60?) /tmp/go/src/go/types/expr.go:800 +0x128 go/types.(*Checker).exprInternal(0x140001c2000, {0x0, 0x0}, 0x140001b0e80, {0x10315bf40, 0x140001a20f0?}, {0x0?, 0x0?}) /tmp/go/src/go/types/expr.go:1401 +0x1634 go/types.(*Checker).rawExpr(0x140001c2000, {0x0, 0x0}, 0x140001b0e80, {0x10315bf40?, 0x140001a20f0?}, {0x0?, 0x0?}, 0x0) /tmp/go/src/go/types/expr.go:965 +0x134 go/types.(*Checker).expr(0x140001c2000?, {0x0?, 0x0?}, 0x103076b67?, {0x10315bf40?, 0x140001a20f0?}) /tmp/go/src/go/types/expr.go:1498 +0x40 go/types.(*Checker).stmt(0x140001c2000, 0x0, {0x10315c270?, 0x140001b0140?}) /tmp/go/src/go/types/stmt.go:574 +0x1314 go/types.(*Checker).stmtList(0x140002c78e8?, 0x0, {0x140001a8280?, 0x0?, 0x0?}) /tmp/go/src/go/types/stmt.go:125 +0x88 go/types.(*Checker).funcBody(0x140001c2000, 0x14000196720, {0x14000198068?, 0x10315e1d8?}, 0x140001b0bc0, 0x140001a2180, {0x0, 0x0}) /tmp/go/src/go/types/stmt.go:45 +0x244 go/types.(*Checker).funcDecl.func1() /tmp/go/src/go/types/decl.go:826 +0x44 go/types.(*Checker).processDelayed(0x140001c2000, 0x0) /tmp/go/src/go/types/check.go:446 +0x12c go/types.(*Checker).checkFiles(0x140001c2000, {0x140001a4000, 0x1, 0x1}) /tmp/go/src/go/types/check.go:390 +0x1fc go/types.(*Checker).Files(...) /tmp/go/src/go/types/check.go:344 golang.org/x/tools/go/packages.(*loader).loadPackage(0x140001501c0, 0x1400016b2f0) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:1052 +0x97c golang.org/x/tools/go/packages.(*loader).loadRecursive.func1() ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:851 +0x178 sync.(*Once).doSlow(0x0?, 0x0?) /tmp/go/src/sync/once.go:74 +0x100 sync.(*Once).Do(...) /tmp/go/src/sync/once.go:65 golang.org/x/tools/go/packages.(*loader).loadRecursive(0x0?, 0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:839 +0x50 golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:846 +0x30 created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1 in goroutine 61 ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:845 +0x84 exit status 2 FAIL gopkg.teddywing.com/capturedrefrace 0.140s I tried upgrading to the latest version of golang.org/x/tools, v0.19.0, but this caused the following error while bisecting the Go compiler: $ /tmp/go/bin/go test -v # golang.org/x/tools/internal/aliases ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/aliases/aliases_go122.go:21:20: undefined: types.Alias ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/aliases/aliases_go122.go:24:54: undefined: types.Unalias ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/aliases/aliases_go122.go:30:13: undefined: types.NewAlias ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/aliases/aliases_go122.go:66:67: undefined: types.Alias # golang.org/x/tools/internal/versions ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/versions/types_go122.go:30:15: info.FileVersions undefined (type *types.Info has no field or method FileVersions) ...go/pkg/mod/golang.org/x/tools@v0.19.0/internal/versions/types_go122.go:40:7: info.FileVersions undefined (type *types.Info has no field or method FileVersions) FAIL gopkg.teddywing.com/capturedrefrace [build failed] The error was caused by the addition of special `types.Alias` handling for Go 1.21 and Go 1.22 (https://github.com/golang/tools/commit/0be034b1e193e98221abc05e710b8ecbf8cc9d45). The earliest tagged version of golang.org/x/tools that I could find without the `internal/aliases` package was v0.15.0, so use that to bisect the Go codebase. --- go.mod | 6 +++--- go.sum | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index 5d08fa1..f1d1907 100644 --- a/go.mod +++ b/go.mod @@ -2,9 +2,9 @@ module gopkg.teddywing.com/capturedrefrace go 1.20 -require golang.org/x/tools v0.9.1 +require golang.org/x/tools v0.15.0 require ( - golang.org/x/mod v0.10.0 // indirect - golang.org/x/sys v0.8.0 // indirect + golang.org/x/mod v0.16.0 // indirect + golang.org/x/sys v0.14.0 // indirect ) diff --git a/go.sum b/go.sum index cf10794..66bbf33 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,7 @@ -golang.org/x/mod v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk= -golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= -golang.org/x/sync v0.2.0 h1:PUR+T4wwASmuSTYdKjYHI5TD22Wy5ogLU5qZCOLxBrI= -golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU= -golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/tools v0.9.1 h1:8WMNJAz3zrtPmnYC7ISf5dEn3MT0gY7jBJfw27yrrLo= -golang.org/x/tools v0.9.1/go.mod h1:owI94Op576fPu3cIGQeHs3joujW/2Oc6MtlxbF5dfNc= +golang.org/x/mod v0.16.0 h1:QX4fJ0Rr5cPQCF7O9lh9Se4pmwfwskqZfq5moyldzic= +golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE= +golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q= +golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/tools v0.15.0 h1:zdAyfUGbYmuVokhzVmghFl2ZJh5QhcfebBgmVPFYA+8= +golang.org/x/tools v0.15.0/go.mod h1:hpksKq4dtpQWS1uQ61JkdqWM3LscIS6Slf+VVkm+wQk= -- cgit v1.2.3 From bf64f32a1a8099ee45d66796b5f60f9ae10a2436 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 9 Mar 2024 18:45:48 +0100 Subject: Debug broken behaviour caused by Go 1.22 The Go 1.22 release notes mention in passing: > The start position (Pos) of the lexical environment block (Scope) that > represents a function body has changed: it used to start at the > opening curly brace of the function body, but now starts at the > function's func token. (https://go.dev/doc/go1.22#go/types) That looked like what was causing the breakage in Capturedrefrace. I started by trying to compare the differences between Go 1.21 and Go 1.22: $ go install golang.org/dl/go1.22.0@latest $ go1.22.0 download $ go install golang.org/dl/go1.21.8@latest $ go1.21.8 download $ go1.22.0 test === RUN Test analysistest.go:522: gocapturedrefrace/testdata/simple.go:29:10: unexpected diagnostic: captured reference copied in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:57:10: unexpected diagnostic: captured reference s in goroutine closure --- FAIL: Test (0.40s) FAIL exit status 1 FAIL gopkg.teddywing.com/capturedrefrace 0.410s $ go1.21.8 test -v === RUN Test --- PASS: Test (0.38s) PASS ok gopkg.teddywing.com/capturedrefrace 0.389s Printing different AST values showed differences, but it was difficult to see what was going on. What was clear was that the problem came from the `if funcScope != scope` line. Since it was clear the problem came from the Go standard library rather than the golang.org/x/tools package, I tried bisecting Go (with some guidance from https://scribe.rip/@fzambia/bisecting-go-performance-degradation-4d4a7ee83a63): $ git clone https://github.com/golang/go.git $ git checkout go1.22.0 $ git bisect start $ git bisect good go1.21.8 $ git bisect bad $ cd src/ $ ./make.bash ... And in this repository: $ /tmp/go/bin/go test -v That turned up this commit: https://github.com/golang/go/commit/a27a525d1b4df74989ac9f6ad10394391fe3eb88. Now I know what's causing the problem, and that the change isn't likely to be reverted because the issues it references describe it as a bug. Next, I'll have to work out how to work around the change in the analyzer. --- capturedrefrace.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/capturedrefrace.go b/capturedrefrace.go index c0bc646..52f7c34 100644 --- a/capturedrefrace.go +++ b/capturedrefrace.go @@ -51,6 +51,7 @@ package capturedrefrace import ( + "fmt" "go/ast" "go/token" "go/types" @@ -179,6 +180,8 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { ast.Inspect( funcLit, func(node ast.Node) bool { + // fmt.Printf("node: %#v\n", node) + ident, ok := node.(*ast.Ident) if !ok { return true @@ -197,6 +200,7 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { // Find out whether `ident` was defined in an outer scope. scope, scopeObj := funcScope.LookupParent(ident.Name, ident.NamePos) + // fmt.Printf("ident: %#v\n\t%#v\n\t%#v\n", ident, scope, scopeObj) // Identifier is local to the closure. if scope == nil && scopeObj == nil { @@ -215,6 +219,15 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { return true } + if ident.Name == "copied" { + fmt.Printf("identifier: %#v, obj: %#v, decl: %#v\n", ident, ident.Obj, ident.Obj.Decl) + fmt.Printf("scope: %#v\n", scope) + fmt.Printf("funcScope: %#v\n", funcScope) + fmt.Println() + } + + // TODO: Broken by golang/go a27a525d1b4df74989ac9f6ad10394391fe3eb88 + // Test with golang.org/x/tools@v0.15.0 // Identifier was defined in a different scope. if funcScope != scope { pass.Reportf( -- cgit v1.2.3 From f923099c4302e49a1563183c5217e1e5cf017d85 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 9 Mar 2024 20:58:56 +0100 Subject: Try checking if the same variable in inner scope is in parent scope Can't figure out how to resolve the `funcScope != scope` problem in Go 1.22. Saw the `Names` method in the documentation and out of curiosity tried to see if that could be used as an alternate means of checking if a variable is declared in the parent scope. Turns out I get a bunch of false positives with that so I can't use it: $ go test -v === RUN Test identifier: &ast.Ident{NamePos:4069121, Name:"copied", Obj:(*ast.Object)(0x1400206b720)}, obj: &ast.Object{Kind:4, Name:"copied", Decl:(*ast.Field)(0x140067420c0), Data:interface {}(nil), Type:interface {}(nil)}, decl: &ast.Field{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident{(*ast.Ident)(0x1400215fea0)}, Type:(*ast.Ident)(0x1400215fec0), Tag:(*ast.BasicLit)(nil), Comment:(*ast.CommentGroup)(nil)} scope: &types.Scope{parent:(*types.Scope)(0x14002164960), children:[]*types.Scope{(*types.Scope)(0x14003b12d80)}, number:1, elems:map[string]types.Object{"capturedReference":(*types.Var)(0x14003b12c60), "capturedReference2":(*types.Var)(0x14003b12cc0), "copied":(*types.Var)(0x14003b12d20)}, pos:4069035, end:4069588, comment:"function", isFunc:true} Names: []string{"capturedReference", "capturedReference2", "copied"} funcScope: &types.Scope{parent:(*types.Scope)(0x14002165620), children:[]*types.Scope{(*types.Scope)(0x14003b12ea0)}, number:1, elems:map[string]types.Object{"copied":(*types.Var)(0x14003b12e40), "decl":(*types.Var)(0x14003b13020), "newVar":(*types.Var)(0x14003b12f60), "str":(*types.Var)(0x14003b12fc0)}, pos:4069116, end:4069578, comment:"function", isFunc:true} Names: []string{"copied", "decl", "newVar", "str"} identifier: &ast.Ident{NamePos:4069325, Name:"copied", Obj:(*ast.Object)(0x1400206b720)}, obj: &ast.Object{Kind:4, Name:"copied", Decl:(*ast.Field)(0x140067420c0), Data:interface {}(nil), Type:interface {}(nil)}, decl: &ast.Field{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident{(*ast.Ident)(0x1400215fea0)}, Type:(*ast.Ident)(0x1400215fec0), Tag:(*ast.BasicLit)(nil), Comment:(*ast.CommentGroup)(nil)} scope: &types.Scope{parent:(*types.Scope)(0x14002165620), children:[]*types.Scope{(*types.Scope)(0x14003b12ea0)}, number:1, elems:map[string]types.Object{"copied":(*types.Var)(0x14003b12e40), "decl":(*types.Var)(0x14003b13020), "newVar":(*types.Var)(0x14003b12f60), "str":(*types.Var)(0x14003b12fc0)}, pos:4069116, end:4069578, comment:"function", isFunc:true} Names: []string{"copied", "decl", "newVar", "str"} funcScope: &types.Scope{parent:(*types.Scope)(0x14002165620), children:[]*types.Scope{(*types.Scope)(0x14003b12ea0)}, number:1, elems:map[string]types.Object{"copied":(*types.Var)(0x14003b12e40), "decl":(*types.Var)(0x14003b13020), "newVar":(*types.Var)(0x14003b12f60), "str":(*types.Var)(0x14003b12fc0)}, pos:4069116, end:4069578, comment:"function", isFunc:true} Names: []string{"copied", "decl", "newVar", "str"} analysistest.go:522: gocapturedrefrace/testdata/shadow.go:38:6: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:39:14: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:47:3: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:48:6: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:49:14: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:66:3: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:67:3: unexpected diagnostic: captured reference err2 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:68:6: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:68:21: unexpected diagnostic: captured reference err2 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:69:14: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:69:20: unexpected diagnostic: captured reference err2 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:29:10: unexpected diagnostic: captured reference copied in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:32:3: unexpected diagnostic: captured reference copied in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:39:3: unexpected diagnostic: captured reference newVar in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:42:18: unexpected diagnostic: captured reference str in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:45:3: unexpected diagnostic: captured reference decl in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:46:18: unexpected diagnostic: captured reference decl in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:57:10: unexpected diagnostic: captured reference s in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/simple.go:58:3: unexpected diagnostic: captured reference s in goroutine closure --- FAIL: Test (0.40s) FAIL exit status 1 FAIL gopkg.teddywing.com/capturedrefrace 0.414s --- capturedrefrace.go | 27 +++++++++++++++++++++++++-- go.mod | 7 +++++-- go.sum | 5 +++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/capturedrefrace.go b/capturedrefrace.go index 52f7c34..e155470 100644 --- a/capturedrefrace.go +++ b/capturedrefrace.go @@ -56,6 +56,7 @@ import ( "go/token" "go/types" + "golang.org/x/exp/slices" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" @@ -221,14 +222,36 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { if ident.Name == "copied" { fmt.Printf("identifier: %#v, obj: %#v, decl: %#v\n", ident, ident.Obj, ident.Obj.Decl) - fmt.Printf("scope: %#v\n", scope) - fmt.Printf("funcScope: %#v\n", funcScope) + fmt.Printf(" scope: %#v\n", scope) + fmt.Printf(" Names: %#v\n", scope.Names()) + fmt.Printf(" funcScope: %#v\n", funcScope) + fmt.Printf(" Names: %#v\n", funcScope.Names()) fmt.Println() } + za := slices.Index(funcScope.Names(), ident.Name) + // if za == -1 { + // return true + // } + zb := slices.Index(scope.Names(), ident.Name) + // if zb == -1 { + // return true + // } + // TODO: Broken by golang/go a27a525d1b4df74989ac9f6ad10394391fe3eb88 // Test with golang.org/x/tools@v0.15.0 // Identifier was defined in a different scope. + // if funcScope != scope { + if za != -1 && zb != -1 && funcScope.Names()[za] == scope.Names()[zb] { + pass.Reportf( + ident.Pos(), + "captured reference %s in goroutine closure", + ident, + ) + + return true + } + if funcScope != scope { pass.Reportf( ident.Pos(), diff --git a/go.mod b/go.mod index f1d1907..58d3aba 100644 --- a/go.mod +++ b/go.mod @@ -2,9 +2,12 @@ module gopkg.teddywing.com/capturedrefrace go 1.20 -require golang.org/x/tools v0.15.0 +require ( + golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 + golang.org/x/tools v0.18.0 +) require ( golang.org/x/mod v0.16.0 // indirect - golang.org/x/sys v0.14.0 // indirect + golang.org/x/sys v0.17.0 // indirect ) diff --git a/go.sum b/go.sum index 66bbf33..6659cbc 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,12 @@ +golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 h1:LfspQV/FYTatPTr/3HzIcmiUFH7PGP+OQ6mgDYo3yuQ= +golang.org/x/exp v0.0.0-20240222234643-814bf88cf225/go.mod h1:CxmFvTBINI24O/j8iY7H1xHzx2i4OsyguNBmN/uPtqc= golang.org/x/mod v0.16.0 h1:QX4fJ0Rr5cPQCF7O9lh9Se4pmwfwskqZfq5moyldzic= golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE= golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q= golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/tools v0.15.0 h1:zdAyfUGbYmuVokhzVmghFl2ZJh5QhcfebBgmVPFYA+8= golang.org/x/tools v0.15.0/go.mod h1:hpksKq4dtpQWS1uQ61JkdqWM3LscIS6Slf+VVkm+wQk= +golang.org/x/tools v0.18.0 h1:k8NLag8AGHnn+PHbl7g43CtqZAwG60vZkLqgyZgIHgQ= +golang.org/x/tools v0.18.0/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= -- cgit v1.2.3 From b1b311caf11e9e88ef5735dc4e5512dcd4725059 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 9 Mar 2024 21:20:32 +0100 Subject: Fiddle with `pos` in `funcScope.LookupParent` for Go 1.22 breakage First I tried changing `funcScope.LookupParent` to use the starting brace ("{") of the function, but that didn't work for redeclarations: $ go test -v analysistest.go:522: gocapturedrefrace/testdata/shadow.go:38:6: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:39:14: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:47:3: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:48:6: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:49:14: unexpected diagnostic: captured reference err in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:66:3: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:67:3: unexpected diagnostic: captured reference err2 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:68:6: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:68:21: unexpected diagnostic: captured reference err2 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:69:14: unexpected diagnostic: captured reference err1 in goroutine closure analysistest.go:522: gocapturedrefrace/testdata/shadow.go:69:20: unexpected diagnostic: captured reference err2 in goroutine closure --- FAIL: Test (0.43s) The commit that breaks the analyzer says: > Previously, its value was unset (NoPos), but the correct > value is a point after the signature (FuncType.End) and > before the body. (https://github.com/golang/go/commit/a27a525d1b4df74989ac9f6ad10394391fe3eb88) Recalling that, I tried setting the position to `token.NoPos`, which fixes the analyser and allows the tests to pass. --- capturedrefrace.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/capturedrefrace.go b/capturedrefrace.go index e155470..6b5b1df 100644 --- a/capturedrefrace.go +++ b/capturedrefrace.go @@ -56,7 +56,6 @@ import ( "go/token" "go/types" - "golang.org/x/exp/slices" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" @@ -199,8 +198,12 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { } } + // Idea: If ident is in funcLit signature, then short-circuit. + // Find out whether `ident` was defined in an outer scope. - scope, scopeObj := funcScope.LookupParent(ident.Name, ident.NamePos) + // scope, scopeObj := funcScope.LookupParent(ident.Name, ident.NamePos) + // scope, scopeObj := funcScope.LookupParent(ident.Name, funcLit.Body.Lbrace) + scope, scopeObj := funcScope.LookupParent(ident.Name, token.NoPos) // fmt.Printf("ident: %#v\n\t%#v\n\t%#v\n", ident, scope, scopeObj) // Identifier is local to the closure. @@ -229,11 +232,11 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { fmt.Println() } - za := slices.Index(funcScope.Names(), ident.Name) + // za := slices.Index(funcScope.Names(), ident.Name) // if za == -1 { // return true // } - zb := slices.Index(scope.Names(), ident.Name) + // zb := slices.Index(scope.Names(), ident.Name) // if zb == -1 { // return true // } @@ -242,16 +245,6 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { // Test with golang.org/x/tools@v0.15.0 // Identifier was defined in a different scope. // if funcScope != scope { - if za != -1 && zb != -1 && funcScope.Names()[za] == scope.Names()[zb] { - pass.Reportf( - ident.Pos(), - "captured reference %s in goroutine closure", - ident, - ) - - return true - } - if funcScope != scope { pass.Reportf( ident.Pos(), -- cgit v1.2.3 From ba664a54c0e275c01709e393030aeeae607d0656 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sun, 10 Mar 2024 15:28:54 +0100 Subject: go.mod: Upgrade 'golang.org/x/tools' to latest v0.19.0 We were previously using golang.org/x/tools v0.9.1, which caused a segfault when running against the latest Go 1.22: $ go test panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x100862f14] goroutine 76 [running]: go/types.(*Checker).handleBailout(0x14000184200, 0x14000639b98) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/check.go:367 +0x9c panic({0x10097d640?, 0x100b50600?}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/runtime/panic.go:770 +0x124 go/types.(*StdSizes).Sizeof(0x0, {0x1009c9d28, 0x100b539a0}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/sizes.go:228 +0x314 go/types.(*Config).sizeof(...) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/sizes.go:333 go/types.representableConst.func1({0x1009c9d28?, 0x100b539a0?}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/const.go:76 +0x9c go/types.representableConst({0x1009cb030, 0x100b47d80}, 0x14000184200, 0x100b539a0, 0x0) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/const.go:92 +0x138 go/types.(*Checker).arrayLength(0x14000184200, {0x1009ca168, 0x14000226b20?}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:510 +0x238 go/types.(*Checker).typInternal(0x14000184200, {0x1009ca138, 0x140001a42a0}, 0x0) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:299 +0x3bc go/types.(*Checker).definedType(0x14000184200, {0x1009ca138, 0x140001a42a0}, 0x14000639158?) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:180 +0x2c go/types.(*Checker).varType(0x14000184200, {0x1009ca138, 0x140001a42a0}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:145 +0x2c go/types.(*Checker).structType(0x14000184200, 0x140001a49f0, 0x140001a49f0?) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/struct.go:113 +0x128 go/types.(*Checker).typInternal(0x14000184200, {0x1009ca3a8, 0x140001145b8}, 0x140001597c0) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:316 +0xed0 go/types.(*Checker).definedType(0x14000184200, {0x1009ca3a8, 0x140001145b8}, 0x100690214?) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/typexpr.go:180 +0x2c go/types.(*Checker).typeDecl(0x14000184200, 0x140001597c0, 0x1400022e8c0, 0x0) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/decl.go:615 +0x39c go/types.(*Checker).objDecl(0x14000184200, {0x1009cd298, 0x140001597c0}, 0x0) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/decl.go:197 +0x880 go/types.(*Checker).packageObjects(0x14000184200) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/resolver.go:681 +0x3c0 go/types.(*Checker).checkFiles(0x14000184200, {0x1400061e2e8, 0x1, 0x1}) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/check.go:408 +0x164 go/types.(*Checker).Files(...) /opt/homebrew/Cellar/go/1.22.1/libexec/src/go/types/check.go:372 golang.org/x/tools/go/packages.(*loader).loadPackage(0x140001881c0, 0x14000011110) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:1052 +0x870 golang.org/x/tools/go/packages.(*loader).loadRecursive.func1() ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:851 +0x178 sync.(*Once).doSlow(0x0?, 0x0?) /opt/homebrew/Cellar/go/1.22.1/libexec/src/sync/once.go:74 +0x100 sync.(*Once).Do(...) /opt/homebrew/Cellar/go/1.22.1/libexec/src/sync/once.go:65 golang.org/x/tools/go/packages.(*loader).loadRecursive(0x0?, 0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:839 +0x50 golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0x0?) ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:846 +0x30 created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1 in goroutine 67 ...go/pkg/mod/golang.org/x/tools@v0.9.1/go/packages/packages.go:845 +0x84 exit status 2 FAIL gopkg.teddywing.com/capturedrefrace 0.129s Upgrade to the latest version of the library, which doesn't cause the problem. --- go.mod | 4 ++-- go.sum | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 58d3aba..3225857 100644 --- a/go.mod +++ b/go.mod @@ -4,10 +4,10 @@ go 1.20 require ( golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 - golang.org/x/tools v0.18.0 + golang.org/x/tools v0.19.0 ) require ( golang.org/x/mod v0.16.0 // indirect - golang.org/x/sys v0.17.0 // indirect + golang.org/x/sys v0.18.0 // indirect ) diff --git a/go.sum b/go.sum index 6659cbc..e31f719 100644 --- a/go.sum +++ b/go.sum @@ -6,7 +6,10 @@ golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE= golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q= golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/tools v0.15.0 h1:zdAyfUGbYmuVokhzVmghFl2ZJh5QhcfebBgmVPFYA+8= golang.org/x/tools v0.15.0/go.mod h1:hpksKq4dtpQWS1uQ61JkdqWM3LscIS6Slf+VVkm+wQk= golang.org/x/tools v0.18.0 h1:k8NLag8AGHnn+PHbl7g43CtqZAwG60vZkLqgyZgIHgQ= golang.org/x/tools v0.18.0/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg= +golang.org/x/tools v0.19.0 h1:tfGCXNR1OsFG+sVdLAitlpjAvD/I6dHDKnYrpEZUHkw= +golang.org/x/tools v0.19.0/go.mod h1:qoJWxmGSIBmAeriMx19ogtrEPrGtDbPK634QFIcLAhc= -- cgit v1.2.3 From 1926d13444dbde1bcd221029ec44bd6bd30396a7 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sun, 10 Mar 2024 16:10:36 +0100 Subject: Clean up Go 1.22 tests and fix broken analysis under Go 1.22 * Remove my debugging tests from trying to understand the Go 1.22 problem. * Describe the problem caused by the change in 'go/types' starting in Go 1.22.0. The problem was caused by https://github.com/golang/go/commit/a27a525d1b4df74989ac9f6ad10394391fe3eb88, which resolved the following two issues related to 'gopls': * https://github.com/golang/go/issues/64292 * https://github.com/golang/go/issues/64295 --- capturedrefrace.go | 61 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/capturedrefrace.go b/capturedrefrace.go index 6b5b1df..66b59e5 100644 --- a/capturedrefrace.go +++ b/capturedrefrace.go @@ -51,7 +51,6 @@ package capturedrefrace import ( - "fmt" "go/ast" "go/token" "go/types" @@ -180,8 +179,6 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { ast.Inspect( funcLit, func(node ast.Node) bool { - // fmt.Printf("node: %#v\n", node) - ident, ok := node.(*ast.Ident) if !ok { return true @@ -198,13 +195,40 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { } } - // Idea: If ident is in funcLit signature, then short-circuit. - // Find out whether `ident` was defined in an outer scope. - // scope, scopeObj := funcScope.LookupParent(ident.Name, ident.NamePos) - // scope, scopeObj := funcScope.LookupParent(ident.Name, funcLit.Body.Lbrace) + // + // Starting in Go 1.22.0, function scopes in the 'go/types' package + // have changed: + // + // > The start position (Pos) of the lexical environment block + // > (Scope) that represents a function body has changed: it used + // > to start at the opening curly brace of the function body, but + // > now starts at the function's func token. + // + // (https://go.dev/doc/go1.22#go/types) + // + // This was caused by the following commit, which corrected + // behaviour for gopls: + // + // https://github.com/golang/go/commit/a27a525d1b4df74989ac9f6ad10394391fe3eb88 + // + // As a result, we can no longer use `ident.NamePos` as the + // position for `funcScope.LookupParent`. Doing so causes false + // positives for function arguments, incorrectly finding them to be + // captured references from the outer scope. + // + // The commit message of a27a525d1b4df74989ac9f6ad10394391fe3eb88 + // states: + // + // > set correct Var.scopePos for parameters/results + // > + // > Previously, its value was unset (NoPos), but the correct + // > value is a point after the signature (FuncType.End) and + // > before the body. + // + // We can restore the previous behaviour by using `token.NoPos` + // instead of `ident.NamePos`. scope, scopeObj := funcScope.LookupParent(ident.Name, token.NoPos) - // fmt.Printf("ident: %#v\n\t%#v\n\t%#v\n", ident, scope, scopeObj) // Identifier is local to the closure. if scope == nil && scopeObj == nil { @@ -223,28 +247,7 @@ func checkClosure(pass *analysis.Pass, funcLit *ast.FuncLit) { return true } - if ident.Name == "copied" { - fmt.Printf("identifier: %#v, obj: %#v, decl: %#v\n", ident, ident.Obj, ident.Obj.Decl) - fmt.Printf(" scope: %#v\n", scope) - fmt.Printf(" Names: %#v\n", scope.Names()) - fmt.Printf(" funcScope: %#v\n", funcScope) - fmt.Printf(" Names: %#v\n", funcScope.Names()) - fmt.Println() - } - - // za := slices.Index(funcScope.Names(), ident.Name) - // if za == -1 { - // return true - // } - // zb := slices.Index(scope.Names(), ident.Name) - // if zb == -1 { - // return true - // } - - // TODO: Broken by golang/go a27a525d1b4df74989ac9f6ad10394391fe3eb88 - // Test with golang.org/x/tools@v0.15.0 // Identifier was defined in a different scope. - // if funcScope != scope { if funcScope != scope { pass.Reportf( ident.Pos(), -- cgit v1.2.3