Compiler/GoVet assistance with ignored error return value

[Ugorji Nwoke]

tl;dr It will be nice if either the compiler or govet supports informing us when an error return value is ignored (ie not used or assigned to something, even _).

I know GO is averse to supporting compiler warnings. 

https://groups.google.com/forum/#!topic/golang-nuts/Jf-hUONY9xE

https://groups.google.com/forum/#!topic/golang-nuts/obbh9Ak-LzQ

https://groups.google.com/d/topic/golang-nuts/hiFZ9FDcTOQ/discussion

GO handles some "warnings" as errors because they don't want to have noise, but want to also notify users about things which could cause hard to track bugs. (e.g. declaring packages/variables/etc and not using it, etc).

One of the major areas of hard to track bugs I've seen while programming GO code is when we ignore the os.Error return value of a function. I've been careful, but have still been bitten by this a few times, and finding the problem took a while.

With languages that "recommend" exceptions, they don't really have this issue since a nice stack trace will always greet you, and you have to explicitly/knowingly ignore an exception (i.e. try {...} catch(...) {}).

Some ideas to handle it:

- Pass a compiler flag so that warnings are enabled. Warnings are *only* supported for a few situations where breaking compile is not an option e.g. when ignoring an important return value like error. (if you assign the return value to _, the warning goes away).

- Expand govet to support this. From looking at govet code, this may be harder, since it only looks at static source code one file at a time. However, this seems like the more natural location for this.

As we go into GO 1, I think this will be good to handle. I can see this being something that bites a lot of people when the floodgates are opened. 

[Ian Lance Taylor]

Ugorji Nwoke <ugo...@gmail.com> writes:

> *tl;dr It will be nice if either the compiler or govet supports informing

> us when an error return value is ignored (ie not used or assigned to

> something, even _).*

The problem is that for some functions it is just fine to ignore errors.
For others it is not. There is no single enforcement policy which fits
all cases. Perhaps there is a good change we can make here. However,
requiring everybody to write
_, _ = fmt.Printf("Hi!")
does not seem to me like a path that Go should follow.

Ian

[Ugorji Nwoke]

Yes, I agree that having the compiler fail due to this, or requiring everyone to assign values to _, is not a good path for GO to follow. I appreciate that there's not a quick answer to this.

In my post, I suggested 2 potential ways to handle it. 

Some ideas to handle it:
- Pass a compiler flag so that warnings are enabled. Warnings are *only* supported for a few situations where breaking compile is not an option e.g. when ignoring an important return value like error. (if you assign the return value to _, the warning goes away).
- Expand govet to support this. From looking at govet code, this may be harder, since it only looks at static source code one file at a time. However, this seems like the more natural location for this.

If GO had a policy for warnings, then this would be a good use-case for warnings (with some exceptions like fmt.XXX, etc). GO's current policy wrt warnings is hard and fast: no warnings. 

[Ian Lance Taylor]

Ugorji Nwoke <ugo...@gmail.com> writes:

> Yes, I agree that having the compiler fail due to this, or requiring
> everyone to assign values to _, is not a good path for GO to follow. I
> appreciate that there's not a quick answer to this.
>
> In my post, I suggested 2 potential ways to handle it.
>
>> Some ideas to handle it:
>> - Pass a compiler flag so that warnings are enabled. Warnings are *only*
>> supported for a few situations where breaking compile is not an option e.g.
>> when ignoring an important return value like error. (if you assign the
>> return value to _, the warning goes away).
>> - Expand govet to support this. From looking at govet code, this may be
>> harder, since it only looks at static source code one file at a time.
>> However, this seems like the more natural location for this.
>
>
> If GO had a policy for warnings, then this would be a good use-case for
> warnings (with some exceptions like fmt.XXX, etc). GO's current policy wrt
> warnings is hard and fast: no warnings.

Right, that is a policy which is simple to implement and easy to
explain.

I personally don't care for either of your ideas, because I don't think
that either of them addresses the problem of
_, _ = fmt.Printf("Hi")

They are both global approaches which apply to all the code in question.
But in the real world, some error returns should almost always be
checked, and some error returns may be checked under some circumstances.
That is, when considering whether to make it a warning/error if error
returns are not checked, you need to consider the function being called,
not just the function doing the calling. Your proposals only consider
the latter.

