code review 97480043: go.tools/cmd/vet: add a check for finalizers that refer...

go.tools/cmd/vet: add a check for finalizers that reference their object This check guards against an accidental direct reference to the object being finalized in the finalizer, instead of using the finalizer's argument. This check, as implemented currently, checks only finalizers specified as function literals. Fixes issue 7546.

[robryk, 2014-05-14 16:44:08]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.tools

[r, 2014-05-14 17:33:24]

We are in feature freeze pending the Go 1.3 release. Please send this again once 1.3 is out. Also spell check your CL description.

[gobot, 2014-05-14 17:33:44]

R=golang-dev (assigned by r@golang.org)

[r, 2014-06-13 17:37:14]

Please sync your client and update your changes to the new registration mechanism used by vet.

[gobot, 2014-06-16 19:50:30]

R=r@golang.org (assigned by r@golang.org)

[robryk, 2014-06-18 17:18:54]

On 2014/06/13 17:37:14, r wrote: > Please sync your client and update your changes to the new registration > mechanism used by vet. Done.

[josharian, 2014-06-19 18:55:15]

As a data point, I ran this over a fairly large corpus of public code. It flagged four instances: https://github.com/gocircuit/circuit/blob/master/kit/x/file/file.go#L141 https://github.com/akhenakh/hunspellgo/blob/master/hunspellgo.go#L31 https://github.com/bluepeppers/libsg/blob/master/golibsg/sgimage.go#L76 https://github.com/losinggeneration/hge-go/blob/master/hge/resource/resource....

[r, 2014-06-19 19:59:59]

And how many finalizers appear in that same corpus? I think this is a rare problem but one that's hard to diagnose and suitable for vet to catch. -rob 

[josharian, 2014-06-19 20:15:14]

> And how many finalizers appear in that same corpus? 1152 > I think this is a > rare problem but one that's hard to diagnose and suitable for vet to > catch. SGTM. I was unsure how to interpret the data, which is why I didn't draw any conclusions from it. A few code nitpicks coming shortly...

[josharian, 2014-06-19 20:17:08]

https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014 https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:48: f.Badf(n.Pos(), "call to runtime.SetFinalizer with %d args", len(n.Args)) Add a test for this? https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:60: if arg.Obj == nil { May as well combine this with the !ok arg type assertion check a few lines earlier. https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... File cmd/vet/testdata/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... cmd/vet/testdata/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014 https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... cmd/vet/testdata/finalizers.go:18: runtime.SetFinalizer(f, func(x *finalized) { _ = f }) // ERROR "reference to f within f's finalizer" If you put the body on a separate line, it'd be clearer that the error message is attached to the reference to f rather than to the call to SetFinalizer.

[robryk, 2014-06-19 21:38:49]

https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/06/19 20:17:08, josharian wrote: > 2014 Done. https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:48: f.Badf(n.Pos(), "call to runtime.SetFinalizer with %d args", len(n.Args)) On 2014/06/19 20:17:08, josharian wrote: > Add a test for this? Done. https://codereview.appspot.com/97480043/diff/100001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:60: if arg.Obj == nil { On 2014/06/19 20:17:08, josharian wrote: > May as well combine this with the !ok arg type assertion check a few lines > earlier. Done. https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... File cmd/vet/testdata/finalizers.go (right): https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... cmd/vet/testdata/finalizers.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/06/19 20:17:08, josharian wrote: > 2014 Done. https://codereview.appspot.com/97480043/diff/100001/cmd/vet/testdata/finalize... cmd/vet/testdata/finalizers.go:18: runtime.SetFinalizer(f, func(x *finalized) { _ = f }) // ERROR "reference to f within f's finalizer" On 2014/06/19 20:17:08, josharian wrote: > If you put the body on a separate line, it'd be clearer that the error message > is attached to the reference to f rather than to the call to SetFinalizer. Done.

[josharian, 2014-06-19 22:04:47]

The code looks good. Want to add an entry to doc.go? Sorry for missing these earlier. https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:13: register("finalizers", "check that the finalizers don't retain references to their object", checkFinalizer, callExpr) Hmm. How about "check that finalizer functions don't refer to their object"?

[robryk, 2014-06-19 22:11:40]

Added an entry in doc.go, too. https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/120001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:13: register("finalizers", "check that the finalizers don't retain references to their object", checkFinalizer, callExpr) On 2014/06/19 22:04:46, josharian wrote: > Hmm. How about "check that finalizer functions don't refer to their object"? Thanks; done. My wording is usually weird.

[josharian, 2014-06-19 22:27:13]

LGTM but I'll wait for r before submitting One wording tweak before r looks... https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go#newcode152 cmd/vet/doc.go:152: finalize. This causes the object to never be unreferenced and thus, s/they should finalize/they finalize/ s/never be unreferenced and thus,//

[robryk, 2014-06-19 22:29:02]

https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/97480043/diff/70003/cmd/vet/doc.go#newcode152 cmd/vet/doc.go:152: finalize. This causes the object to never be unreferenced and thus, On 2014/06/19 22:27:13, josharian wrote: > s/they should finalize/they finalize/ > s/never be unreferenced and thus,// Done.

[r, 2014-06-19 22:35:39]

https://codereview.appspot.com/97480043/diff/150001/cmd/vet/doc.go File cmd/vet/doc.go (right): https://codereview.appspot.com/97480043/diff/150001/cmd/vet/doc.go#newcode151 cmd/vet/doc.go:151: Finalizer functions that refer directly to the objects that they finalize. Go doesn't have objects. We all know what you mean, but let's avoid the term. s/object/item/ here and on the next line. https://codereview.appspot.com/97480043/diff/150001/cmd/vet/finalizers.go File cmd/vet/finalizers.go (right): https://codereview.appspot.com/97480043/diff/150001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:13: register("finalizers", "check that the finalizer functions don't refer to their objects", checkFinalizer, callExpr) check that finalizer functions don't reference the item they are finalizing https://codereview.appspot.com/97480043/diff/150001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:35: // f is a function literal that holds a reference to x. // checkFinalizer looks for calls to runtime.SetFinalizer(x, f) // where f is a function literal that references x. https://codereview.appspot.com/97480043/diff/150001/cmd/vet/finalizers.go#new... cmd/vet/finalizers.go:48: f.Badf(n.Pos(), "call to runtime.SetFinalizer with %d args", len(n.Args)) this should just be Warnf, because if the code is compilable (which is not something we're checking), "runtime" is not the one we expect. unlikely but it's not what we're looking for. https://codereview.appspot.com/97480043/diff/150001/cmd/vet/testdata/finalize... File cmd/vet/testdata/finalizers.go (right): https://codereview.appspot.com/97480043/diff/150001/cmd/vet/testdata/finalize... cmd/vet/testdata/finalizers.go:24: runtime.SetFinalizer(f, f.fn, f.fn) // ERROR "call to runtime.SetFinalizer with 3 args" d

[josharian, 2014-07-03 23:39:32]

Ping, robryk.

[josharian, 2014-08-12 18:22:12]

Ping, robryk. Any interest in, er, finalizing this? If not, mind if I finish this in a new CL?

[gobot, 2014-12-19 05:19:28]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/97480043 is best) in the description in your new CL. Thanks very much.