Go: Proposal: disallow "fall through" to a labeled statement

[griesemer]

[Edited to make an exception for labeled loops where the labels are only used to break out of (or continue) the loops with a break or continue statement.]

We propose that the language be changed such that the statement immediately before a labeled statement (in the same block) must be a terminating statement or an explicit fallthrough statement if (and only if) the label is the target of a goto statement. (Note that a goto statement jumping to the label is a terminating statement and thus also a valid choice.)

Specifically, loops that are labeled only so that they can be exited or continued with "labeled" break or continue statements (with no goto statement jumping to the same label anywhere) do not need a "fallthrough" to enter the loop.

This is not a backward-compatible change.

Rationale

Currently the language permits code to "fall through" into a label. For instance, the following code is permitted:

   // arbitrary statement which is not a terminating statement

exit:
  // handle special condition
  return ...
}

Here, an exit label is used to handle a special condition, for instance an error. Code such as this is not uncommon in functions that deal with a great number of conditions that all need to be satisfied for some reason, and which bail if any of the conditions is not satisfied. Examples of such code can be found in the compiler's type checker and other places. In many of these situations it is arguably an error if the code before the label "falls through" into the code following the label. Typically the code before the label will be a return statement.

In situations such as these, using a goto and a label in this semi-structured way is a simple, and very effective approach, but often looked down upon due the negative reputation goto's have. Sometimes a goto is exactly the right solution, and any rewrite or factoring only leads to more complexity.

Disallowing unintended fall throughs into labeled sections of a block will prevent a class of stupid errors. If fall through is intended, it's trivially achieved with an explicit fallthrough statement (or a goto to the label) which has the nice side-effect of documenting the intent clearly.

The language has a related requirement already for switch statements (note that in original C, a switch statement is essentially a computed goto).

Implementation

Besides updating the spec and related documentation, making this change would require a relatively simple change to the compiler's label and goto checker code in the front-end. The analogous change would be needed in go/types and gccgo. And of course, existing code that currently falls through would have to be adjusted (this could be done with gofmt).

[Merovius]

Can't this be done as a vet-check instead, for much of the benefit without the breakage?

Personally, I pretty much only use labeled statements to label outer loops to break out of (or continue). IMO, it is pretty awkward having to fallthrough to that - it's just a label to refer to the loop.

IMHO the upsides don't outweigh the downsides on this one…

[MichaelTJones]

This is a surprising proposal in that it has the tail wagging the dog.

The meaning of labels in Go1 is as a tag, a handle for "there" in goto label and a label for "what to break from" in break label. This proposal suggests that because some developers write code that (a) enters a labeled section by sequential execution, (b) where they do not want be in the labeled section, and, (c) they also don't want to write return, goto, break, continue, or panic at the end of their preceding code, that the language should change to make their refusal to say what happens before the label a compile error.

I strongly prefer the Go1 definition of this as a programmer error. Let's look at cases:

a := 1
if b {
   goto label
}
a *= 8
// I, programmer, refuse to do anything special here
label:
a *=2
return a

When the developer says, "but I wanted 2 or 8, not 2 or 16!", this proposal changes the language. I'd rather change the programmer. Not so much to resist change as to defend reason in the fundamental sequential nature of imperative programming, not just in Go, but in nearly all programming (other than SNOBOL with T and F both defined), and also in recipes, knitting and crochet, preflight checklists, and most every structured human endeavor.

If they want 2 or 8, they need not to refuse to put code where that comment is.

[99yun]

remove fall through

[alanfo]

@griesemer

