A reminder about MOZ_MUST_USE and [must_use]

[Nicholas Nethercote]

Hi,

We have two annotations that can be used to prevent missing return value
checks:

- MOZ_MUST_USE for C++ functions where the return type indicates
success/failure, e.g. nsresult, bool (in some instances), and some other
types.

- [must_use] for IDL methods and properties where the nsresult value should
be checked.

We have *many* functions/methods/properties for which these annotations are
appropriate, and *many* missing return value checks. Unfortunately, trying
to fix these proactively is a frustrating and thankless task, because it's
difficult to know in advance which missing checks are likely to cause
problems in advance, and adding missing checks is not always
straightforward.

However, if you do see clearly buggy behaviour (e.g. a crash) caused by a
missing return value, please take the opportunity to retroactively add the
annotation(s) in that case!
https://bugzilla.mozilla.org/show_bug.cgi?id=1331619 is a good example of
such a bug, and https://bugzilla.mozilla.org/show_bug.cgi?id=1332453 is the
follow-up to add the annotations.

Nick

[Eric Rescorla]

What would actually be very helpful would be a way to selectively turn on
checking of
*all* return values in a given file/subdirectory. Is there some
mechanism/plan for that?

Thanks,
-Ekr

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[gsqu...@mozilla.com]

And the next step would be to make must-use the default, and have MOZ_CAN_IGNORE for the rest. ;-)

Gerald (who is not volunteering!)

> > dev-pl...lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >

[Nicholas Nethercote]

On Fri, Jan 20, 2017 at 9:29 AM, Eric Rescorla <e...@rtfm.com> wrote:

> What would actually be very helpful would be a way to selectively turn on
> checking of
> *all* return values in a given file/subdirectory. Is there some
> mechanism/plan for that?
>

Not that I know of, other than manually annotating them.

As someone who's spent some time adding these annotations, I think this
idea sounds great in theory but turns out to be impractical. There are lots
of functions where not checking the return value is reasonable, such as
close(). And there are lots of functions that have a return value due to
interface constraints, but it doesn't need to be checked, such as many
XPIDL methods. And yes, you can use mozilla::Unused to mark functions that
don't need checking, but that gets annoying quickly.

If you want to be ambitious and avoid piecemeal approaches, I suggest using
the new mozilla::Result type, which has the MOZ_MUST_USE_TYPE annotation on
it that means static analysis builds will fail if results aren't checked.
Doing this stuff in new code is easier than retrofitting old code.

Nick

[Nicholas Nethercote]

On Fri, Jan 20, 2017 at 10:01 AM, <gsqu...@mozilla.com> wrote:

> And the next step would be to make must-use the default, and have
> MOZ_CAN_IGNORE for the rest. ;-)
>

I actually tried this with all XPIDL methods. After adding several hundred
"Unused <<" annotations for calls that legitimately didn't need to check
the return value -- and I was only a fraction of the way through the
codebase -- I decided that a big bang approach wasn't going to work. So I
then implemented [must_use] as an incremental alternative.

Nick

[gsqu...@mozilla.com]

I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.

So in cases where a return value from a function can be ignored, instead of adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in front of the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more appropriate.

Of course I'm sure it would still take a lot of work!

Maybe there could be a slow hybrid approach:
- Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
- Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for larger failures.
- Gradually "infect" more directories.
- Once it's everywhere, make MOZ_MUST_USE the default, and remove the above scaffolding.
- Profit!

Gerald

[Chris Peterson]

To encourage all new XPIDL methods to use must_use, could you add
MOZ_MUST_USE to the C++ macro definition of NS_IMETHOD and rename all
the existing uses of NS_IMETHOD to something like
NS_IMETHOD_RESULT_OPTIONAL or NS_IMETHOD_RESULT_UNUSED? Developers
adding new methods will reach the familiar (and shorter) NS_IMETHOD
macro and get must_use for free.

[Bobby Holley]

On Thu, Jan 19, 2017 at 3:26 PM, <gsqu...@mozilla.com> wrote:

> On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote
> wrote:
> > On Fri, Jan 20, 2017 at 10:01 AM, <gsqu...@mozilla.com> wrote:
> >
> > > And the next step would be to make must-use the default, and have
> > > MOZ_CAN_IGNORE for the rest. ;-)
> > >
> >
> > I actually tried this with all XPIDL methods. After adding several
> hundred
> > "Unused <<" annotations for calls that legitimately didn't need to check
> > the return value -- and I was only a fraction of the way through the
> > codebase -- I decided that a big bang approach wasn't going to work. So I
> > then implemented [must_use] as an incremental alternative.
> >
> > Nick
>
> I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.
>

I think the point is that it's not obvious that "must check the return
value" is a sufficiently-dominant common case for arbitrary return values.
FWIW, Rust took the [must_use] rather than [can_ignore] approach too.

It probably depends a lot on the return value type.

> So in cases where a return value from a function can be ignored, instead
> of adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in
> front of the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more
> appropriate.
>
> Of course I'm sure it would still take a lot of work!
>
> Maybe there could be a slow hybrid approach:
> - Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
> - Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for
> larger failures.
> - Gradually "infect" more directories.
> - Once it's everywhere, make MOZ_MUST_USE the default, and remove the
> above scaffolding.
> - Profit!
>
> Gerald

