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: disallow "fall through" to a labeled statement #32565
Comments
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 IMHO the upsides don't outweigh the downsides on this one… |
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:
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. |
remove fall through |
Just wondering whether the timing of the present proposal has anything to do with the 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 Are you saying that |
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 |
To prevent accidental fallthrough, isn't it sufficient to add |
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 ***@***.***> 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
<#32565?email_source=notifications&email_token=AB4DFJJSWYPLCFQVOHS2FULP2EBE7A5CNFSM4HXFKSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQSSZI#issuecomment-501295461>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB4DFJPMEZ5XIS5VF3ZPQWDP2EBE7ANCNFSM4HXFKSUQ>
.
--
*Michael T. Jonesmichael.jones@gmail.com <michael.jones@gmail.com>*
|
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 |
@rogpeppe I'd be on board with that |
@MichaelTJones The example you are giving is exactly the kind of code where a 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.) |
@alanfo Yes, a |
Are there documented cases where the current label rules caused bugs? If so, could you list references? |
While I certainly agree that this should only apply to labeled statements that are the targets of // ...
// This got added later.
if somethingOrOther {
goto loop
}
// ...
loop:
for {
// ...
if yetAnotherThing {
break loop
}
// ...
} When that goto gets inserted, suddenly the
Or something. On a related note, something similar might be nice for the |
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 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 |
@DeedleFake This proposal was not meant to encourage unjustified uses of |
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 What should I tell to the junior programmer who once a million lines of code will see such egregiously outplaced |
@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). |
@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 |
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. |
Good case for a |
@Merovius Yes, indeed, thanks for pointing this out. The scanner could just use a Maybe this proposal is not such a good idea after all. |
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.) |
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 |
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 < ***@***.***> 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
<#32565?email_source=notifications&email_token=AB4DFJIKHAGDP3BW7BH47D3P2XIQJA5CNFSM4HXFKSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXZF3EI#issuecomment-502422929>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB4DFJPJDN422IG3YIBCZ3TP2XIQJANCNFSM4HXFKSUQ>
.
--
*Michael T. Jonesmichael.jones@gmail.com <michael.jones@gmail.com>*
|
@josharian Since a |
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 goto L
L: ... i.e., use a Closing. |
[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
orcontinue
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 agoto
statement. (Note that agoto
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
orcontinue
statements (with nogoto
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:
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 areturn
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 reputationgoto
's have. Sometimes agoto
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 agoto
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, aswitch
statement is essentially a computedgoto
).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 ingo/types
andgccgo
. And of course, existing code that currently falls through would have to be adjusted (this could be done withgofmt
).The text was updated successfully, but these errors were encountered: