Go-tools: Bug: misjudgement of return statement in set.Set Each function

[qshuai]

my code:

setParent.Each(func(item interface{}) bool {
    parent := item.(*mempool.TxMempoolEntry)
    if !ba.inBlock.Has(parent) {
        return true      // to return function result
    }
    return true              // to control Each loop
})
return false                     // to return function result

error log in gosimple, but it is correct in fact:

warning: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008) (gosimple)
warning: should use '\''return <expr>'\'' instead of '\''if <expr> { return <bool> }; return <bool>'\'' (S1008) (gosimple)

[dmitshur]

if !ba.inBlock.Has(parent) {
    return true // to return function result
}
return true // to control Each loop

That code has the same behavior as:

return true

Is that intentional? Why do you check !ba.inBlock.Has(parent) if the result doesn't matter?

[qshuai]

oh, my god! It is my fault error, the first return statement is also control loop exec. It works as following:

func (ba *BlockAssembler) isStillDependent(te *mempool.TxMempoolEntry) bool {
    setParent := blockchain.GMemPool.GetMemPoolParents(te)
    ret := false
    setParent.Each(func(item interface{}) bool {
        if !ba.inBlock.Has(item.(*mempool.TxMempoolEntry)) {
            ret = true
            return false
        }
        return true
    })
    return ret
}

[dmitshur]

Are you using the latest version of gosimple?

I can't reproduce with a simplified snippet:

package main

import (
    "fmt"
    "time"
)

func main() {
    fmt.Println(isStillDependent())
}

func isStillDependent() bool {
    ret := false
    forEach(func(item interface{}) bool {
        if time.Now().Before(time.Now()) {
            ret = true
            return false
        }
        return true
    })
    return ret
}

func forEach(f func(item interface{}) bool) {
    f(nil)
}

Can you share complete code that compiles and reproduces this, or point to where the above code can be fetched from?

Also, what line(s) does the warning point to?

[qshuai]

@shurcooL Ok. Your instance is different from mine. The function forEach does not control a loop, but set.Each() is.

Simplified example, as following:

package main

import "gopkg.in/fatih/set.v0"

func main() {
    s := set.New()
    s.Add(1)
    s.Add(2)
    s.Add(3)

    s.Each(func(item interface{}) bool {
        ele := item.(int)
        if ele == 2 {
            return false
        }
        return true
    })
}

// Result：
gosimple test.go
a.go:13:3: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)

To keep the same logic, your code should be modified like this(control loop):

package main

import (
    "fmt"
    "time"
)

func main() {
    fmt.Println(isStillDependent())
}

func isStillDependent() bool {
    forEach(func(item interface{}) bool {
        if time.Now().Before(time.Now()) {
            return false
        }
        return true
    })
    return true
}

func forEach(f func(item interface{}) bool) {
    for {
        if !f(nil) {
            break
        }
    }
}

// output
gosimple test.go
a.go:14:3: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)

There is the partial origin code of set

func (s *Set) Each(f func(item interface{}) bool) {
    s.l.RLock()
    defer s.l.RUnlock()

    for item := range s.m {
        if !f(item) {   // the boolean result control loop，not to return final result
            break 
        }
    }
}

[qshuai]

@shurcooL So gosimple works OK on this. It is my error.

[dmitshur]

Thanks, now I can reproduce it.

However, the check appears to be correct. Namely, it's suggesting that this Go code:

if ele == 2 {
    return false
}
return true

Can be simplified to:

return ele != 2

And this Go code:

if time.Now().Before(time.Now()) {
    return false
}
return true

Can be simplified to:

return !time.Now().Before(time.Now())

Both of which are true.

You can find more information about the S1008 check at https://staticcheck.io/docs/gosimple#S1008.