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: named return values produce functionally different code #21049

Closed
aronatkins opened this issue Jul 17, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@aronatkins
Copy link

commented Jul 17, 2017

This might be a duplicate of #20859. Filing it separately because the discussion in #20859 is mostly about code-generation, not functional differences.

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

go version go1.8.1 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aron/dev/rstudio/connect/_vendor:/Users/aron/dev/rstudio/connect"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ts/s940qvdj5vj1czr9qh07fvtw0000gn/T/go-build120790787=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

https://play.golang.org/p/Ig-Jnvz07B

package main
import (
  "errors"
  "fmt"
)
func ModifyErr(perr *error) {
  *perr = errors.New("this err was modifed")
}
func Foo() (err error) {
  err = errors.New("Foo error")
  defer ModifyErr(&err)
  return err
}
func Bar() error {
  var err error
  err = errors.New("Bar error")
  defer ModifyErr(&err)
  return err
}
func main() {
  fmt.Printf("Foo: %s\n", Foo())
  fmt.Printf("Bar: %s\n", Bar())
}

What did you expect to see?

The same behavior from both Foo and Bar; probably Bar is correct.

What did you see instead?

This code produces:

Foo: this err was modifed
Bar: Bar error
@aronatkins

This comment has been minimized.

Copy link
Author

commented Jul 17, 2017

The example given was derived from some error logging/reporting code. Here is an example that uses primitives (int) and might make for simpler analysis.
https://play.golang.org/p/nQ_z0ScUsm

On the playground, it prints:

Foo: 42
Bar: 17
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

This is working as intended.

These are actually different.

In your Foo func, the return statement is modifying the named return value before it returns, per the spec:

https://golang.org/ref/spec#Return_statements

A "return" statement that specifies results sets the result parameters before any deferred functions are executed.

@bradfitz bradfitz closed this Jul 17, 2017

@aronatkins

This comment has been minimized.

Copy link
Author

commented Jul 17, 2017

@bradfitz Both Foo and Bar are attempting to modify err; only the Foo example succeeds (the one with the named return value).

@aronatkins

This comment has been minimized.

Copy link
Author

commented Jul 17, 2017

The only difference between Foo and Bar is how err is declared.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

No, that's not true. They really are different err variables.

The err in defer ModifyErr(&err) in Foo is referring to the return value err.

The err in defer ModifyErr(&err) in Bar is referring to the local variable err.

Think of your Bar func like this to see what it's actually doing:

func Bar() (err error) {
	var otherErr error
	otherErr = errors.New("Bar error")
	defer ModifyErr(&otherErr)
	err = otherErr  // runs before ModifyErr
	return
}

Again, this is all per the language spec I linked above. See also the scope sections.

@aronatkins

This comment has been minimized.

Copy link
Author

commented Jul 17, 2017

@bradfitz Thanks. That example helped. I expected it had something to do with the scoping of the named return parameter. I didn't see that in the Bar case, there's an implicit result parameter (which can often be optimized away).

@frankandrobot

This comment has been minimized.

Copy link

commented Jul 18, 2017

I think you guys are missing the point---if you forget to name the return result, you get different results. That is, a "fat finger" typo can produce in subtle bugs. Incidentally, it also very easy to overlook in a code review. Are there any automated tooling that can come to the rescue?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

@frankandrobot It's true that this is something one needs to learn about Go, but I don't think it is any more subtle than other aspects of the language. The odd thing is naming a result parameter: when you name a result parameter, you have to consider any references to that result parameter in deferred functions. That is one of the major uses of named result parameters: being able to change them in a deferred function.

@frankandrobot

This comment has been minimized.

Copy link

commented Jul 18, 2017

It's not a matter of learning @ianlancetaylor---the concept of named return vars can be grokked in a few minutes. It's a matter of doing. Forgetting to name the return variable is very easy to do, very easy to overlook in a code review, yet can produce subtle bugs.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

I would say that forgetting to name the result variable is the normal case.

I understand you to be saying is that if you want to do something fancy--to use a deferred function to change the result--then it is possible to accidentally refer to a local variable rather than to a named result variable. That is true. But I don't see that as any more subtle than any other sort of variable shadowing.

@frankandrobot

This comment has been minimized.

Copy link

commented Jul 18, 2017

Perhaps in this use case, "use a deferred function to change the result", there is an alternative pattern? Incidentally, the real use case is committing a database transaction and then ensuring any errors get propagated.

@golang golang locked and limited conversation to collaborators Jul 18, 2018

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.