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.

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

SA4017 false positive #949

Closed
arashpayan opened this issue Mar 19, 2021 · 2 comments
Closed

SA4017 false positive #949

arashpayan opened this issue Mar 19, 2021 · 2 comments

Comments

@arashpayan
Copy link

@arashpayan arashpayan commented Mar 19, 2021

Firstly, thank you for building such an incredible tool! It's saved me from many bugs. :-)

I recently encountered a false positive while working on some code that I've reduced to the smallest reproducible example I could and attached to this issue.

  • The output of 'staticcheck -version'
    staticcheck 2020.2.3 (v0.1.3)

  • The output of 'staticcheck -debug.version' (it is fine if this command fails)

staticcheck 2020.2.3 (v0.1.3)

Compiled with Go version: go1.16.2
Main module:
	honnef.co/go/tools@v0.1.3 (sum: h1:qTakTkI6ni6LFD5sBwwsdSO+AQqbSIxOauHTTQKZ/7o=)
Dependencies:
	github.com/BurntSushi/toml@v0.3.1 (sum: h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=)
	golang.org/x/mod@v0.3.0 (sum: h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=)
	golang.org/x/sys@v0.0.0-20210119212857-b64e53b001e4 (sum: h1:myAQVi0cGEoqQVR5POX+8RR2mrocKqNN1hmeMqhX27k=)
	golang.org/x/tools@v0.1.0 (sum: h1:po9/4sTYwZU9lPhi1tOrb4hCv3qrhiQ77LZfGa2OjwY=)
	golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 (sum: h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=)
  • The output of 'go version'
    go version go1.16.2 linux/amd64

  • The output of 'go env'

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/<REDACTED>/.cache/go-build"
GOENV="/home/<REDACTED>/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/<REDACTED>/gocode/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/<REDACTED>/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/<REDACTED>/scheck/go.mod"
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-build199834044=/tmp/go-build -gno-record-gcc-switches"
  • Exactly which command you ran
    staticcheck ./...

  • Output of the command and what's wrong with the output
    app/app.go:21:10: Errorf is a pure function but its return value is ignored (SA4017)
    The return value from fmt.Errorf is used as the return value of the function. It's definitely being used.

  • Where we can read the code you're running staticcheck on
    I've attached a tiny project that reproduces the bug with the smallest example I could make. scheck.tar.gz

There seem to be 2 key elements to triggering the bug.

  1. The method must not be in the main package.
  2. If I remove the for loop at the end of the function, the error also disappears.

The example is based on a server that I'm writing, hence the non-terminating for loop.

In short, staticcheck has a problem with the following bit of code:

func Run() error {
	err := configEnvironment()
	if err != nil {
		return fmt.Errorf("failed to load config: %v", err)  // <-- FALSE POSITIVE HERE
	}
        // method continues

If I return err directly instead of wrapping it with fmt.Errorf() the error will also go away.

@arashpayan arashpayan added false-positive needs-triage Newly filed issue that needs triage labels Mar 19, 2021
@dominikh dominikh added started Issues we've started working on and removed needs-triage Newly filed issue that needs triage labels Mar 20, 2021
@dominikh
Copy link
Owner

@dominikh dominikh commented Mar 20, 2021

Thanks for reducing the problem to a minimal example. I can reproduce the issue and am looking into it.

@dominikh
Copy link
Owner

@dominikh dominikh commented Mar 20, 2021

This is fixed in master. I will make a new patch release containing the fix early next week.

@dominikh dominikh removed the started Issues we've started working on label Mar 20, 2021
dominikh added a commit that referenced this issue May 1, 2021
We correctly considered fake exits when building the (post-)dominator
tree, but forgot to do so when calculating the frontier. This would
lead to missing phi and sigma nodes, ultimately resulting in returning
wrong values.

Closes gh-949

(cherry picked from commit 390eb5b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants