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

Jump to bottom

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

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

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

mdempsky opened this issue Jul 18, 2016 · 8 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@mdempsky
Copy link
Member

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
Copy link
Member

FWIW, this check is implemented in staticcheck.

@mdempsky
Copy link
Member Author

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
Copy link
Member

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

@adonovan
Copy link
Member

I'm in favor of adding this to vet.

@josharian
Copy link
Contributor

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

@gopherbot
Copy link

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

@bradfitz
Copy link
Contributor

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
@josharian
cmd/internal/obj/x86: minor code cleanup
35e25ef 
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
Copy link
Contributor

robpike commented Aug 16, 2016 •
edited

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 as completed 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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@bradfitz @mdempsky @dominikh @josharian @robpike @adonovan @gopherbot