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

cmd/vet: warn when return value of context.WithValue is lost #41149

Closed
ancarda opened this issue Aug 31, 2020 · 6 comments
Closed

cmd/vet: warn when return value of context.WithValue is lost #41149

ancarda opened this issue Aug 31, 2020 · 6 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@ancarda
Copy link

ancarda commented Aug 31, 2020

What version of Go are you using (go version)?

go version go1.14.7 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

Not Applicable, but anyway...

go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ancarda/.cache/go-build"
GOENV="/home/ancarda/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ancarda/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build172081220=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/NY8BWaciqad

package main

import (
	"context"
	"fmt"
)

func f(ctx context.Context) {
	context.WithValue(ctx, "foo", -6)
}

func main() {
	ctx := context.TODO()
	f(ctx)
	fmt.Println(ctx.Value("foo"))
}

What did you expect to see?

An error indicating that line 9 has a bug; that is, WithContext returns a new context but is not returned. Perhaps something like:

./main.go:9:2: Context.WithValue returns a new instance, which is lost.

What did you see instead?

When I run go vet on this code, I don't get any errors.
@dmitshur dmitshur added FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 1, 2020
@dmitshur dmitshur added this to the Backlog milestone Sep 1, 2020
@dmitshur dmitshur changed the title cmd/vet: Warn when return value of context.WithValue is lost cmd/vet: warn when return value of context.WithValue is lost Sep 1, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2020

/cc @alandonovan per owners.

It may be relevant that according to this tweet and issue dominikh/go-tools#819, this check is planned to be added to staticcheck.

Also /cc @davecheney @Sajmani @dominikh FYI.

@alandonovan
Copy link
Contributor

Add context.WithValue to this list?
https://github.com/golang/tools/blob/master/go/analysis/passes/unusedresult/unusedresult.go#L47

@ancarda ancarda mentioned this issue Sep 1, 2020
Closed
@ancarda
Copy link
Author

ancarda commented Sep 1, 2020

Thanks @alandonovan, I've filed a PR here: golang/tools#248 🙂

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/252057 mentions this issue: unusedresult.go: Include context.With* functions

@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Sep 1, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2020

The x/tools commit will need to be vendored into the main repository so that cmd/vet picks it up. See CL 236138 for a past example.

Reopening for that.

@dmitshur dmitshur reopened this Sep 1, 2020
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 1, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/252941 mentions this issue: cmd: update golang.org/x/tools to v0.0.0-20200901153117-6e59e24738da

@golang golang locked and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

Successfully merging a pull request may close this issue.

unusedresult.go: Include context.With* functions
4 participants
@dmitshur @ancarda @alandonovan @gopherbot