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

AppendInvoke silently drops errors subtly #61

Closed
thomasdesr opened this issue Aug 6, 2022 · 3 comments
Closed

AppendInvoke silently drops errors subtly #61

thomasdesr opened this issue Aug 6, 2022 · 3 comments

Comments

@thomasdesr
Copy link

thomasdesr commented Aug 6, 2022

Thanks for putting together this library!

The behavior I found surprising was that if you do not use the less-common pattern where you define variables in the function return signature, that using defer multierr.AppendInvoke(&err, someInvoker) will not have the error returned by someInvoker included in the error that is returned by the function.

Example: https://go.dev/play/p/JoSVtFwapQL

Note how outer20 is missing foo from its list of errors.

I guess Go's runtime is returning (or copying the return value) before the defer is running when the return value is defined in the function body vs in the function signature.

@rabbbit
Copy link

rabbbit commented Aug 6, 2022

This seems to be expected given how defer works in Go.

https://go.dev/play/p/eF8TmyEnTxt

func t1() int {
	var i int
	defer func() {
		i++
	}()
	return i
}

func t2() (i int) {
	defer func() {
		i++
	}()
	return i
}

func main() {
	fmt.Println(t1())
	fmt.Println(t2())
}

Results in:

0
1
Program exited.

Even playing with pointers does not work around this:

func t3() int {
	var i int
	ii := &i
	defer func() {
		*ii++
	}()
	return *ii
}
// returns 0

My brief reading of the spec here https://go.dev/ref/spec#Defer_statements calls out that you need to be using named returns for defers to modify the return value:

For instance, if the deferred function is a function literal and the surrounding function has named result parameters that are in scope within the literal, the deferred function may access and modify the result parameters before they are returned.

@abhinav
Copy link
Collaborator

abhinav commented Aug 12, 2022

This is a good point, but yeah @rabbbit is right, because of how defers work, the library can't address this.
We can do two things here:

  • Update the documentation of AppendInvoke to loudly state that if you're using it inside a defer, the variable must be a named return.
  • In the future, we can look into building a go/analysis-based linter to catch these cases since this is pretty easily lintable.

abhinav added a commit that referenced this issue Aug 12, 2022
As discussed in #61, if you attempt to use `defer multierr.AppendInvoke`
with an error that is not a named return, the system will lose the
error.

```go
func fails() error { return errors.New("great sadness") }

func foo() error {
	var err error
	defer multierr.AppendInvoke(&err, multierr.Invoke(fails))
	return err
}

func main() {
	fmt.Println(foo()) // nil
}
```

https://go.dev/play/p/qK4NR-VYLvo

This isn't something the library can address because of how defers work.

This change adds a warning about the error variable being a named return
in all places where we suggest use of multierr with defer.

While we're at it, this makes use of the new `[Foo]` godoc syntax to
generate links to other functions in the package in "See Foo"
statements in the documentation.
abhinav added a commit that referenced this issue Aug 12, 2022
As discussed in #61, if you attempt to use `defer multierr.AppendInvoke`
with an error that is not a named return, the system will lose the
error.

```go
func fails() error { return errors.New("great sadness") }

func foo() error {
	var err error
	defer multierr.AppendInvoke(&err, multierr.Invoke(fails))
	return err
}

func main() {
	fmt.Println(foo()) // nil
}
```

https://go.dev/play/p/qK4NR-VYLvo

This isn't something the library can address because of how defers work.

This change adds a warning about the error variable being a named return
in all places where we suggest use of multierr with defer.

While we're at it, this makes use of the new `[Foo]` godoc syntax to
generate links to other functions in the package in "See Foo"
statements in the documentation.
abhinav added a commit that referenced this issue Aug 12, 2022
As discussed in #61, if you attempt to use defer multierr.AppendInvoke
with an error that is not a named return, the system will lose the
error.
func fails() error { return errors.New("great sadness") }

func foo() error {
	var err error
	defer multierr.AppendInvoke(&err, multierr.Invoke(fails))
	return err
}

func main() {
	fmt.Println(foo()) // nil
}
https://go.dev/play/p/qK4NR-VYLvo
This isn't something the library can address because of how defers work.
This change adds a warning about the error variable being a named return
in all places where we suggest use of multierr with defer.
While we're at it, this makes use of the new [Foo] godoc syntax to
generate links to other functions in the package in "See Foo"
statements in the documentation.
@abhinav
Copy link
Collaborator

abhinav commented Mar 20, 2023

Closing because documentation was updated in #63, and there's nothing we can do at the library level besides that.

@abhinav abhinav closed this as completed Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants