Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mdempsky opened this issue Jul 18, 2016 · 8 comments

Comments

Projects
None yet
7 participants
@mdempsky
Copy link
Member

commented Jul 18, 2016

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

@mdempsky mdempsky added this to the Unplanned milestone Jul 18, 2016

@dominikh

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

FWIW, this check is implemented in staticcheck.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

@adonovan

This comment has been minimized.

Copy link

commented Jul 18, 2016

I'm in favor of adding this to vet.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

Seems at least worth gathering data on. I happened to clean up one of these yesterday because it confused me: josharian@0457271.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 16, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

I'll repeat my comment I left to @josharian on his josharian@0457271

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.

gopherbot pushed a commit that referenced this issue Aug 16, 2016

cmd/internal/obj/x86: minor code cleanup
Update #16415

Change-Id: I83e0966931ada2f1ed02304685bb45effdd71268
Reviewed-on: https://go-review.googlesource.com/26665
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2016

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.

@robpike robpike closed this Aug 16, 2016

@golang golang locked and limited conversation to collaborators Aug 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.