go-tools icon indicating copy to clipboard operation
go-tools copied to clipboard

SA4006/SA4017: false positive in infinite loop

Open josharian opened this issue 1 year ago • 3 comments
trafficstars

These are false positives, I'm pretty sure.

$ staticcheck .
dialog.go:112:2: this value of start is never used (SA4006)
dialog.go:112:11: Now doesn't have side effects and its return value is ignored (SA4017)

Relevant code:

func (d *Dialog) LoopSound(ctx context.Context, wavPath string) (turn Turn, err error) {
	start := time.Now()
	for {
		turn, err := d.PlaySound(ctx, wavPath)
		if err != nil {
			return turn.WithElapsed(time.Since(start)), err
		}
		switch turn.Reason {
		case EndReasonContextExpired, EndReasonInterrupted:
			return turn.WithElapsed(time.Since(start)), nil
		}
	}
}

start is definitely used a few lines down.


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

Compiled with Go version: go1.22.1
Main module:
	honnef.co/go/[email protected] (sum: h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=)
Dependencies:
	github.com/BurntSushi/[email protected] (sum: h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=)
	golang.org/x/exp/[email protected] (sum: h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE=)
	golang.org/x/[email protected] (sum: h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=)
	golang.org/x/[email protected] (sum: h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM=)
	golang.org/x/[email protected] (sum: h1:Vk4mysSz+GqQK2eqgWbo4zEO89wkeAjJiFIr9bpqa8k=)
  • The output of 'go version'
go version go1.22.1 darwin/arm64
  • The output of 'go env'
GO111MODULE=''
GOARCH='arm64'
GOBIN='/Users/josh/bin'
GOCACHE='/Users/josh/Library/Caches/go-build'
GOENV='/Users/josh/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/josh/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/josh'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/josh/go/1.22'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/josh/go/1.22/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.1'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/vm/htvrhp4177v2dfhdjlvl0pqh0000gn/T/go-build1914124922=/tmp/go-build -gno-record-gcc-switches -fno-common'
  • Exactly which command you ran
staticcheck .

josharian avatar Mar 21 '24 23:03 josharian

Any chance that d.PlaySound panics unconditionally on the target you're building for?

dominikh avatar Mar 22 '24 00:03 dominikh

Hmmm. It may have done. (I've been writing and tweaking code since, I'm afraid.)

josharian avatar Mar 22 '24 01:03 josharian

With this:

package main

import (
	"context"
	"time"
)

func main() {}

type (
	Dialog struct{}
	Turn   struct{ Reason int }
)

const (
	EndReasonContextExpired = iota
	EndReasonInterrupted
)

func (t Turn) WithElapsed(time.Duration) Turn { return t }

func (d *Dialog) PlaySound(context.Context, string) (Turn, error) {
	//panic("oh noes")
	return Turn{}, nil
}

func (d *Dialog) LoopSound(ctx context.Context, wavPath string) (turn Turn, err error) {
	start := time.Now()
	for {
		turn, err := d.PlaySound(ctx, wavPath)
		if err != nil {
			return turn.WithElapsed(time.Since(start)), err
		}
		switch turn.Reason {
		case EndReasonContextExpired, EndReasonInterrupted:
			return turn.WithElapsed(time.Since(start)), nil
		}
	}
}

I get no errors:

% staticheck
[no error, exit 0]

However, if I uncomment that panic() I do indeed get:

% staticcheck
main.go:28:2: this value of start is never used (SA4006)
main.go:28:11: Now doesn't have side effects and its return value is ignored (SA4017)

Arguably that warning is accurate, although it's a bit confusing.

arp242 avatar May 26 '24 22:05 arp242