proposal: x/tools: annotation to generalize lostcancel for other kinds of cleanup function

[adonovan]

Proposal Details

The context.WithCancel function returns a "cancel" function that must be called on all execution paths. It's easy to forget to do so, especially in early-return error paths, leading to a context leak. This pattern is quite common: the gopls codebase has many functions that return a release function that decrements a reference count [example], and we've had a number of bugs from failure to follow the proper discipline. Kubernetes has many of its own too. Searching for functions that return a value of type func(), often named something like cleanup, release, stop, close, or shutdown, it's easy to find more instances of this pattern.

Go vet currently has a lostcancel analyzer that reports problems of this sort, and it is not hard to generalize it to handle other functions besides context.WithCancel. (I implemented it over the weekend.) But the hardest part of the problem is reliably identifying which functions are cleanup functions, and which don't exactly follow the discipline [example]. The name is not a reliable clue, and many functions don't name their result variables.

The actual type of the function returned by WithCancel is type CancelFunc func(), a named type. This suggests an approach to generalization: allow modules to define their own clean-up function types analogous to CancelFunc, and then register them with the analyzer using the annotation mechanism that we plan to design and develop this year.

Here's a sketch of what that might look like:

package p

import "golang.org/x/tools/analysis/annotations/lostcancel"

type Resource struct { ... }

// Acquire (on success) returns a resource.
// The caller must call the release function when they are finished using the resource.
func Acquire() (*Resource, ReleaseFunc, error) { ... }

func ReleaseFunc func() 

func _() {
    lostcancel.Register(CancelFunc(nil))
}

This proposal is obviously subordinate to the annotations proposal, but one immediate question is: should the standard library provide a standard CancelFunc? Long time viewers may remember a similar discussion around a standard NoCopy annotation, which ended in a "no" decision. But as we think about generalizing the patterns of vet checking beyond the standard library, we may want to revisit whether the standard library should provide a lightweight package for declaring the most important annotations. This might have benefits in uniformity and boilerplate reduction, and would also allow the standard library to use the annotations, which (if they lived alongside the analyzers) would otherwise be unavailable to it.

package annot // hypothetical new std annotations package

// A CleanupFunc is a function that must be called to clean up a resource.
//
// Here is the typical pattern:
//
//        resource, cleanup, err := p.Acquire()
//        if err != nil { return err }
//        defer cleanup()
//        ... use resource ... 
//
// Failure to call cleanup (or at least mention it) on all control paths not
// dominated by `err != nil` may be reported as an error by the `lostcancel` checker.
type CleanupFunc func()

Related:

• proposal: text/template: annotations to enable static checking of syntax and semantics #64543
• proposal: cmd/vet: add declarative support for identifying functions for printf check #58340
• proposal: cmd/vet: add new flag to support stdmethod checks for specific methods #52445
• https://go.dev/cl/489835

[earthboundkid]

Is there a reason not to do this via a magic comment? That would be my first instinct as a user. Something like //go:vet nolostcancel on the return closure type.

[jimmyfrasche]

Maybe //vet:noLostCancel or go-vet: so it doesn't imply that it has runtime support?

Tangentially, there doesn't seem to be an API for getting these special comments anywhere that I can find. It would be nice to not have to parse them out yourself.

The link to the NoCopy issue does not work. It looks like that was #8005

[adonovan]

There are many reasons to prefer annotations over magic comments. The primary one is that they are checked by the compiler and type checker, so they cannot be misspelled, or lost during migrations, renamings, etc. In addition they are self-documenting, since you can jump to their definition, and enumerable using your IDE's references query, making it easy to find examples of use.

[jimmyfrasche]

I agree in general though I'm not especially enthused by

func _() {
    lostcancel.Register(CancelFunc(nil))
}

If you could write type A[T any] = T, you could do type MyCleanupFunc annotations.CleanupFunc[func()]. That loses some of the type checking but vet could also provide an error for that.

[adonovan]

If you could write type A[T any] = T, you could do type MyCleanupFunc annotations.CleanupFunc[func()]. That loses some of the type checking but vet could also provide an error for that.

That's a neat way to avoid an extra line of declaration, but I don't think the cleanup func actually needs to vary its function type (if that were even expressible).

I'm open to suggestions of better ways to write a no-op Go declaration to express "type T is special".

[earthboundkid]

There's already a common convention of using package level var _ = for type checks like Impl implements Interface. It would be nice to be able to fit this into that and not need an init func. Ideally, it would all compile away.

[timothy-king]

@earthboundkid It is not clear you cannot do var _ = ... yet. I don't think the proposal specifies what lostcancel.Register(...) returns. If it returns something, this is clearly doable. It might be less clear that lostcancel.Register is a not a run time operation. Though I am bit doubtful that func _() { ... } will be obvious to most Go programmers.

[earthboundkid]

I misread the underbar func as an init func, for example.

[adonovan]

No-one seems to like the idea of using Go syntax for annotations, and there is precedent for using directive comments of the form //go:fix inline (e.g. #32816). So I'm open to alternative notions for the concrete syntax in this proposal. For example:

// A CleanupFunc is a function that must be called to clean up a resource.
//
// Here is the typical pattern:
//
//        resource, cleanup, err := p.Acquire()
//        if err != nil { return err }
//        defer cleanup()
//        ... use resource ... 
//
// Failure to call cleanup (or at least mention it) on all control paths not
// dominated by `err != nil` may be reported as an error by the `lostcancel` checker.
//
//vet:lostcancel
type CleanupFunc func()

func Acquire() (*Resource, CleanupFunc, error) { ... }

func f() {
    Acquire() // lostcancel: CleanupFunc is not used on all paths
    return
}

There remain many open questions about the exact spelling of the directive (all three components), and whether it should annotate a function type (such as CleanupFunc) or a function such as Acquire.