checking unused return values

[Jonathan Amsterdam]

I recently learned about gcc's __attribute__ ((warn_unused_result)),
which is attached to a non-void function and generates a warning
whenever the return value of that function is ignored by a caller. I
think Go could benefit from something similar -- a way to mark a
function's return value so that calls ignoring that value fail to
compile.

This feature would close a hole in Go's style of dealing with errors
via return value. With more than one return value (e.g. one "normal"
return value and one error object), it's impossible to ignore the
error while still extracting the desired result. But with functions
that return only an error value, e.g.

func (req *Request) Write(w io.Writer) os.Error

it's easy (and probably common) to drop the error value on the floor.

Of course, we can do better than gcc's syntax. I was thinking of
something like

func (req *Request) Write(w io.Writer) os.Error!

This requires no new keywords and, IMHO, does a good job of conveying
the intent.

Callers that insist on ignoring the return value can always write

_ = req.Write(w)

[Namegduf]

I think it'd improve safety and make writing reliable programs easier
to just apply this to all functions returning a value. This would make
it less nice for people throwing together quick scripts, of course...

Perhaps a compiler flag?

[bflm]

-1

[Jonathan Amsterdam]

> I think it'd improve safety and make writing reliable programs easier
> to just apply this to all functions returning a value. This would make
> it less nice for people throwing together quick scripts, of course...

There are many cases where the return value is legitimately ignorable.
E.g. an Insert method on a data structure that returns true iff the
value was already present. It's cheaper to detect this when doing the
insertion than to call a membership function later, but most clients
won't care.

[andrey mirtchovski]

Report as spam

:ϸ

[Matt Kane's Brain]

Well, those folks would call every function like _, _, _ =
FunctionWithThreeReturns()

Which is hideous.

--
matt kane's brain
http://hydrogenproject.com
capoeira in boston http://capoeirageraisboston.com
aim -> mkbatwerk ; skype/y! -> mkb218 ; gtalk -> mkb.dirtyorg

[Steven Blenkinsop]

Another example: you almost never care about the return values of fmt.Println.

[Jessta]

On Fri, Jul 8, 2011 at 2:23 AM, Jonathan Amsterdam
<jbams...@gmail.com> wrote:
> I recently learned about gcc's __attribute__ ((warn_unused_result)),
> which is attached to a non-void function and generates a warning
> whenever the return value of that function is ignored by a caller. I
> think Go could benefit from something similar -- a way to mark a
> function's return value so that calls ignoring that value fail to
> compile.

I don't think the compiler is the place for this.
It would be fairly easy to create a simple program that could find
place in code where return values were ignored.
Perhaps it could go in govet.

> This feature would close a hole in Go's style of dealing with errors
> via return value. With more than one return value (e.g. one "normal"
> return value and one error object), it's impossible to ignore the
> error while still extracting the desired result. But with functions
> that return only an error value, e.g.
>
>    func (req *Request) Write(w io.Writer) os.Error
>
> it's easy (and probably common) to drop the error value on the floor.
>
> Of course, we can do better than gcc's syntax. I was thinking of
> something like
>
>    func (req *Request) Write(w io.Writer) os.Error!
>
> This requires no new keywords and, IMHO, does a good job of conveying
> the intent.
>
> Callers that insist on ignoring the return value can always write
>
>    _ = req.Write(w)
>

--
=====================
http://jessta.id.au

[Brad Fitzpatrick]

Perl has a function, wantarray ($ perldoc -f wantarray), that you can use within a function to tell you if your caller is using your return value as a scalar (single value), a list, or ignoring it ("void context").

I used that often in Perl to implement error handling that would return an error if my caller was looking at it, but logged and/or exploded if my caller was ignoring the error.

I'm not saying whether that would be good in Go, just mentioning it.

[Matt Kane's Brain]

On Thu, Jul 7, 2011 at 14:03, Jessta <jes...@jessta.id.au> wrote:
> It would be fairly easy to create a simple program that could find
> place in code where return values were ignored.
> Perhaps it could go in govet.

Can we at least call it goverity? :)

[Kyle Lemons]

I don't think the compiler is the place for this.
It would be fairly easy to create a simple program that could find
place in code where return values were ignored.
Perhaps it could go in govet.

govet certainly seems like a great place for this sort of functionality.  I agree both that the compiler shouldn't complain about this and also that it can sometimes be a bug.  For a large project, I can see keeping a golden list of govet warnings, and requiring signoff whenever a CL would add a new warning to the govet output.

~K 

[Jonathan Amsterdam]

> govet certainly seems like a great place for this sort of functionality.

I don't see how.

If govet warns about all unused return values, it will be too noisy
(as discussed in this thread, there are many cases where ignoring the
return value is fine).

If govet only warns about ignoring os.Error, it will miss bugs. Not
everyone uses os.Error to indicate errors, nor should they.
Furthermore, there are other cases where you want to insist that the
return value be used. A great example is the built-in append function,
where failure to use the return value is definitely a bug, arising
from the common misapprehension that append always modifies its first
argument. And hey, what do you know, the compiler gives an error if
you ignore append's return value!

If you have to mark your code somehow to suppress or enable govet
warnings, we're back to cruft like __attribute__
((warn_unused_result)).

[Paul Borman]

I think Kyle already provided the answer to this: have a file of known exceptions

[Steven Blenkinsop]

On 2011-07-07, at 6:12 PM, Paul Borman <bor...@google.com> wrote:

> I think Kyle already provided the answer to this: have a file of known exceptions

The problem with this is that it leaves control entirely up to the user, who might not be the best one to decide. Maybe there should be a way to temporarily silence a warning, but overall, I think it's better to be able to disable that and still have sensible warnings.

[Paul Borman]

I disagree, it leaves control up to the developers.  The first time you run it you would see a large number of "errors".  You then determine which ones are harmless and fix the other ones.     Now in the future as you modify the code you will no longer hear about the the harmless ones but any new ones will generate warnings for you to look at.

[Steven Blenkinsop]

On 2011-07-07, at 10:31 PM, Paul Borman <bor...@google.com> wrote:

> I disagree, it leaves control up to the developers. The first time you run it you would see a large number of "errors". You then determine which ones are harmless and fix the other ones. Now in the future as you modify the code you will no longer hear about the the harmless ones but any new ones will generate warnings for you to look at.

You can't have a machine global list. That will just accumulate ignores to the point of meaninglessness (unless it is locked, standard, and enforced by the company/project). So, either each package comes with good defaults, which you can revert to easily, or you have to go from scratch for each package, and manually go through and reassess every decision as situations change. The latter is simply unmanageable, since you can't separate the important decisions from the noise.

[Paul Borman]

The lists certainly go hand in hand with the source files.  This is something that should be decided on a project by project basis, not thrust upon an entire community.

I was viewing this as how can a team write good code, validate "risky" constructs,  and catch errors of regression.  So yes, I believe it should be enforced at the project level. From your comments perhaps you, as a package developer, want to force certain practices upon developers who use your package (which would explain why you used the term "users").  If that is your goal then we probably will agree to disagree :-)