Compare to gcc's extended C language. There is no way in gcc to say
"check all return values." But there is a way to say "check the return
value of this function" (via __attribute__ ((warn_unused_result))). Of
course even this has its problems, and I don't recommend adopting it
into Go.

Ian

[Ugorji Nwoke]

Fair enough. I just wanted to put this on the team's mind. (I have a feeling this one will come back to the table later on.)

I acknowledge the idea that for some functions, it is ok ignoring the return code (e.g. fmt.XXX). Those can be included in an exception list, which the user calling govet can supplement.

The precedence to supplement is already supported by govet with implicit checks (Printf, Errorf, etc) and user-supplied supplements (via -printfuncs parameter e.g. Warnf). 

For most functions that return errors, GO's idiom seems to be "don't ignore them". When programmers do, it costs a lot of time to track down the bugs. Most times, the first thing that comes to mind when something goes wrong is: "I'm probably ignoring an error return somewhere". Any form of tool help will go a long way. 

P.S. Govet is currently extended to support checking canonical methods that look like they implement certain common interfaces. This may be another nice extension if we can figure out a solution. 

[Jonathan Amsterdam]

>  But there is a way to say "check the return
> value of this function" (via __attribute__ ((warn_unused_result))).  Of
> course even this has its problems, and I don't recommend adopting it
> into Go.

What are its problems? I've been lovin' it ever since I learned about
it a few months ago.

[Russ Cox]

On Wed, Nov 2, 2011 at 16:59, Jonathan Amsterdam <jbams...@gmail.com> wrote:
> What are its problems? I've been lovin' it ever since I learned about
> it a few months ago.

For one thing, someone decided it was a good idea to
tag perfectly reasonable C library calls like write(2) with
the warn_unused_result attribute, which is a royal pain.
I am expecting the next Ubuntu release to tag exit(2).

Russ

[Hein Meling]

Hi Russ,

Surely it would not be ideal to have to discard return values in cases like this. However, I would trust the Go team to be smarter than to add such annotations onto functions where it is not needed. But there are definitely cases where it is essential that the error be checked.

I would suggest to add a keyword or annotation (let's called it "checked" for the purposes of this discussion) in front of the error return part of the function signature, to indicate to the compiler to enforce that an error be received by the caller. 

The benefit of getting a compiler error for missing error handling on certain important functions by far outweighs the "overhead" imposed on us by some (hopefully few) poorly designed APIs. And actually, if some poorly designed API function turned out to have been incorrectly marked, it would not break an existing code base to remove the checked annotation in a future release.

:) Hein

[minux]

On Sun, Nov 4, 2012 at 8:29 AM, Hein Meling <hein....@gmail.com> wrote:

Surely it would not be ideal to have to discard return values in cases like this. However, I would trust the Go team to be smarter than to add such annotations onto functions where it is not needed. But there are definitely cases where it is essential that the error be checked.

I would suggest to add a keyword or annotation (let's called it "checked" for the purposes of this discussion) in front of the error return part of the function signature, to indicate to the compiler to enforce that an error be received by the caller. 

The benefit of getting a compiler error for missing error handling on certain important functions by far outweighs the "overhead" imposed on us by some (hopefully few) poorly designed APIs. And actually, if some poorly designed API function turned out to have been incorrectly marked, it would not break an existing code base to remove the checked annotation in a future release.

Don't do that. The compiler can't make guarantee that the error is correctly handled.

for example, given the gcc warn_unused_result attribute for function f, you can do the following

to trick gcc (-Wall -Wextra compiled without warning):

__attribute__((__warn_unused_result__)) int f(int x) { return x * 2; }

void g() {

    __typeof__(f(1)) y;

    y = f(1);

}

However, this surely isn't what the f()'s author expects -- he/she wants the user to act on the

return value, but this is not the case.

[Kevin Gillette]

I think this is an excellent job for a separate tool; when the compiler doesn't produce any warnings whatsoever right now, it takes exponentially more momentum and justification to add the first warning, compared to if you suggested the gcc team add yet another warning to their already long list. Beyond that, it doesn't seem like a hard problem to me:

A separate tool can:

1) Parse any number of configured or option-passed whitelist/blacklist files, of the form:

