Go-tools: SA4004 false positive

[JensRantil]

I have code similar to the following

for {
    err := doSomething()
    if err == nil {
        break
    }
    return err   // line 275
}

I get the following error:

myfile.go:275:3:warning: the surrounding loop is unconditionally terminated (SA4004) (megacheck)

My code is correct, so I'm not sure I should be seeing this warning.

[JensRantil]

$ megacheck --version
megacheck 2017.2

[dominikh]

Your code behaves identically to this:

if err := doSomething(); err != nil {
  return err
}

There is no possible way that your loop will execute more than once. Either the function call succeeded and the loop is broken out of, or the function call failed and the function is returned from.

[tv42]

I have something similar to this, that isn't as easily rewritten. I make a search (that will only ever get at most one result), process & return the hit if any, and if nothing was found I create the entry.

        for {
            doc, err := iter.Next()
            if err == iterator.Done {
                break
            }
            if err != nil {
                return nil, err
            }
            // unmarshal
            return result, nil
        }

        // create here

If at least right now don't see as neat of a way to rewrite that. At best I can see a switch or an if-else, both of which lead to indenting more code than the current -- the create branch is the longest one, and I would like to keep if flat. Or I could shuffle things into helper functions, but this code is already inside an inner function.

[tv42]

Well, I guess I'm rewriting that as

        doc, err := iterOne(iter)
        if err != nil {
            return nil, err
        }
        if doc != nil {
            // unmarshal
            return x, nil
        }

        // create here

Extracting the "iterate one" was the gimmick needed -- though I ended up with an ugly return nil, nil scenario.

[elonzh]

A false positive case?

SA4004: the surrounding loop is unconditionally terminated (staticcheck)
                return ptr

func reverseList(head *ListNode) *ListNode {
    for head != nil && head.Next != nil {
        ptr := head.Next
        head.Next = nil
        pre := head
        for ptr.Next != nil {
            tmp := ptr.Next
            ptr.Next, pre = pre, ptr
            ptr = tmp
        }
        ptr.Next = pre
        return ptr
    }
    return head
}

[dominikh]

@elonzh no, that's a true positive. The loop on line 1 will execute at most once, because you unconditionally return on line 12.