Using WARN_UNUSED_RESULT

[Ryan Sleevi]

Similar to the discussions related to when it is appropriate to use
OVERRIDE, I was curious if there were any policies or preferences
regarding when it is appropriate to throw a WARN_UNUSED_RESULT onto a
function signature.

Right now, its usage seems pretty scarce. Codesearch shows 83
instances, and outside of FilePath, many of the instances are limited
to testing related files. I wasn't sure if that was an intentional
design/style decision to use it sparingly, or if, like OVERRIDE, it
was a useful tool that should be used more often/liberally.

One key difference between OVERRIDE and WARN_UNUSED_RESULT is that the
former is supported on MSVC, Clang+OS X, and Clang+Linux, while the
latter is only supported on GCC and Clang. Is that a significant
reason to discourage its usage? Or is the added compile-time checking,
versus runtime checking via DCHECK()/CHECK() within the function
implementation, enough to look past the limited support?

For what it's worth, http://www.chromium.org/developers/coding-style
is silent about the matter, so it will be good to update it with
whatever the result of this discussion is.

[Scott Hess]

My opinion is that this is one that we shouldn't go hog-wild with.
F/i, a place I have an unfinished CL to add it to is app/sql/*. My
rationale is that over time I've run across a surprising number of
cases where someone executes a SQL statement without checking the
return value, even though the following code obviously assumes
statement success. When the statement is making schema changes, you
can end up in very bad places.

Basically, it seems reasonable to use in places where you want to
assert that ignoring the return value is VERY unlikely to be correct.

-scott

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
>    http://groups.google.com/a/chromium.org/group/chromium-dev
>

[James Robinson]

On Wed, Jul 27, 2011 at 6:12 PM, Scott Hess <sh...@chromium.org> wrote:

My opinion is that this is one that we shouldn't go hog-wild with.
F/i, a place I have an unfinished CL to add it to is app/sql/*.  My
rationale is that over time I've run across a surprising number of
cases where someone executes a SQL statement without checking the
return value, even though the following code obviously assumes
statement success.  When the statement is making schema changes, you
can end up in very bad places.

Basically, it seems reasonable to use in places where you want to
assert that ignoring the return value is VERY unlikely to be correct.

Big second to this.  Be very, very careful when adding wur to a new function.  The glibc maintainers thought that it would be helpful to add this to fwrite(), and it spawned this explosion of a bug:

https://bugs.launchpad.net/glibc/+bug/305176

a bug against gcc asking to disable the warning completely:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509

and then finally this glibc patch to remove the attribute:

http://sourceware.org/bugzilla/show_bug.cgi?id=11959

- James 

[Ryan Sleevi]

On Jul 27, 9:42 pm, James Robinson <jam...@google.com> wrote:
> On Wed, Jul 27, 2011 at 6:12 PM, Scott Hess <sh...@chromium.org> wrote:
> > My opinion is that this is one that we shouldn't go hog-wild with.
> > F/i, a place I have an unfinished CL to add it to is app/sql/*.  My
> > rationale is that over time I've run across a surprising number of
> > cases where someone executes a SQL statement without checking the
> > return value, even though the following code obviously assumes
> > statement success.  When the statement is making schema changes, you
> > can end up in very bad places.
>
> > Basically, it seems reasonable to use in places where you want to
> > assert that ignoring the return value is VERY unlikely to be correct.
>
> Big second to this.  Be very, very careful when adding wur to a new
> function.  The glibc maintainers thought that it would be helpful to add
> this to fwrite(), and it spawned this explosion of a bug:https://bugs.launchpad.net/glibc/+bug/305176
> a bug against gcc asking to disable the warning completely:http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
> and then finally this glibc patch to remove the attribute:http://sourceware.org/bugzilla/show_bug.cgi?id=11959
>
> - James

I do think there is a big difference between the glibc maintainers
making the change and us making the change for Chromium-internal code.
Perhaps I'm not aware of all the hidden/external dependencies that may
be pulling in Chromium sources, but any errors presented should be
things we control and can patch - the responsibility of the person
introducing WUR to a given function signature.

One of the big reactions to the GCC change is that there were limited
opportunities to suppress the warning if you truly, legitimately meant
to ignore the result, and in the case of fwrite(), there were many
cases where it was legitimately fine to ignore the result. With the
Chromium code, there is a common means to suppress - the easily-
available ignore_result ( http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/basictypes.h&l=338
). Further, I think if ignore_result ends up being used though, it's a
sign that something shouldn't be WUR to begin with - much like was
decided with fwrite().

The context of the question came up when looking at some code that had
methods DoFoo() and DoBar(), both which returned bools. The
assumption, which was generally true, was that if DoFoo() failed,
DoBar() would also fail. However, there turned out to be a bug in
DoBar() that would cause it to erroneously report success under
certain situations when the necessary call to DoFoo() failed. Several
callers of DoBar() weren't checking the result of DoFoo(), assuming/
relying on DoBar() to fail, so they were affected by this.

This is the sort of situation where I assume a liberal dose of WUR
would be a/the correct response, because it can serve as an early
warning for API misuse. Additionally, from the design side, it forces
callers to decouple from the assumption that if DoFoo() fails, DoBar()
is /guaranteed/ to fail as well - something that may be true today but
not tomorrow. Would it have found the bug in DoBar()? No. But it would
have limited the scope of impact.

My gut is that there are more places like this - where a chain of
calls are done to setup some persistent state - and the only guards
are a DCHECK(). If the condition is an extreme edge condition, it
might not be hit by the unit tests, or by developers testing locally,
or worse, only hit flakily. WUR would/could catch these. However,
given that we already have some runtime coverage via DCHECK()/CHECK(),
I wasn't sure if the readability costs for WUR outweighed the supposed
benefits.

In case it's not clear, I'm a big fan of using WUR when appropriate,
but given some of the reactions to OVERRIDE, I wanted to see what the
consensus was before making more use of it.

[Evan Martin]

On Wed, Jul 27, 2011 at 6:42 PM, James Robinson <jam...@google.com> wrote:
> On Wed, Jul 27, 2011 at 6:12 PM, Scott Hess <sh...@chromium.org> wrote:
>>
>> My opinion is that this is one that we shouldn't go hog-wild with.
>> F/i, a place I have an unfinished CL to add it to is app/sql/*.  My
>> rationale is that over time I've run across a surprising number of
>> cases where someone executes a SQL statement without checking the
>> return value, even though the following code obviously assumes
>> statement success.  When the statement is making schema changes, you
>> can end up in very bad places.
>>
>> Basically, it seems reasonable to use in places where you want to
>> assert that ignoring the return value is VERY unlikely to be correct.
>
> Big second to this.  Be very, very careful when adding wur to a new
> function.  The glibc maintainers thought that it would be helpful to add
> this to fwrite(), and it spawned this explosion of a bug:
> https://bugs.launchpad.net/glibc/+bug/305176
> a bug against gcc asking to disable the warning completely:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
> and then finally this glibc patch to remove the attribute:
> http://sourceware.org/bugzilla/show_bug.cgi?id=11959

Coincidentally, I just this week disabled this warning on Linux in all
third-party code we import, due to more or less this problem exactly.
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=commit;h=40499a685521ad804602dee4736bb6b2a20681bd

(PS: My attitude about warnings in third_party is: if upstream doesn't
care about 'em, and nobody on our team is fixing 'em, it's better to
not show them so that we don't become blind to warnings. For example,
our current Windows build emits 304 warnings that I don't think anyone
is currently looking into...)

[Evan Martin]

On Wed, Jul 27, 2011 at 7:09 PM, Ryan Sleevi <rsl...@chromium.org> wrote:
> My gut is that there are more places like this - where a chain of
> calls are done to setup some persistent state - and the only guards
> are a DCHECK(). If the condition is an extreme edge condition, it
> might not be hit by the unit tests, or by developers testing locally,
> or worse, only hit flakily. WUR would/could catch these.

To provoke discussion, here's my favorite example in our codebase.
PathService::Get can return false if for some reason it's unable to
compute a path.
If the caller doesn't check, it'll start doing file operations in a
directory other than it intended.

It appears we don't check the return code in 174 places:
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22%20%20PathService::Get(%22%20file:%5C.cc&type=cs
Whereas we do check the return code in 94:
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22(PathService::Get(%22%20file:%5C.cc&type=cs

Does that mean we're using this API wrong in >60% of cases? I'm not
sure. Maybe it's a bad API and we should just CHECK() on failure.

[Nico Weber]

On Wed, Jul 27, 2011 at 6:42 PM, James Robinson <jam...@google.com> wrote:

> On Wed, Jul 27, 2011 at 6:12 PM, Scott Hess <sh...@chromium.org> wrote:
>>
>> My opinion is that this is one that we shouldn't go hog-wild with.
>> F/i, a place I have an unfinished CL to add it to is app/sql/*.  My
>> rationale is that over time I've run across a surprising number of
>> cases where someone executes a SQL statement without checking the
>> return value, even though the following code obviously assumes
>> statement success.  When the statement is making schema changes, you
>> can end up in very bad places.
>>
>> Basically, it seems reasonable to use in places where you want to
>> assert that ignoring the return value is VERY unlikely to be correct.
>
> Big second to this.  Be very, very careful when adding wur to a new
> function.

Agreed. Add it where it _will_ find bugs (example: All these FilePath
methods like Append() that return a new FilePath – everyone assumes
they modify *this), don't add it where it maybe eventually might find
a bug under the right circumstances.

Nico

[Scott Hess]

IMHO, something like that which returns the useful value by reference
with a success code to gate usage is a great place to force the caller
to explicitly ignore the result. Even if the API is "bad", that
doesn't mean the caller is licensed to use it in a brittle fashion.

-scott