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/go/analysis/passes/lostcancel: incorrect warning about context leak using defer #58850

Closed
whereswaldon opened this issue Mar 3, 2023 · 3 comments
Closed

x/tools/go/analysis/passes/lostcancel: incorrect warning about context leak using defer #58850

whereswaldon opened this issue Mar 3, 2023 · 3 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@whereswaldon
Copy link
Contributor

gopls version

$ gopls -v version
Build info
----------
golang.org/x/tools/gopls v0.11.0
    golang.org/x/tools/gopls@(devel)
    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.4

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/chris/.cache/go-build"
GOENV="/home/chris/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/chris/Code/go/pkg/mod"
GONOPROXY="<redacted>"
GONOSUMDB="<redacted>"
GOOS="linux"
GOPATH="/home/chris/Code/go"
GOPRIVATE="<redacted>"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/chris/.local/lib/go-1.20.1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/chris/.local/lib/go-1.20.1/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="<redacted>"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3227410984=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Edit this source code:

package pkg

import "context"

func foo(parent context.Context, someChan chan int) {
	_, cancel := context.WithCancel(parent)
	defer func() {
		cancel()
	}()
	cancel()
	_, cancel = context.WithCancel(parent)
}

You will be warned that:

  • warning: this return statement may be reached without using the cancel var defined on line X
  • warning: the cancel function is not used on all paths (possible context leak)

However, it is evident that the cancel function is used on all paths. The analysis is not correctly understanding how the deferred cancellation closure will interact with the return.

I encountered this first on a more sophisticated example:

ctx, cancel := context.WithCancel(parent)
defer func() {
    cancel()
}()
// Possibly use ctx
for {
    select {
    case <-parent.Done():
            return
    case <-someChan:
        cancel()
        ctx, cancel = context.WithCancel(parent)
        // use ctx
    }
}

Thanks to @dominikh for helping me build to smaller reproducer.

What did you expect to see?

No warnings, as this code is correct and will cancel the context on all return paths.

What did you see instead?

Warnings.

Editor and settings

I use kakoune with kak-lsp running gopls. I use the default kak-lsp settings.

Logs

I suspect that this issue is not a result of editor interaction, but of an assumption within the analysis code, so my logs won't be of much help.
@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 Mar 3, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 3, 2023
@whereswaldon
Copy link
Contributor Author

This warning is also emitted by go vet, so it definitely isn't unique to gopls.

@hyangah hyangah changed the title x/tools/gopls: incorrect warning about context leak using defer x/tools/go/analysis/passes/lostcancel: incorrect warning about context leak using defer Mar 6, 2023
@hyangah
Copy link
Contributor

hyangah commented Mar 6, 2023

I think this is the same class of the false positives reported on lostcancel analyzer #25720
This analyzer isn't perfect. A workaround is to add _ = cancel or disable the analyzer from gopls settings (which I don't recommend).

@dominikh dominikh added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 6, 2023
@dominikh
Copy link
Member

dominikh commented Mar 6, 2023

I think this is the same class of the false positives reported on lostcancel analyzer

I think it's exactly the same false positive, in the same analyzer. I'll close this as a duplicate of #25720.

@dominikh dominikh closed this as completed Mar 6, 2023
@jiangsunan jiangsunan mentioned this issue Jun 27, 2023
Open
@golang golang locked and limited conversation to collaborators Mar 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. 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
@dominikh @whereswaldon @hyangah @gopherbot