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: the compiler should probably recognize runtime.Goexit #37193

Closed
perillo opened this issue Feb 12, 2020 · 7 comments
Closed

cmd/compile: the compiler should probably recognize runtime.Goexit #37193

perillo opened this issue Feb 12, 2020 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Feb 12, 2020

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

$ go version
go version go1.14rc1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/manlio/sdk/go1.14rc1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/manlio/sdk/go1.14rc1/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build423783759=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.14rc1 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.14rc1
uname -sr: Linux 5.5.2-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.31.
gdb --version: GNU gdb (GDB) 8.3.1

What did you do?

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

What did you expect to see?

The program to be compiled.

What did you see instead?

/prog.go:13:1: missing return at end of function

The Go compiler does not know that runtime.Goexit causes the function to exit, so the return statement is unreachable and not required.

Of course there are similar functions like os.Exit, but Goexit is part of the runtime, so it should probably be recognized by the compiler.

On the other hand, vet should know about runtime.Goexit and should report that the statements after Goexit() are not reachable.

The compile or vet commands should also report an error if a function used in the go statement has a non empty result in the signature, but this is a different issue and probably there is a reason why it does not report an error.

@josharian
Copy link
Contributor

The relevant bit of the spec is https://golang.org/ref/spec#Terminating_statements. I’d personally be reluctant to add runtime.Goexit; everything else in that list is lower level.

It is important for vet to use the same set of terminating statements as the compiler. Otherwise you’ll be forced to add a terminating statement in some cases (like this) to make your code compile but vet will then complain about it.

@zigo101
Copy link

zigo101 commented Feb 14, 2020

The Goexit function really acts as a low level API: #29226

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 15, 2020
@dmitshur dmitshur added this to the Backlog milestone Feb 15, 2020
@dmitshur
Copy link
Contributor

/cc @randall77 @griesemer per owners.

@griesemer
Copy link
Contributor

The definition of "terminating statements" is based strictly on properties of the language spec (which are unlikely to change), not APIs (which may change).

Also, even if we added runtime.Goexit it wouldn't be satisfactory anyway: It's not unlikely that runtime.Goexit is factored out into a function that does cleanups or perhaps prints additional error information; thus it is that function that will be called (possibly from multiple places) in a program. And unless we propagate the "exit" information to the caller, knowing that runtime.Goexit doesn't return won't be very helpful - it'll save a single return statement. That doesn't seem worth it.

More precisely, runtime.Goexit doesn't terminate the function, it terminates the goroutine that calls it. If we wanted to capture that correctly, we'd need to propagate that up the call stack, with extra rules. I am not convinced this is worth the extra complexity or the mixing of language spec and library APIs.

Not really a proposal, but leaving open for a decision by the proposal review commitee.

@griesemer griesemer added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 18, 2020
@elichai
Copy link

elichai commented Oct 6, 2020

I've found a bunch of times where I call t.Fatal in a testing utility but I still need to return, so instead I currently add switch{} after the fatal calls,
adding some attribute(like C++'s noreturn) or even type (like Rust's Never type(!)) would be really useful even if it would be limited to the standard library only

@breml
Copy link
Contributor

breml commented Oct 26, 2021

I also hit this case while working with Fatal/Fatalf from the testing package. While I agree, that adding the missing return statement after the call to Fatal is not that big of a deal, I still think it is confusing for some one reading the code, because it can lead to the conclusion, that the execution does not stop at the Fatal.

@ianlancetaylor
Copy link
Member

We made a decision a long time ago that the language is going to require a return statement at the end of a function that has results, and we wrote that into the language spec. We aren't going to add increasing complexity in the language spec in order to let people avoid writing return or panic(0). I'm going to close this issue so that people don't start to think that this might change in the future. Sorry.

@golang golang locked and limited conversation to collaborators Oct 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants