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

Document named return constraint for defer #63

Merged
merged 1 commit into from Aug 12, 2022
Merged

Document named return constraint for defer #63

merged 1 commit into from Aug 12, 2022

Conversation

abhinav
Copy link
Collaborator

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

@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #63 (a61c151) into master (492b792) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #63   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          108       108           
=========================================
  Hits           108       108           
Impacted Files Coverage Δ
error.go 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Base automatically changed from go119 to master August 12, 2022 16:14
@sywhang
Copy link
Contributor

sywhang commented Aug 12, 2022

Looks like there's a merge conflict, but other than that lgtm

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
Copy link
Collaborator Author

abhinav commented Aug 12, 2022

Looks like there's a merge conflict, but other than that lgtm

I had stacked this on top of the other PR.
When you merge the base PR, GitHub updates the base branch for other PRs stacked on it.
But unless it's a merge-type commit (so not squash-and-merge),
what ends up on master is considered a different commit,
and so the original commits from the original base branch show up in the stacked PR and cause that conflict.
As far as I can tell, GitHub does not have a means of automatically rebasing the PR,
so you have to do it manually (which I just did).
(Yeah, this is a whole thing.)

@abhinav abhinav merged commit 80b07a7 into master Aug 12, 2022
@abhinav abhinav deleted the defer-doc branch August 12, 2022 16:28
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

Successfully merging this pull request may close these issues.

None yet

2 participants