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 appendending 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]

Suggested change

	sig = defer.getCall().getCalleeExpr().getType().(SignatureType) and
	sig = defer.getCall().getCalleeExpr().getType() and

(I see ql-for-ql has also picked this up.)

[mbg]

Fixed in c907a9e

[owen-mc]

getCalleeName isn't always defined, so we would lose any alerts for those cases, though I can't remember exactly what they are at the moment. Is it worth trying to provide a different string when getCalleeName doesn't exist, to cover those cases? People defer anonymous functions quite a bit, though it would be weird to write an anonymous function with return values and then defer it.

[mbg]

🤷🏻‍♂️ I have improved this in 5620d24 in any case.

[owen-mc]

If you want to be thorough, you can cover some more cases: (1) assign a function to a variable and use the variable in the defer statement; (2) defer a function literal.

[mbg]

Added in 5620d24

[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.