[Steven Blenkinsop]

On Jul 7, 2011, at 11:46 PM, Paul Borman <bor...@google.com> wrote:

> The lists certainly go hand in hand with the source files. This is something that should be decided on a project by project basis, not thrust upon an entire community.
>
> I was viewing this as how can a team write good code, validate "risky" constructs, and catch errors of regression. So yes, I believe it should be enforced at the project level. From your comments perhaps you, as a package developer, want to force certain practices upon developers who use your package (which would explain why you used the term "users").

Not "force", just "encourage" :). The package provider is in a better place to know if ignoring the return values is safe in general. The package user is in a better place to know if ignoring the return values is safe for their use case, but would have to really think about it before going against the suggested use to make sure it's actually sufficiently safe for them. I think you need a bit of both to prevent the tool becoming useless in the face of noise, from either direction.

> If that is your goal then we probably will agree to disagree :-)

But that's no fun! :D

[Kyle Lemons]

Not "force", just "encourage" :).

I think you'll find that "encouraging best practices" is the reason for the compiler not having warnings at all.  The idea is that if it's worth saying anything, it's worth being an error, otherwise it's left up to other tools or the writer/reviewer to catch.  Having warnings, even ones that can be silenced, encourages people to be cavalier about ignoring them.  Silencing warnings is also a disaster waiting to happen, as we all know that we'd silence them in development and think "I'll fix that later" and forget to actually do so.

I don't personally think adding a "govet" target to a package makefile and diff'ing its output with a golden requires much effort, and is a solution that is probably obvious enough to anyone who wants to enforce this sort of style for their team.  If you have a review system in place, you don't even need to do anything more than just redirect the govet output to a file in version control, and reviewers can call on you to justify any changes.  The startup cost of analyzing the output of govet is probably somewhat high, but if you start early in a project lifetime you even avoid that.

~K 

[Jonathan Amsterdam]

govet and its linty friends are appropriate when the issue is one of style, or something that's really a library matter (as with govet's current focus, printf-like methods), or with conservative static analyses, like static data race detection. In all those cases, it makes sense to have a separate tool with separately-maintained exception files, since the tools are imperfect and/or people can legitimately differ on what is an error.

I think this case is different. It is always an error to ignore the return value of append, or a Size method on a data structure. It's not a matter of style. And in the event you do want to ignore it (maybe in a benchmark), you can do so by simply writing "_ = f()". We're talking about a compile-time check that is easy to notate, cheap to compute exactly, can be easily worked around if necessary, takes only a sentence or two to spec out, and that catches significant bugs. Those are good reasons for putting it in the language.

By the way, besides failing to check error values, there is another class of bugs this will catch that may be small now but that I think is only going to grow as more programmers adopt a functional style. That is the bug that causes the statement "point.Translate(dx, dy)" to be a no-op, because the writer of the Point type made it immutable and wrote Translate to return a new Point. It's similar to append, except that append would rather not return a new object but has to sometimes.

[Steven Blenkinsop]

This sounds to me that there are two distinct problems. One is forcing the user, at compile time, to put the return values somewhere. The other is, at vetting time, catching cases where the programmer explicitly chose to ignore such return values, so that the programmer can verify that they really still want to do that.