Skip to content
This repository has been archived by the owner on Jul 31, 2022. It is now read-only.

false positive #2

Closed
sni opened this issue Jan 26, 2021 · 6 comments
Closed

false positive #2

sni opened this issue Jan 26, 2021 · 6 comments

Comments

@sni
Copy link

sni commented Jan 26, 2021

ifshort complains about a waitgroup beeing used only once but in fact it is used multiple times afterwards:

lmd/listener.go:208:2: variable 'wg' is only used in the if-statement (lmd/listener.go:222:2); consider using short syntax (ifshort)
        wg := &sync.WaitGroup{}
func SendCommands(commandsByPeer map[string][]string) (code int, msg string) {
	code = 200
	msg = "OK"
	resultChan := make(chan error, len(commandsByPeer))
	wg := &sync.WaitGroup{}
	for pID := range commandsByPeer {
		PeerMapLock.RLock()
		p := PeerMap[pID]
		PeerMapLock.RUnlock()
		wg.Add(1)
		go func(peer *Peer) {
			defer logPanicExitPeer(peer)
			defer wg.Done()
			resultChan <- peer.SendCommandsWithRetry(commandsByPeer[peer.ID])
		}(p)
	}

	if waitTimeout(wg, PeerCommandTimeout) {
		code = 202
		msg = "sending command timed out but will continue in background"
		return
	}
...
}

Original code can be found here:
https://github.com/sni/lmd/blob/f6eefbe1af8012d29998e122441c3c705d648a3d/lmd/listener.go#L204

sni added a commit to sni/ifshort that referenced this issue Jan 26, 2021
esimonov added a commit that referenced this issue Jan 26, 2021
@esimonov
Copy link
Owner

Hello @sni and thank you for your interest!
I appreciate your effort of submitting a PR with a new test case, but as it turned out, this false positive warning has nothing specific to do with waitgroups; it was caused by the assignment to a pointer, which cannot be shortened. Therefore, this test case should be sufficient to cover your use case - f4c7e24#diff-57ce4993559e6d59201b14d9309889cf5ce53e1d7e3107683fa30a1a25782a49

@sni
Copy link
Author

sni commented Jan 26, 2021

no worries, just wanted to make it easier to reproduce. Yours is definitly better.

@sni
Copy link
Author

sni commented Jan 26, 2021

thanks for fixing this so quick

@jpreese
Copy link

jpreese commented Jan 26, 2021

A really dumb example, but I assume @sni's issue is the same as this one?

	ctxValue := ctx.Value("foo")
	if ctxValue == nil {
		panic("no foo!")
	}

	otherCtx, ok := ctxValue.(*context.Context)
	if !ok {
		panic("no ctx!")
	}

	return
variable 'ctxValue' is only used in the if-statement (xxx); consider using short syntax (ifshort)
        ctxValue := ctx.Value("foo")

Where ctx.Value returns interface{} (which is a pointer due to interface shenanigans)

@esimonov
Copy link
Owner

Hello @jpreese , thanks for joining us!
In fact, it is yet another case that wasn't properly handled.
It's now fixed in 113a74d.

@jpreese
Copy link

jpreese commented Jan 27, 2021

Oh perfect, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants