Detect return parameters that are just a receiving parameter

[mvdan]

In other words, when the values must already be in the hands of the caller.

For example, the receiver:

func (f *foo) bar() foo {
    doWork()
    return f
}

A receiver's field:

func (f *foo) bar() int {
    doWork()
    return f.intField
}

A received parameter:

func bar(n int) int {
    doWork()
    return n
}

Of course, the value must not have been modified under any of those cases.

[mvdan]

Hmm, the first type could be problematic with projects that use the idiom of chaining methods, like in f.bar().etc(). We probably want to leave it out for now.

[kevinburke]

I could imagine a situation where I define a method in anticipation of it changing later, say,

func (u *User) Suspended() bool {
    return u.Canceled
}

where later it might turn into (once a new field gets added):

func (u *User) Suspended() bool {
    return u.Canceled && !u.SuspendedAt.IsZero()
}

The first way I'd have to go find every place Canceled was called and decide whether I really meant Suspended and change it.

[mvdan]

You're right, fields is also problematic.

I think I'll just go for return parameters that are always one of the receiving parameters (minus method receivers).

[tehsphinx]

Also consider this case:

func foo (n *someStruct) *someStruct {
    n.Val1 = "bar"
    return n
}

The return value is not necessary.

[mvdan]

@tehsphinx ah yes, modifying what's behind a pointer as long as the pointer itself is not modified means that the return value can still be unnecessary. Thanks for pointing that out.

[mvdan]

Implemented this, but starting to have some doubts that it's actually useful. On the latest tip, these are the newly found warnings:

container/list/list.go:92:39: result 0 (*container/list.Element) is just parameter e
container/list/list.go:109:35: result 0 (*container/list.Element) is just parameter e
crypto/rsa/rsa.go:348:54: result 0 (*math/big.Int) is just parameter c
crypto/tls/conn.go:163:47: result 0 (error) is just parameter err
crypto/tls/conn.go:537:50: result 0 (*crypto/tls.block) is just parameter b
crypto/tls/tls.go:194:40: result 1 (error) is just parameter err
go/ast/commentmap.go:246:46: result 0 (go/ast.Node) is just parameter new
go/build/build.go:1125:61: result 1 (map[string][]go/token.Position) is just parameter m
go/parser/parser.go:227:35: result 0 (*go/parser.parser) is just parameter p
html/template/template.go:367:35: result 0 (*html/template.Template) is just parameter t
internal/testenv/testenv.go:228:33: result 0 (*os/exec.Cmd) is just parameter cmd
math/big/int.go:231:43: result 1 (*math/big.Int) is just parameter r
math/big/int.go:289:43: result 1 (*math/big.Int) is just parameter m
os/file.go:266:39: result 1 (error) is just parameter err
runtime/trace.go:635:67: result 1 (*runtime.traceBufPtr) is just parameter bufp
text/template/helper.go:21:35: result 0 (*text/template.Template) is just parameter t

None of them seem particularly good warnings. Thoughts? Perhaps run the tool on your codebases, grep for this new warning, and let me know if any of the warnings are actually useful.

[mvdan]

I have found the pattern of false positives. It's usually like:

func transformFoo(f *Foo) *Foo {
    f.Field = "bar"
}

[...]

func SomeCode(f *Foo) {
    SomeOtherCode(transformFoo(f))
    // or: return transformFoo(f)
}

I've analysed this check in other codebases as well as std cmd and my own, and a vast majority of them aren't useful warnings. A few could be useful, and we could skip the check for the patterns above, but I struggle to see how it could be useful with even all of those.

Disabling the check for now. If anyone wants to give it a test-run, they can always revert 18c5021 (or go to the commit right before it).