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/vet: incorrectly emits "unreachable code" for jumped over consts #27760

Closed
schmichael opened this issue Sep 19, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@schmichael
Copy link
Contributor

commented Sep 19, 2018

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

go version go1.11 linux/amd64

(and playground which as of 2018-09-19 is on 1.10.3)

What did you do?

https://play.golang.org/p/O7If8gy-bUc

package main

import (
	"fmt"
)

func main() {
	goto PRINT

	const hello = "Hello World!"

PRINT:
	fmt.Println(hello)
}

What did you expect to see?

I don't even know what I expected. This might not even be a bug. Try stupid things, get stupid results?

What did you see instead?

In the playground:

prog.go:10: unreachable code
Go vet exited.

Hello World!

Program exited.

Locally:

~$ go vet foo.go
# command-line-arguments
./foo.go:10: unreachable code
~$ go run foo.go
Hello World!
~$ go version
go version go1.11 linux/amd64
~$
@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

It seems like const declarations are bound to the function scope differently than declared variables. If you change to hello := "Hello World !", it works as expected.

In any case, I don't think we should fix this. The code is bug-prone and should be corrected.

/cc @mvdan if there is anything else to do here.

@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Vet should never have false positives, but I too don't think this is one. Always jumping over a number of statements doesn't seem to ever make sense, even if that can still be used to declare constants.

In other words - has anyone ever encountered this in real code? If not, I think we should just close this issue until someone does.

@beoran

This comment has been minimized.

Copy link

commented Sep 20, 2018

This is indeed code go vet should complain about, but I think go vet should, in this case, not complain about "unreachable code" but something more specific in the vein of "goto used to jump over const definitions". schmichael, would be more in line with what you expected to happen?

@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Like @mvdan says, I don't think we should try to optimize for cases that does not occur in real code. If it was indeed found in real code, I would like to understand why.

@schmichael

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@mvdan: In other words - has anyone ever encountered this in real code? If not, I think we should just close this issue until someone does.

I kind of did? I wanted to simulate a pause here: https://github.com/hashicorp/nomad/blob/a333efb8369540081e1f79514e44bb93e259d2e3/client/allocrunnerv2/taskrunner/task_runner.go#L271

So I added the following code:

		dur := 30 * time.Second
		tr.logger.Debug("sleeping", "duration", dur)
		time.Sleep(dur)

Which gave me the expected error:

# github.com/hashicorp/nomad/client/allocrunnerv2/taskrunner
./task_runner.go:271:9: goto RESTART jumps over declaration of dur at ./task_runner.go:278:7

Since this was temporary testing code I figured a const might work since they're evaluated at compile-time (as opposed to runtime like assignments), so I changed the code to:

		const dur = 30 * time.Second
		tr.logger.Debug("sleeping", "duration", dur)
		time.Sleep(dur)

Of course I wanted to disgust and horrify my coworkers with this clever hack, so I created the simplified playground posted in the ticket and saw the vet error.

@beoran: ...something more specific in the vein of "goto used to jump over const definitions"

SGTM.

That being said it's probably not worth worrying about as @agnivade pointed out. I'd be fine with closing this until someone comes up with a more compelling reason to fix it.

@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

But in your task_runner.go code, did you get vet warnings for unreachable code ? As I see, the goto RESTART is inside an if condition. So vet shouldn't complain. The example that you posted in the issue unconditionally jumps to fmt.Println, so there's a difference.

@schmichael

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@agnivade You're right! No vet warnings in my real example! Sorry for the noise!

@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

No worries. Closing as discussed above.

@agnivade agnivade closed this Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.