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/compile: unreachable "break" after return in switch statement causes erroneous compiler static analysis #37364

Closed
fsufitch opened this issue Feb 21, 2020 · 8 comments
Milestone

Comments

@fsufitch
Copy link

@fsufitch fsufitch commented Feb 21, 2020

What version of Go are you using (go version)?

$ go version
1.13.7

(from the Playground)

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

The Go playground at https://play.golang.org

What did you do?

https://play.golang.org/p/K_qpq7GmBhF

The break after a return in a switch/case causes the compiler to think that the wrapping function is missing a return, when in fact it is impossible for the function to break out of the switch.

What did you expect to see?

1

(or 2 or 3 depending on the variable)

What did you see instead?

./prog.go:17:1: missing return at end of function
@therealfakemoot
Copy link

@therealfakemoot therealfakemoot commented Feb 21, 2020

A reproducing code sample that's even more aggressively minimal: https://play.golang.org/p/MGido5G7mc7

@fsufitch
Copy link
Author

@fsufitch fsufitch commented Feb 21, 2020

This may be a philosophical argument about what constitutes a "terminating statement" in the context of switch. The language spec says a terminating statement is:

A "switch" statement in which:

  • there are no "break" statements referring to the "switch" statement,
  • there is a default case, and
  • the statement lists in each case, including the default, end in a terminating statement, or a possibly labeled "fallthrough" statement.

(https://golang.org/ref/spec#Terminating_statements)

In the code examples above, the switch statement was not a terminating statement because it had a break statement in it referring to it. However, that break statement is itself unreachable, so the switch is unaffected by it, and is functionally a terminating statement (even if it does not fit the spec's strict definition).

In other words, the strict wording of the spec does not support the behavior in this issue being a "bug" per se.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 21, 2020

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 21, 2020

I think the most important point in this area is that we keep the rules for terminating statements simple, so that all compilers can simply and easily agree as to which statements terminate and which do not. I think that is much more important than handling cases like return; break. Admittedly humans can easily see that the break is not relevant, but writing down simple rules for that is less easy.

Also, return; break is simply not common Go code.

So my preference here would be to do nothing.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 21, 2020

Also, vet should catch this.

@toothrot toothrot added this to the Backlog milestone Feb 21, 2020
@fsufitch
Copy link
Author

@fsufitch fsufitch commented Feb 21, 2020

$ go build
./test.go:17:1: missing return at end of function

$ go vet
vet: ./test.go:17:1: missing return

Vet has the same confusion about the missing return that build does. Is that what you meant by vet "catching" it, or could vet give a more helpful error like "line 11 is unreachable"?

@josharian
Copy link
Contributor

@josharian josharian commented Feb 21, 2020

Oh right, because vet needs the code to typecheck before it runs.

@odeke-em odeke-em changed the title Unreachable "break" in switch statement causes erroneous compiler static analysis cmd/compile: unreachable "break" after return in switch statement causes erroneous compiler static analysis Feb 22, 2020
@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 24, 2020

I agree with @ianlancetaylor here: Making the rules more complicated just to catch a few extra, possibly pathological, cases does not seem worth it. It is also a slippery slope: What about

func f() int {
   if true {
      return 0
   }
}

(https://play.golang.org/p/_bZ7FxnpNbt)

It's obvious that this function returns, yet it is not covered by the current rules and the compiler will complain. It's not obvious where to stop, once we make the existing rules even just slightly more complicated.

That all said, the current behavior is correct according to the spec and I am closing this as "working as intended".

If you disagree, I encourage you to open a new issue with a concrete proposal regarding the rule change you're envisioning. Thanks.

@griesemer griesemer closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.