Go: Add query to check for deferred calls to functions which may return errors

[mbg]

A simple query which checks that there are no deferred calls to functions which may return errors, since it prevents errors from being handled.

[github-actions]

QHelp previews:

go/ql/src/InconsistentCode/DeferredErrorCall.qhelp

Deferred call may return an error

When calling a function which may return an error, we should check whether an error has occurred and handle it in some meaningful way. Deferring a function call does not allow us to perform such a check. Therefore, calls to functions which may return errors should not be deferred.

Recommendation

Examine the deferred function call carefully to check whether any errors that may arise should be handled explicitly.

Example

In the following example, a call to io.WriteLine is deferred with the intention of appending some data to a file after some other data has been written to it. However, the call may not succeed and return and error, as demonstrated in the other use of io.WriteLine.

package main

import (
	"io"
	"log"
	"os"
)

func example() {
	defer io.WriteString(os.Stdout, "All done!")

	if _, err := io.WriteString(os.Stdout, "Hello"); err != nil {
		log.Fatal(err)
	}
}

To correct this issue, do not defer the function call that may return an error and handle the error explicitly instead:

package main

import (
	"io"
	"log"
	"os"
)

func example() {
	if _, err := io.WriteString(os.Stdout, "Hello"); err != nil {
		log.Fatal(err)
	}

	if _, err := io.WriteString(os.Stdout, "All done!"); err != nil {
		log.Fatal(err)
	}
}

References

• The Go Programming Language Specification: Defer statements.
• The Go Programming Language Specification: Errors.

[owen-mc]

Looks great.

[owen-mc]

Oh, I just realised that it needs a change note. Luckily there's an automatic check for that. Oh, and it needs docs review. I think you have to add the ready-for-doc-review label and they will come and do it.

[hubwriter]

Docs review: one typo, otherwise LGTM 👍

[sidshank]

Would it be possible to present a code example that preserves the behaviour we get from the usage of defer, while still handling any errors that that callee may produce i.e. what if I wanted to defer and also handle errors?

[mbg]

Great question! It seems like there are techniques for doing this by deferring an anonymous function which captures and writes to an error variable, that is then returned by the parent function. I will modify the example to make use of that technique.

[mbg]

From running this query on LGTM, it seems that it is currently quite noisy since it seems like a common (anti?)-pattern to defer calls to .Close() for file handles, which is probably fine for read-only handles, but not for writable ones. @smowton and I discussed this and I will experiment a bit more to see if I can come up with a less noisy query.

[mbg]

Since this query is very noisy and I am now working on more specific ones that are more targeted, I will close this PR for now in favour of new ones for the new queries as they are ready.