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: cmd/vet: warning on a special case of switch code block use. #23651

Closed
dotaheor opened this issue Feb 1, 2018 · 16 comments
Closed

Comments

@dotaheor
Copy link

dotaheor commented Feb 1, 2018

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

go version go1.9.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

func alwaysFalse() bool {
	return false
}

func main() {
	switch alwaysFalse() {
	case true: println("1: true")
	case false: println("1: false")
	}
	
	switch alwaysFalse() // vet should warn here.
	{
	case true: println("2: true")
	case false: println("2: false")
	}
}

What did you expect to see?

go vet should warn on the second switch code block use.

What did you see instead?

go vet reports nothing.

@gopherbot gopherbot added this to the Proposal milestone Feb 1, 2018
@mvdan
Copy link
Member

mvdan commented Feb 1, 2018

Why should it warn on the second switch but not on the first? I'm not sure I understand your point.

@cznic
Copy link
Contributor

cznic commented Feb 1, 2018

The second block is the same as

switch alwaysFalse(); {
case true: println("2: true")
case false: println("2: false")
}

@josharian
Copy link
Contributor

To be honest, I was surprised to learn that that compiles. I doubt that it hits the “frequency” criterion for vet, but making gofmt put that on a single line would probably make the bug obvious enough.

@j7b
Copy link

j7b commented Feb 1, 2018

@josharian maybe it shouldn't compile, https://golang.org/ref/spec#ExprSwitchStmt specifies the ";"

Edit: also the EBNF presented at #ExprSwitchStmt in the spec appears to be missing the alternating and grouping components.

@josharian
Copy link
Contributor

cc @griesemer

@cznic
Copy link
Contributor

cznic commented Feb 1, 2018

maybe it shouldn't compile, https://golang.org/ref/spec#ExprSwitchStmt specifies the ";"

It's valid Go code, the semicolon is injected at EOL after ) being the last token on the line

@dgryski
Copy link
Contributor

dgryski commented Feb 1, 2018

If it's correct but dubious, possibly the check belongs elsewhere.

cc @dominikh

@dominikh
Copy link
Member

dominikh commented Feb 1, 2018

I'll probably add such a check to staticcheck, unless someone can come up with a valid use case, other than generated code.

@griesemer
Copy link
Contributor

@j7b The syntax always specifies all required semicolons ( https://golang.org/ref/spec#Semicolons ). The source is free to omit them when they are automatically inserted. For instance, it's equally valid to write

for i := 0; i < 10; i++ {}

or

for
	i := 0
	i < 10
	i++ {}

@griesemer
Copy link
Contributor

griesemer commented Feb 1, 2018

@josharian gofmt explicitly introduces a semicolon, I think we're good there.

More generally, I doubt this hits the frequency requirement for go vet. There's really nothing wrong here except that the programmer chose to use a) a bad style, and b) not to use gofmt.

@josharian
Copy link
Contributor

gofmt explicitly introduces a semicolon, I think we're good there.

And if there's no trailing comment on the switch line, it will move the brace up as well. Agreed, we're good here.

I think there's consensus that this isn't for vet, and there's nothing to do on the gofmt side. I'm going to close this. I trust that if @dominikh adds this to staticcheck, he'll update here.

@j7b
Copy link

j7b commented Feb 2, 2018

@griesemer my thought was the explicit semicolon disambiguates SimpleStmt and Expression, the latter of which may be a simple statement. Also gofmt should probably fix where both occur, https://play.golang.org/p/RNUhHUaQBv4 for example.

@griesemer
Copy link
Contributor

@j7b Understood. But there's no notion of explicit or implicit semicolon from the compiler's parser's point of view; the semicolons are inserted during lexical analysis, purely based on the last token on a line. The point of Go's semicolon rules is exactly to not be "smart" (which is to say, context-sensitive) about it (say like Javascript, or Scala), because that makes the rules actually simple and clear.

For the same reason, gofmt cannot "fix" the code because there's actually a bug in the source: In your example, at the end of line 6, the parser sees a (implicitly inserted) semicolon. (*)

(*) To be precise, there's a little bit of localized smarts in error messages: In this case, even though the parser sees a semicolon (which is the reason for the bug), it knows that it's caused by a newline and thus mentions newline rather than semicolon. In the distant past it would say "expected '{' found semicolon" and people were confused (because there isn't one in the source). The current error message is more accurate but slightly harder to match with the actual syntax (one needs to know that the newline got translated into a semicolon). That trade-off is justified because it's a much more useful error message: It's clear that one just needs to remove the newline to fix the bug.

But let's be clear: The automatic semicolon insertion in Go is one of the things Go "gets right": it has tremendous benefit on readability of source (by removing clutter) yet it permits a simple syntax (with consistent semicolons in productions for robustness), and requires only two context-insensitive rules on the lexical level (leading to a trivial implementation).

@j7b
Copy link

j7b commented Feb 3, 2018

@griesemer gofmt might not be right, https://play.golang.org/p/k9XXEYf8gBZ doesn't compile but formats into something wrong in a different way.

I think #Semicolons compounds the problem, it's about omitting semicolons from source that occur as part of a production, the semicolon is an optional term in the switch clause, how can the parser presume it's omitted? The compounding includes https://play.golang.org/p/du62DMw8-P- which demonstrates semicolon injection where there's no semicolon to omit. Also injection only applies to "terminators," which aren't defined, the only projection that looks to me like it has things that are unambiguously terminators is the statement list.

@cznic
Copy link
Contributor

cznic commented Feb 3, 2018

gofmt might not be right, https://play.golang.org/p/k9XXEYf8gBZ doesn't compile but formats into something wrong in a different way.

Gofmt formats that code precisely right w/o changing any of its semantics. The code is invalid before formatting and is invalid after formatting in the exactly same way. The compiler error before/after formatting is the same.

Gofmt works in this case flawlessly and as intended.

@griesemer
Copy link
Contributor

@j7b As @cznic already mentioned, gofmt does exactly the right thing in this example ( https://play.golang.org/p/k9XXEYf8gBZ ): As written, after true == false, because false is the last Go token on that line, a semicolon is automatically inserted during lexical analysis - it doesn't matter what the switch syntax says with respect to semicolons. That is, that code looks exactly the same for the compiler as https://play.golang.org/p/k9XXEYf8gBZ. The code doesn't compile because true == false is in the position of the switch's init statement, and (most) expressions w/o side-effects are not permitted as a statement, hence the code doesn't compile.

Semicolons are injected automatically purely on lexical basis (i.e., depending on the last lexical token on a line), not dependent on where they are required in the syntax. That is the whole point.

Please let's not discuss this any further on the issue tracker. If you have more questions in this regard, please use of the open discussion fora (golang-nuts, etc.). Thanks.

@golang golang locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants