Add a check for boolean expressions containing pointless/overriding connectives

[josharian]

I originally prepared a go vet check for this, but came to the conclusion that it was a better fit for go lint. I'm opening this issue as a place to stash the idea + tested go vet code until such time as I (or someone else) adapts it.

Running this check over a recent corpus of public code caught this distribution of ugliness:

    4303 use ! instead of '== false'
    2762 '== true' is extraneous
    2710 use ! instead of '!= true'
    1398 '!= false' is extraneous
      63 '&& false' always yields false
      32 '&& true' is extraneous
      29 '!!' is extraneous
      18 '|| false' is extraneous
      17 '|| true' always yields true

Test cases:

func OhItHurts() {
    var b bool

    _ = b == true  // ERROR "'== true' is extraneous"
    _ = b == false // ERROR "use ! instead of '== false'"
    _ = b != false // ERROR "'!= false' is extraneous"
    _ = b != true  // ERROR "use ! instead of '!= true'"

    _ = b && true  // ERROR "'&& true' is extraneous"
    _ = true && b  // ERROR "'&& true' is extraneous"
    _ = b || false // ERROR "'|| false' is extraneous"

    _ = b && true && b // ERROR "'&& true' is extraneous"
    _ = b && b && true // ERROR "'&& true' is extraneous"
    _ = true && b && b // ERROR "'&& true' is extraneous"

    _ = !!b // ERROR "'!!' is extraneous"
}

func DebugMeNot() {
    var b bool

    _ = b && false // ERROR "'&& false' always yields false"
    _ = b || true  // ERROR "'|| true' always yields true"
    _ = true || b  // ERROR "'|| true' always yields true"
}

Excerpted vet check:

func checkBoolUnaryExpr(f *File, e *ast.UnaryExpr) {
    if e.Op != token.NOT {
        return
    }
    if u, ok := e.X.(*ast.UnaryExpr); ok && u.Op == token.NOT {
        f.Badf(e.Pos(), "'!!' is extraneous")
    }
}

var dual = [token.VAR + 1]token.Token{ // token.VAR+1: issue 7951
    token.LAND: token.LOR,
    token.LOR:  token.LAND,
    token.EQL:  token.NEQ,
    token.NEQ:  token.EQL,
}

func checkBoolBinaryExpr(f *File, e *ast.BinaryExpr) {
    if dual[e.Op] == token.Token(0) { // not a boolean operator
        return
    }

    var tf string
    var op token.Token
    for _, expr := range [...]ast.Expr{e.X, e.Y} {
        if ident, ok := expr.(*ast.Ident); ok {
            switch ident.Name {
            case "false":
                op = dual[e.Op]
                tf = ident.Name
                break
            case "true":
                op = e.Op
                tf = ident.Name
                break
            }
        }
    }

    if tf == "" {
        return
    }

    switch op {
    case token.EQL, token.LAND:
        f.Badf(e.Pos(), "'%s %s' is extraneous", e.Op, tf)
    case token.NEQ:
        f.Badf(e.Pos(), "use ! instead of '%s %s'", e.Op, tf)
    case token.LOR:
        f.Badf(e.Pos(), "'%s %s' always yields %s", e.Op, tf, tf)
    }
}

Note that most of these can be fixed with simple gofmt -r invocations.

[dsymonds]

It is indeed a better fit for lint than vet.

I'm not sure about your first four (comparing against true/false). It's true that they are equivalent, but code can carry semantic emphasis by doing that, so I'm wary of flagging them as wrong.

The remainder, now that I think about it, might match vet better. Though they are not incorrect code, they may be accidentally hiding code, and flagging them there probably makes sense in the same way that flagging unreachable code makes sense. I wonder what Rob thinks about that idea.

[josharian]

I'm not sure about your first four (comparing against true/false). It's true that they are equivalent, but code can carry semantic emphasis by doing that, so I'm wary of flagging them as wrong.

Do you have an example handy?

Flipping through instances from the corpus, comparing against true/false appears to be a habit of the developer, rather than being reserved for emphasis.

The remainder, now that I think about it, might match vet better. Though they are not incorrect code, they may be accidentally hiding code, and flagging them there probably makes sense in the same way that flagging unreachable code makes sense.

That was the theory I held when I wrote it as a vet check, but it didn't survive first contact. I did some sampling, and I did not find any flagged code that was clearly buggy. I found lots of code that I didn't understand -- and which thus might have been buggy -- but nothing obviously incorrect.

I'd prefer to be wrong about this though, so feel free to push on it. :)

[dsymonds]

On 28 June 2014 01:53, Josh Bleecher Snyder notifications@github.com wrote:

Do you have an example handy?

I don't have an example handy, but it reminds me of test code for
functions that return a bool, and sometimes I find it clearer to write
if foo(tt.in) == false. I don't think that's necessarily that much
worse than if !foo(tt.in) to warrant warning people about it.

That was the theory I held when I wrote it as a vet check, but it didn't survive first contact. I did some sampling, and I did not find any flagged code that was clearly buggy. I found lots of code that I didn't understand -- and which thus might have been buggy -- but nothing obviously incorrect.

I'd prefer to be wrong about this though, so feel free to push on it. :)

If it empirically doesn't find any bad code, or very little bad code,
we should probably just drop it.

[josharian]

Sounds right. Dropped! Thanks for the helpful discussion.

[two]

m