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

proposal: spec: require explicit labels for break, continue #8591

Closed
griesemer opened this issue Aug 25, 2014 · 7 comments
Closed

proposal: spec: require explicit labels for break, continue #8591

griesemer opened this issue Aug 25, 2014 · 7 comments
Labels
FrozenDueToAge LanguageChange v2 A language change or incompatible library change
Milestone

Comments

@griesemer
Copy link
Contributor

It's a not uncommon mistake to use a unlabeled break statement inside a switch statement
with the intention to break a surrounding for loop:

for some_condition {
   switch tag {
   case foo:
      ...
      break  // intent is to break out of for loop
   ...
}

The compiler won't complain but the bug is non-obvious and often not easy to spot.

Break (and also continue) should require use of explicit labels. This will make the code
more readable and remove a not uncommon source of bugs. The benefit outweighs the minor
inconvenience; also this cases don't occur too often.

A slightly more complicated (but more convenient) approach would be to require the label
only if the target statement is ambiguous (i.e., a switch/for statement containing a
break is not nested within another statement that could be targeted by the break; and
analogous for continue).
@ianlancetaylor
Copy link
Contributor

Comment 1:

I'm not sure this is worth fixing.  If you really think it is, then I think the step to
take today would be to add a go vet warning for any ambiguous break or continue
statement.

@griesemer
Copy link
Contributor Author

Comment 2:

I think it is, and go vet should probably warn. It's happened enough time to me (and
others) that it's annoying. It's easy to miss, especially if the algorithm keeps running
anyway (e.g. an early exit that is an optimization).\

@robpike
Copy link
Contributor

robpike commented Aug 26, 2014

Comment 3:

Almost never happened to me. The idea of complaining if there is a potential ambiguity
is interesting though.
Vet maybe, language change meh.

@gopherbot
Copy link

Comment 4 by toddw@google.com:

FYI I don't encounter this alot, but definitely enough times to annoy me.  It's an easy
bug to miss when reviewing code.  Here's an example where I've seen it occur.  We start
with this:
for cond {
  if tag == foo {
    ...
    break // breaks out of for loop
  } else if tag == bar {
    ...
  }
}
Now we re-write it as this:
for cond {
  switch tag {
  case foo:
    ...
    break // doesn't break out of for loop
  case bar:
    ...
  }
}
Here's another example; "break" applies to the innermost "for", "switch" or "select",
while "continue" only applies to the innermost "for".  I'm not proposing any changes to
that; it just highlights my example.  E.g.
for cond {
  switch tag {
  case foo:
    ...
    break // runs moreLogic and runs next loop iteration
  case bar:
    ...
    continue // skips moreLogic and runs next loop iteration
  }
  moreLogic()
}
This is pretty weird, since my intuition for "break" and "continue" is reversed.  I.e.
in a simple for loop, "break" runs fewer or equal lines of code in the loop than
"continue", while it's reversed when you nest a switch or select statement.  Sadly I've
reviewed actual code written like this, and had bugs all over.
Note that the definition of "ambiguity" is kind of complex.  The for loop is the only
valid target for the continue statement, and to me it doesn't seem confusing without the
"break" case.  But once you add the "break" case I find both the "break" and "continue"
cases confusing.  Maybe I'm weird.
FWIW my vote is for go vet warnings, only when there could be more than one valid target
for a break or continue statement.  I also wouldn't mind if the language enforced this,
but don't feel strongly.

@griesemer griesemer added longterm LanguageChange v2 A language change or incompatible library change labels Nov 8, 2014
@griesemer griesemer self-assigned this Nov 8, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title spec: require explicit labels for break, continue proposal: spec: require explicit labels for break, continue Jun 17, 2017
@pciet
Copy link
Contributor

pciet commented Dec 20, 2017

This happened to me recently but I don't like writing OUTER: everywhere. Maybe a breakall would be better? I use break enough to not want to have to use a label, although the for + switch/select break is definitely a source of mistakes for me.

@griesemer
Copy link
Contributor Author

I'm going to close this one. It's unlikely to find enough support and it's tedious and verbose in most programs.

@griesemer
Copy link
Contributor Author

Just for the record, if this issue were to find interest after all, instead of labels one could follow the break keyword with the keyword of the statement to be broken; e.g. break for to break the innermost enclosing for statement, etc. The 2nd keyword could be optional.

@golang golang locked and limited conversation to collaborators Jan 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

6 participants