Interesting. When I open a file for reading, I am ignoring the error returned by Close, and it's okay to do so. But when I open a file for writing, it is important to check the error from Close, because it could report an I/O error that a previous call to write did not. The error comes directly from the operating system. In both cases, the error comes from (*os.File).Close. Could this proposal account for this somehow?

@acln0 That is a good point. I had an earlier more complex idea that handled that case, but I'm not sure that this one does. The tool could special case the os.(*File).Close method, but I'm not sure whether other special cases would be required.

Given that a lot of existing code does not distinguish between ignoring errors by assigning to _ and ignoring errors by not binding a variable at all, I suspect that this analysis would need to be guarded by the go language version used to compile each package.

Even then, I don't think this proposal is in line with the Go 2 Transitions document: in order for tests to pass successfully, we would be de facto redefining the meaning of failing to bind a variable of type error — from “ignore the error” to “a possibly-undiagnosed run-time error”.

Moreover, unlike some of the static-checking proposals, this proposal would not catch accidentally-ignored return values of types other than error.

(For example, it would not catch ignored return-values from functional-style methods such as time.Time.Add, which return new values rather than updating the receiver in-place.)

@bcmills I'm not proposing adding something to the language or to go vet. I'm proposing a separate tool that does a dynamic analysis akin to the race detector. It would be invoked separately, not as part of a normal build. There are no transition or language version issues here.

I agree that this tool, if it is possible at all, does not solve all problems.

As far as I can see, either the tool will be adopted as authoritative (like the race detector) — in which case it is de facto a language change (if not formally one) — or it will not be adopted as authoritative and package authors will not consistently accept patches to fix violations. If the latter, I don't think the dynamic analyzer would be worth its cost: its rate of false-positives would remain high, and I suspect that too many users would tend to dismiss its reports as false-positives rather than investigating and fixing them.

Dynamic analyses are most effective when they have no false-positives.

I agree that the tool must have no false positives. @acln0 pointed out a potential false positive, and I agree that that case must be fixed before we could possibly create this tool.

Did you have other false positives in mind? You mentioned the distinction between assigning to _ and not assigning the result anywhere, but I don't see how that matters. That leads to false negatives (error hidden by unintended assignment to _), not false positives.

I'm concerned about both false-positives (intentionally-ignored errors that are flagged by the analyzer) and false-negatives (unintentionally-ignored errors that are not flagged), but I'm much more concerned about the false positives because they will encourage users to ignore or discount the analyzer.

In addition to the known Close cases, I suspect that intentionally-ignored errors will be common with things like (*exec.Cmd).Wait (such as the one here, waiting after an error is already known), (*os.Process).Signal (may be best-effort), (flag.Value).Set (may be best-effort), ioutil.WriteFile (may be used in tests, where a failed write will fail the test later anyway), os.RemoveAll (may be used best-effort in tests), os.Mkdir (may fail harmlessly if the directory already exists, and existence may be checked later), and similar user-defined APIs. In some cases, those will be marked by assigning to _. However, in others they may be marked by a comment, or by no comment at all (if the reason for ignoring the error is obvious to human readers).

If this were actually a language change, we would say that all of the intentionally-ignored non-nil errors must be updated with some kind of explicit marking as of some particular Go version, so that existing code that lacks such markings would produce no false-positives and would not need to be updated. However, as a dynamic analysis without a corresponding language change, we would have no way to distinguish these kinds of intentional-but-unmarked call sites from accidentally-unmarked sites in new code.

I'm not entirely convinced of the specific mechanic, but the underlying idea of detecting such things has appeal.

Another possible approach might be something annotation-ish. A given function which returns an error can usually be identified as one of three types:

(1) May return an error you should not ignore.
(2) Definitely can't return an error you can safely ignore.
(3) Passes on the ignorability of an error from something else.

So, for instance, fmt.Fprintf has the error-ignorability of the target's Write(), which means that in some cases, you can statically determine that no error is possible.

I suppose one strategy might be a function annotation like //go:errcheck which indicates that the annotated function's error type must be "checked". Users can still _ it, but they can't just ignore it entirely; they have to indicate that they're aware of it.

But thinking about it: At that point, perhaps it would also be useful to consider doing this to non-errors. I think some functions may return a value which it's necessarily a bad idea to ignore, because there's a necessary cleanup which can't be done without the return value. So possibly what's wanted is a per-return annotation for "it is an error not to use this value"?

Sorta wild ideas here, but maybe they'll be useful.

Needs more thought.