Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: No warning "loop variable xx captured by func literal". #60016

Closed
tttoad opened this issue May 6, 2023 · 2 comments
Closed

x/tools/gopls: No warning "loop variable xx captured by func literal". #60016

tttoad opened this issue May 6, 2023 · 2 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@tttoad
Copy link

tttoad commented May 6, 2023

gopls version

golang.org/x/tools/gopls v0.11.0
golang.org/x/tools/gopls@v0.11.0 h1:/nvKHdTtePQmrv9XN3gIUN9MOdUrKzO/dcqgbG6x8EY=
github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
golang.org/x/exp@v0.0.0-20221031165847-c99f073a8326 h1:QfTh0HpN6hlw6D3vu8DAwC8pBIwikq0AI1evdm+FksE=
golang.org/x/exp/typeparams@v0.0.0-20221031165847-c99f073a8326 h1:fl8k2zg28yA23264d82M4dp+YlJ3ngDcpuB1bewkQi4=
golang.org/x/mod@v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=
golang.org/x/sync@v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
golang.org/x/sys@v0.2.0 h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A=
golang.org/x/text@v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
golang.org/x/tools@v0.3.1-0.20221213193459-ca17b2c27ca8 h1:7/HkGkN/2ktghBCSRRgp31wAww4syfsW52tj7yirjWk=
golang.org/x/vuln@v0.0.0-20221109205719-3af8368ee4fe h1:qptQiQwEpETwDiz85LKtChqif9xhVkAm8Nhxs0xnTww=
honnef.co/go/tools@v0.3.3 h1:oDx7VAwstgpYpb3wv0oxiZlxY+foCpRAwY7Vk6XpAgA=
mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.19

go env

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/toad/Library/Caches/go-build"
GOENV="/Users/toad/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/toad/go/go1.19/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/toad/go/go1.19"
GOPRIVATE=""
GOPROXY="https://goproxy.cn,direct"
GOROOT="/opt/homebrew/Cellar/go/1.19/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.19/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/toad/work/gopls-demo/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/f
olders/g1/tgmnlrdn3vxgv08kdgh9vpkw0000gn/T/go-build537849748=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"fmt"
)

func main() {
	al := make([]string, 0)
	al = append(al, "123", "456")

	for _, a := range al {
		go func() {
			fmt.Println(a)
		}()

		go func(a string) {
			fmt.Println(a)
		}(a)
	}
}

What did you expect to see?

in line 13: warning, loop variable a captured by func literal.

What did you see instead?

2023-05-06.10.18.06.mov
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels May 6, 2023
@gopherbot gopherbot added this to the Unreleased milestone May 6, 2023
@findleyr
Copy link
Contributor

findleyr commented May 8, 2023

This is working as intended. The loopclosure analyzer only catches references in go statements that are the last statement in a loop body. Otherwise, the goroutine could be synchronized by a number of means, causing the analyzer to produce false positives.

Knowing whether or not the loop value escapes the loop iteration is a surprisingly tricky problem.

Note that there is discussion about changing loop variable semantics in #56010.

CC @adonovan @timothy-king

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale May 8, 2023
@timothy-king
Copy link
Contributor

This specific example can be caught by an alternative definition of "last statements". Here is an example WiP that IIRC can detect this specific example: https://go-review.git.corp.google.com/c/tools/+/455195 . The 'lastness' is modified 'statement must be able to reach the end of the loop'. These definitions are unfortunately a bit hard to predict. It still does not take much to stop such analyzers. Any expression that may panic or function call stops going backwards.

func foo() {
	...
	for _, a := range al {
		go func() {
			fmt.Println(a)
		}()
		extraCall() // extra function call that can panic or potentially synchronize
		go func(a string) {
			fmt.Println(a)
		}(a)
	}
}

Neither syntactic definition ends up entirely satisfying.

Efforts in this direction may end up somewhat moot due to #56010.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@timothy-king @gopherbot @tttoad @findleyr