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

x/tools/gopls: track uses with fillreturns adding error returns #47637

Open
stamblerre opened this issue Aug 11, 2021 · 1 comment
Open

x/tools/gopls: track uses with fillreturns adding error returns #47637

stamblerre opened this issue Aug 11, 2021 · 1 comment

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 11, 2021

Fillreturns uses any in-scope variables when they match the type, but this doesn't work well with error returns.
An example of this is found in https://www.youtube.com/watch?v=6r08zGi38Tk around the 11:20 mark. The err return variable has already been used in an earlier return statement, but we still return it again below. It would be nice to check that it had already been used and use nil instead.

@matoous
Copy link

@matoous matoous commented Aug 29, 2021

There are some edge cases that would need to be solved, e.g. (with a grain of salt):

car, err := getCar()
if errors.Is(err, ErrNotFound) {
    return nil, err
}
car.SetDefaults()
return // What do we suggest here?

In this case, the user might still want to return err instead of nil but the err has been already returned once. E.g. the the variable has already been used in an earlier return statement most likely won't work. A lot of cases would be probably covered by checking for

car, err := getCar()
if err != nil {
    return nil, err
}
car.SetDefaults()
return // We can suggest `nil` for err as the error has been "completely" used

Not sure if that makes sense.

Anyway, I think the changes (if any) need to be made here : https://github.com/golang/tools/blob/master/internal/lsp/analysis/fillreturns/fillreturns.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants