Go: cmd/vet: warn about `break` at end of case/comm clause

[mdempsky]

Consider this code from golang.org/cl/24975:

    // Check selection is in a FuncDecl, but not in its body.
    var decl *ast.FuncDecl
    for _, n := range path {
        switch n := n.(type) {
        case *ast.BlockStmt:
            return fmt.Errorf("selection is inside function body")
        case *ast.FuncDecl:
            decl = n
            break
        }
    }
    if decl == nil {
        return fmt.Errorf("selection is not a function declaration")
    }

The intention was for the break statement to terminate the enclosing for loop, but it actually uselessly terminates the switch statement instead. A break at the end of a switch or select statement is always useless, and when the outer context is also breakable, it might be a mistake.

I suspect this isn't actually common enough to warrant a cmd/vet check, but thought it at least deserves an issue for discussion.

/cc @alandonovan @robpike

[dominikh]

FWIW, this check is implemented in staticcheck.

[mdempsky]

Checks that break statements aren't missing labels when trying to break out of a loop from inside a switch or select statement.

That sounds like staticcheck would only warn if the for loop above already had a label?

[dominikh]

See https://github.com/dominikh/go-staticcheck/blob/master/testdata/scoped-break.go for the test cases.

[adonovan]

I'm in favor of adding this to vet.

[josharian]

Seems at least worth gathering data on. I happened to clean up one of these yesterday because it confused me: https://github.com/josharian/go/commit/04572717882343ecda360194404e971494c9a95f.

[gopherbot]

CL https://golang.org/cl/26665 mentions this issue.

[bradfitz]

I'll repeat my comment I left to @josharian on his https://github.com/josharian/go/commit/04572717882343ecda360194404e971494c9a95f

I wrote:

I don't like empty switch cases. They look like TODOs to me. "break" seems as a good as an explicit comment " // do nothing".

But I'm not fussy about it.

[robpike]

I agree with Brad. More objectively, Brad's point shows that there is a valid and reasonable use for this construct, which makes it unsuitable for vet to prevent.

Closing.