gordonklaus / ineffassign

Detect ineffectual assignments in Go code.
MIT License
390 stars 22 forks source link

ineffassign flagged on `err` assignment in for-loop. #85

Open AlexVulaj opened 8 months ago

AlexVulaj commented 8 months ago

I have the following snippet from some code I've written, where I've marked what I believe is a false-positive in-line. The context here is retrying calls with an AWS SDK.

result, err := AssumeRole(roleSessionName, nextClient, roleArn)
retryCount := 0
for err != nil {
    if retryCount < assumeRoleMaxRetries {
        time.Sleep(assumeRoleRetryBackoff)
        nextClient, err = createAssumeRoleSequenceClient(stsClientProviderFunc, lastCredentials, proxyURL)
        result, err = AssumeRole(roleSessionName, nextClient, roleArn) <-- this line, "err" is flagged even though it determines if we keep looping
        retryCount++
    } else {
        return aws.Credentials{}, fmt.Errorf("failed to assume role %v: %w", roleArn, err)
    }
}

For full context, here's a PR I've written that is being held up by this flag. I've linked directly to the line: https://github.com/openshift/backplane-cli/pull/235/files?diff=split&w=0#diff-d6d924fed86757f7dd2d61b335c33abce0fcbbd679ecde5a0c69019e4032412eR129

What's more odd is I've tried to reproduce this in a more simple manner and cannot seem to. For example, this does not get flagged:

import (
    "errors"
    "fmt"
)

var retryCount = 0

func main() {
    result, err := doThing()
    for err != nil {
        retryCount++
        if retryCount < 5 {
            fmt.Println("retrying")
            result, err = doThing()
        } else {
            return
        }
    }
    fmt.Println(result)
}

func doThing() (string, error) {
    if retryCount < 4 {
        return "", errors.New("error")
    } else {
        return "success", nil
    }
}

It's totally possible I'm misunderstanding something here, but to me this appears to be a false positive.

AlexVulaj commented 8 months ago

And of course... immediately as I posted this I realized my problem 😅 . Closing.

AlexVulaj commented 8 months ago

One quick question - I would've expected the previous line to be the one getting flagged, as that's the err assignment getting wiped out. Is that a potential bug?

gordonklaus commented 8 months ago

Yes, in your original example I would expect the first assignment to err in the loop to be flagged. Can you provide a self-contained example where it is not?