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

Comments

Projects
None yet
9 participants
@dotaheor
Copy link

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

@gopherbot gopherbot added the Proposal label Feb 1, 2018

@mvdan

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

The second block is the same as

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

@cznic

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

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

cc @dominikh

@dominikh

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

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.

@josharian josharian closed this Feb 1, 2018

@j7b

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

@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.

@dominikh dominikh referenced this issue Sep 18, 2018

Open

staticcheck: incorporate go vet checks #149

5 of 22 tasks complete

@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.
You can’t perform that action at this time.