allow:

fmt

deny:

fmt Scan*

This would have a syntax similar to hgignore and gitignore files (using globs or regexes -- it doesn't really matter). The above will configure the tool to not report any unchecked errors for calls into the fmt package, unless it's a fmt.Scan* func being called. There are few enough white-listable cases that patterns may not be useful. White/blacklisting can happen on the package, type, or func/method level, and could borrow godoc's commandline syntax (which would hopefully be extended to handle methods).

When resolving funcs or methods, the most specific applicable rule (func/method, then type, then package) takes precedence, and where conflicting rules of the same specificity are present, blacklisting takes precedence. Default would be to blacklist anything without a rule, as that is aligned with the common philosophy to, when in doubt, check every error.

2) Scan requested source files and find all calls that return an error type that is ignored, and for each, unless the most applicable rule is to allow the error to go unchecked, report that file/line to stdout.

This design doesn't account for other potentially pernicious cases, like ignoring the isPrefix return value from `bufio *Reader ReadLine` (which in Go syntax would be `(*bufio.Reader).ReadLine`, I think.

[Hein Meling]

On Sunday, November 4, 2012 6:57:45 AM UTC+1, minux wrote:

On Sun, Nov 4, 2012 at 8:29 AM, Hein Meling <hein....@gmail.com> wrote:

Surely it would not be ideal to have to discard return values in cases like this. However, I would trust the Go team to be smarter than to add such annotations onto functions where it is not needed. But there are definitely cases where it is essential that the error be checked.

I would suggest to add a keyword or annotation (let's called it "checked" for the purposes of this discussion) in front of the error return part of the function signature, to indicate to the compiler to enforce that an error be received by the caller. 

The benefit of getting a compiler error for missing error handling on certain important functions by far outweighs the "overhead" imposed on us by some (hopefully few) poorly designed APIs. And actually, if some poorly designed API function turned out to have been incorrectly marked, it would not break an existing code base to remove the checked annotation in a future release.

Don't do that. The compiler can't make guarantee that the error is correctly handled.

IMO, that's beside the point. The compiler cannot reveal all kinds of other bugs either. The point is to let the programmer know that you really should handle this particular error. Not to prevent the programmer from ignoring it, if that is what the programmer wants. And by using _ to ignore such errors, the overhead is marginal for those cases.

 

for example, given the gcc warn_unused_result attribute for function f, you can do the following

to trick gcc (-Wall -Wextra compiled without warning):

__attribute__((__warn_unused_result__)) int f(int x) { return x * 2; }

void g() {

    __typeof__(f(1)) y;

    y = f(1);

}

I'm for sure not suggesting to use such a horrible notation with __ and (( )) all over... 

 

However, this surely isn't what the f()'s author expects -- he/she wants the user to act on the

return value, but this is not the case.

Again, the point is not to prevent the programmer from doing something stupid, but to let that be a conscious decision made by the programmer, e.g. by using _ to discard the error. However, if the compiler does not check/warn about this, it is quite easy to forget!! 

All the best,

:) Hein 

[Hein Meling]

Well, I'd much rather have the standard compiler help us with this... And it should not be a warning. It should err on you, much like if you forget to use an import or declare a variable and don't use it.

Just to clarify my suggestion with an example:

func RegisterChannel(ch interface{}) checked error {

}

(I use the keyword checked here, but that is not important... it can be anything.)

Using it would be like:

err := RegisterChannel(myCh)

Or, to ignore the error (here you are required to use some minor boilerplate to consciously ignore the err):

_ := RegisterChannel(myCh)

But the compiler would prevent you from doing just this:

RegisterChannel(myCh)

All the best,

:) Hein

[Kamil Kisiel]

What would be the criteria for deciding if an error needs to be checked or not? Everyone's opinion on what should be a checked error will differ and there would be no consistency between different bodies of code. You wouldn't be gaining very much for the expense of complicating the language.

I think this functionality is much better suited for a tool like go vet and configurable with a whitelist.