Just wondering whether the timing of the present proposal has anything to do with the try proposal (#32437) where @josharian suggested that labelled error handlers (based on something you'd originally mooted yourself) might be preferable to using defer?

I can't say I'd be in favor of it otherwise as only a few days ago I remember writing some code where I needed to jump to a spot intermediate between two nested for statements.

Are you saying that fallthrough would become available in any such scenario and not just in switch statements as at present?

[deanveloper]

I would love to see the actual uses of labels in everyday repositories. Standard library may not be a good test case for this because it requires lots of micro-optimization so it may have higher prevalence of things like goto. I have a feeling that labels are mainly used in order to label and break out of outer loops, which require fallthrough behavior. I may be wrong though, as I am only speaking from personal experience, so it would be good to see some data.

[carlmjohnson]

To prevent accidental fallthrough, isn't it sufficient to add panic("unreachable") before the label in code using goto in this way? Obviously, testing could miss it and like the old switch break pair, it's easy to omit, but goto code is so unusual nowadays, I don't think there would be that much burden to add it as a code review step or whatever. Plus you only need one panic, versus many breaks in a switch.

[MichaelTJones]

Dean, they look like this. The proposal would make it a compile failure to
have the program flow from "idiom := f.Idiom()" to "for _,e := range
b.Succs{"

// addDFphis creates new trivial phis that are necessary to correctly
reflect (within SSA)
// a new definition for variable "x" inserted at h (usually but not
necessarily a phi).
// These new phis can only occur at the dominance frontier of h; block s is
in the dominance
// frontier of h if h does not strictly dominate s and if s is a successor
of a block b where
// either b = h or h strictly dominates b.
// These newly created phis are themselves new definitions that may require
addition of their
// own trivial phi functions in their own dominance frontier, and this is
handled recursively.
func addDFphis(x Value, h, b *Block, f *Func, defForUses []Value, newphis
map[Block]rewrite, sdom SparseTree) {
oldv := defForUses[b.ID]
if oldv != x { // either a new definition replacing x, or nil if it
is proven that there are no uses reachable from b
return
}
idom := f.Idom()
*outer:
for _, e := range b.Succs {
s := e.b
// check phi functions in the dominance frontier
if sdom.isAncestor(h, s) {
continue // h dominates s, successor of b,
therefore s is not in the frontier.
}
if _, ok := newphis[s]; ok {
continue // successor s of b already has a new phi
function, so there is no need to add another.
}
if x != nil {
for _, v := range s.Values {
if v.Op == OpPhi && v.Args[e.i] == x {
* continue outer* // successor s of
b has an old phi function, so there is no need to add another.
}
}
}

            old := defForUses[idom[s.ID].ID] // new phi function is

correct-but-redundant, combining value "old" on all inputs.
headerPhi := newPhiFor(s, old)
// the new phi will replace "old" in block s and all blocks
dominated by s.
newphis[s] = rewrite{before: old, after: headerPhi} //
record new phi, to have inputs labeled "old" rewritten to "headerPhi"
addDFphis(old, s, s, f, defForUses, newphis, sdom) // the
new definition may also create new phi functions.
}
for c := sdom[b.ID].child; c != nil; c = sdom[c.ID].sibling {
addDFphis(x, h, c, f, defForUses, newphis, sdom) // TODO:
convert to explicit stack from recursion.
}
}

On Wed, Jun 12, 2019 at 7:23 AM Carl Johnson [e-mail]
wrote:

To prevent accidental fallthrough, isn't it sufficient to add
panic("unreachable") before the label in code using goto in this way?
Obviously, testing could miss it and like the old switch break pair, it's
easy to omit, but goto code is so unusual nowadays, I don't think there
would be that much burden to add it as a code review step or whatever. Plus
you only need one panic, versus many breaks in a switch.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/32565?email_source=notifications&email_token=AB4DFJJSWYPLCFQVOHS2FULP2EBE7A5CNFSM4HXFKSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQSSZI#issuecomment-501295461,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB4DFJPMEZ5XIS5VF3ZPQWDP2EBE7ANCNFSM4HXFKSUQ
.

--

Michael T. Jonesmichael.[email protected] [e-mail]

[rogpeppe]

I like the intent of this proposal, but I agree with @Merovius that it's not appropriate for labelled loops, where it's entirely appropriate enter the loop but use a label to break out.

Maybe the rule could be that the rule only applies to labels that are the target of a goto (as opposed to a break or continue statement).

[Merovius]

@rogpeppe I'd be on board with that

[griesemer]

@Merovius , @rogpeppe Indeed, you make an excellent point. I missed that last night. I've updated the proposal accordingly: The requirement for an explicit fallthrough to a labeled statement now only applies to labeled statements which are the target of a goto statement.

[griesemer]

@MichaelTJones The example you are giving is exactly the kind of code where a fallthrough would be appropriate. If there's any good reason for not changing that code into the structured and much more easily readable

a := 1
if !b {
   a *= 8
}
a *=2
return a

the programmer should be forced to say so, explicitly. This may just be an unfortunate example, so I'd be curious if you have more convincing ones.

(Please also note that I amended the proposal to accommodate loops better.)

[griesemer]

@alanfo Yes, a fallthrough would not just become available but would be required in cases outlined in the proposal (but see the amendment for loops). There is no denying that this idea came up while thinking about the consequences of using a try jumping to a label rather than doing a return.

[networkimprov]

Are there documented cases where the current label rules caused bugs? If so, could you list references?

[DeedleFake]

While I certainly agree that this should only apply to labeled statements that are the targets of gotos and not ones that are being used to identify loops, proper error messages for this are very important. For example:

// ...

// This got added later.
if somethingOrOther {
  goto loop
}

// ...

loop:
for {
  // ...
  if yetAnotherThing {
    break loop
  }
  // ...
}

When that goto gets inserted, suddenly the loop label becomes an error. The error message should have something like

./example.go:30:2: reachable labeled statement requires explicit fallthrough because of goto at ./example.go:25:2

Or something.

On a related note, something similar might be nice for the shadowed during return error.

[deanveloper]

I think a much more easily noticeable rule would be to disallow fallthrough to labelled terminating statements, rather than disallow fallthrough for labelled statements that are the targets of gotos.

Maybe that's not the correct rule, I'm not sure yet. But I feel like the rule should be based on what the label is labelling rather than whether or not the label is the target of a goto.

[griesemer]

@DeedleFake This proposal was not meant to encourage unjustified uses of gotos. They should be extremely sparingly used, if at all. But there are rare cases where a goto is exactly the right thing. If that is the case in your specific example, it is _ok_ to have to write that fallthrough into the loop.

[ohir]

Excuse me, but IMO such a detail do not deserve a huge breaking¹ change. Warnings about possible slip should be left to go vet.

¹. It is breaking mental model of the language. It not only adds a half of a new meaning to the fallthrough keyword, but also makes its interpretation contextual to the obscure details level »_if (and only if) the label is the target of_«.

What should I tell to the junior programmer who once a million lines of code will see such egregiously outplaced fallthrough?

[Merovius]

@ohir FTR, it is already important whether or not a label is used. And it's not an "only if" - AIUI you can use fallthrough even if the label is not the target of a goto (and instead the target of a break/continue).

[ohir]

@Merovius I cited _"only if"_ from the proposal. I am aware that it was added later in response to @rogpeppe good concerns. My opinion is that people who know where to put a goto need not a nanny compiler but people who do not feel good in goto's presence will either stuck at 'misplaced' fallthrhrough or will have to remember a murky rule about rare usage of the keyword that for long had only been a part of the switch construct.

[Merovius]

Another case I just stumbled over that IMO is interesting in light of this proposal: go/scanner.go. IMO requiring a fallthrough as the first line of the function is overkill. Arguably, this code could be fixed some other way (maybe the function should just recurse), but it's an example to keep in mind.

[carlmjohnson]

Good case for a become statement.

[griesemer]

@Merovius Yes, indeed, thanks for pointing this out. The scanner could just use a for {} loop and a continue inside (recursion makes the scanner sensitive to stack overflow if fed malicious code - and it's trivially avoided). The only reason for the goto was that I didn't want to have the entire body indented because of the loop.

Maybe this proposal is not such a good idea after all.

[josharian]

You could restrict the proposal to labels that are the target of a forward goto. (Suitably phrased, this also could take care of labels attached to switch and for statements, since those are always backwards-looking.)

[josharian]

This restriction wouldn’t necessarily require adding a new meaning for fallthrough. Since there is by definition a label present, you could use a goto and a panic:

  goto L
  // ...
  goto L
  panic(“required terminating statement”)
L:
  // ...

Given that, it seems worth exploring this initially as a vet check. (With all the restrictions we’ve discovered to date.)

cc @mvdan

[MichaelTJones]

Even though I’m not in favor of the proposal, I am 100% in favor of any and
all Go Vet checks. Was looking recently at professional tools of that kind
and there is marvelous insight in the things they test for.

My wife had a colleague at IBM who did a company wide analysis of common
sources of coding errors and published it. Things like those insights would
be great in Vet. (How likely is this fragment copied code not fully changed
for its new location, for example)

It would be a cool Google R&D thing to gather coding errors and build an AI
model for danger signs. That would make the Go LSP backend a brilliant
resource.

On Sat, Jun 15, 2019 at 10:54 PM Josh Bleecher Snyder <
[email protected]> wrote:

This restriction wouldn’t necessarily require adding a new meaning for
fallthrough. Since there is by definition a label present, you could use a
goto and a panic:

goto L

// ...

goto L

panic(“required terminating statement”)

L:

// ...

Given that, it seems worth exploring this initially as a vet check. (With
all the restrictions we’ve discovered to date.)

cc @mvdan https://github.com/mvdan

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/32565?email_source=notifications&email_token=AB4DFJIKHAGDP3BW7BH47D3P2XIQJA5CNFSM4HXFKSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXZF3EI#issuecomment-502422929,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB4DFJPJDN422IG3YIBCZ3TP2XIQJANCNFSM4HXFKSUQ
.

>

Michael T. Jonesmichael.[email protected] [e-mail]

[griesemer]

@josharian Since a goto is a terminating statement, you don't even need the panic.

[griesemer]

I'm going to retract this proposal. The proposal as stated is not backward-compatible and it doesn't seem warranted to introduce a non-backward compatible language change for what is admittedly a marginal safety improvement. Furthermore, this proposal doesn't address an urgent need.

Note that a vet check wouldn't be possible if it required that one use fallthrough as proposed because fallthrough is not permitted outside switch statements. One could require people to write

    goto L

L: ...

i.e., use a goto statement to the label immediately following instead of a fallthrough if this were desirable.

Closing.