[gsqu...@mozilla.com]

On Friday, January 20, 2017 at 10:36:46 AM UTC+11, Bobby Holley wrote:
> On Thu, Jan 19, 2017 at 3:26 PM, <gsqu...@mozilla.com> wrote:
>
> > On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote
> > wrote:
> > > On Fri, Jan 20, 2017 at 10:01 AM, <gsqu...@mozilla.com> wrote:
> > >
> > > > And the next step would be to make must-use the default, and have
> > > > MOZ_CAN_IGNORE for the rest. ;-)
> > > >
> > >
> > > I actually tried this with all XPIDL methods. After adding several
> > hundred
> > > "Unused <<" annotations for calls that legitimately didn't need to check
> > > the return value -- and I was only a fraction of the way through the
> > > codebase -- I decided that a big bang approach wasn't going to work. So I
> > > then implemented [must_use] as an incremental alternative.
> > >
> > > Nick
> >
> > I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.
> >
>
> I think the point is that it's not obvious that "must check the return
> value" is a sufficiently-dominant common case for arbitrary return values.
> FWIW, Rust took the [must_use] rather than [can_ignore] approach too.

That's unfortunate. But real-world data must trump my idealism in the end. :-)

> It probably depends a lot on the return value type.

Makes sense.


Another idea:
Could all non-void const methods (those that don't modify *this) be MOZ_MUST_USE by default? They supposedly don't have side-effects, so why would their return value ever be ignored?

[Ted Mielczarek]

On Thu, Jan 19, 2017, at 07:00 PM, gsqu...@mozilla.com wrote:
> > I think the point is that it's not obvious that "must check the return
> > value" is a sufficiently-dominant common case for arbitrary return values.
> > FWIW, Rust took the [must_use] rather than [can_ignore] approach too.
>
> That's unfortunate. But real-world data must trump my idealism in the
> end. :-)

The Rust case is helped by the fact that `Result` is the defacto type
for returning success or error, and it's effectively `must_use`. We
don't have a similar default convention in C++.

-Ted

[Nicolas B. Pierron]

We have

http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/mfbt/Result.h#173

we just have to make it the default convention now.

--
Nicolas B. Pierron

[Ted Mielczarek]

On Fri, Jan 20, 2017, at 08:19 AM, Nicolas B. Pierron wrote:
> > The Rust case is helped by the fact that `Result` is the defacto type
> > for returning success or error, and it's effectively `must_use`. We
> > don't have a similar default convention in C++.
>
> We have
>
> http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/mfbt/Result.h#173
>
> we just have to make it the default convention now.

Yes, and this is great, I just meant that in Rust 99% of code that's
returning success/failure is using `Result` because it's in the standard
library, whereas in C++ there's not an equivalent. `mozilla::Result` is
great and I hope we can convert lots of Gecko code to use it, but we
have *a lot* of Gecko code that's not already there.

-Ted

[Michael Layzell]

It wouldn't be too hard to automatically generate Result<T, nsresult>
wrapper methods automatically for all of our XPIDL interfaces. I had a
prototype branch at one point which did this on the rust side, as part of
my now-dead rust XPIDL bindings.

That would convert a good number of our fallable calls to using Result<T,
nsresult>. We might even be able to look into doing something similar with
our WebIDL bindings and ErrorResult.

On Fri, Jan 20, 2017 at 8:59 AM, Ted Mielczarek <t...@mielczarek.org> wrote:

> On Fri, Jan 20, 2017, at 08:19 AM, Nicolas B. Pierron wrote:

> > > The Rust case is helped by the fact that `Result` is the defacto type
> > > for returning success or error, and it's effectively `must_use`. We
> > > don't have a similar default convention in C++.
> >
> > We have
> >
> > http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fad
> d370acfd2f/mfbt/Result.h#173
> >
> > we just have to make it the default convention now.
>

> Yes, and this is great, I just meant that in Rust 99% of code that's
> returning success/failure is using `Result` because it's in the standard
> library, whereas in C++ there's not an equivalent. `mozilla::Result` is
> great and I hope we can convert lots of Gecko code to use it, but we
> have *a lot* of Gecko code that's not already there.
>
> -Ted

[ISHIKAWA,chiaki]

On 2017/01/20 8:10, Nicholas Nethercote wrote:
> There are lots
> of functions where not checking the return value is reasonable, such as
> close().

A file opened for writing and is buffered will flush pending data to
disk upon Close() and may encounter the error such as disk full AT THAT
POINT, and so the return value of
Close() MUST BE CHECKED for those cases.
[I am fighting to fix the issue since C-C TB fails to use buffered write
which causes so much I/O overhead, but then I found the return value of
Close() are not checked at all. Ugh... So I have to fix C-C TB to make
it check the return value of Close() and take appropriate error
reporting/recovery action before I can enable buffering. Very
disappointing...]

A blanket statement above can put bad ideas in many people's mind :-(

Of course, if the "close()" above refers to POSIX close(), fine.

TIA