1268766 - (use-nodiscard) Use [[nodiscard]] everywhere

Status: RESOLVED WORKSFORME

(Core :: General, task)

Description

[Nicholas Nethercote [inactive]]

MOZ_MUST_USE (which was, until bug 1267550, known as MOZ_WARN_UNUSED_RESULT) is great. It finds real bugs. It should be considered for any fallible function that returns a non-pointer type (typically bool or nsresult).

(Unfortunately it's not useful with fallible functions that return a pointer and use nullptr to indicate failure, because if you forget a check in that case you'll still use the result and so no error will be triggered.)

This is a meta-bug.

Comment 1

[Andrew McCreight [:mccr8]]

Coverity would provide good fodder for this. One of its warnings is something like "return value of this function checked in most places, but not this one."

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Coverity would provide good fodder for this. One of its warnings is
> something like "return value of this function checked in most places, but
> not this one."

I don't quite understand this. Does Coverity do this by default? Are you suggesting that it reduce/eliminate the need for MOZ_MUST_USE annotations?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> I don't quite understand this. Does Coverity do this by default? Are you
> suggesting that it reduce/eliminate the need for MOZ_MUST_USE annotations?

This is a standard thing that Coverity reports. I'm suggesting that we could use those Coverity reports as a guide for functions to add MOZ_MUST_USE annotations to.

For instance, a report looks like this:
*** CID 1358644:  Error handling issues  (CHECKED_RETURN)
/dom/filesystem/FileSystemBase.cpp: 121 in mozilla::dom::FileSystemBase::GetDirectoryName(nsIFile *, nsAString_internal &, mozilla::ErrorResult &) const()
115                                      ErrorResult& aRv) const
116     {
117       AssertIsOnOwningThread();
118       MOZ_ASSERT(aFile);
119
120       aRv = aFile->GetLeafName(aRetval);
>>>     CID 1358644:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "NS_warn_if_impl" without checking return value (as is done elsewhere 128 out of 136 times).
121       NS_WARN_IF(aRv.Failed());

Comment 4

[Nicholas Nethercote [inactive]]

> This is a standard thing that Coverity reports. I'm suggesting that we could
> use those Coverity reports as a guide for functions to add MOZ_MUST_USE
> annotations to.

I see. The cited example (NS_warn_if_impl) is a case where an annotation probably is not needed. But if you (or anyone else with access to Coverity results) wants to file bugs about adding the annotation in cases where it is appropriate, that would be very helpful.

Comment 5

[Nicholas Nethercote [inactive]]

Adding these annotations is a big task -- we have a *lot* of code -- but not particularly hard. If anybody wants to join in, here's how I go about it.

- Choose a directory to work on. Look at this bug's blockers to make sure there isn't already a bug filed for it. If not, file a bug for it, make it block this one, and assign it to yourself.

- Look through all the .h files in that directory.

  - grep for all occurrences of |bool|, |nsresult|, and |NS_IMETHOD| (which matches |NS_IMETHOD|, |NS_IMETHODIMP| and |NS_IMETHOD_(T)|, all of which return |nsresult|).

  - If an occurrence is a function return value and the function is fallible, mark the *declaration* with MOZ_MUST_USE. If the function *definition* is separate, there's no need to mark it as well.

  - Note that many functions that return |bool| are actually predicates, i.e. the return value is the answer to a yes/no question rather than a success/failure indicator. These ones don't need to be marked. For example, all |operator==| functions are predicates.

- Do likewise in the C++ sections of .idl files in that directory.

- Once finished, compile. If you get any compile errors, you need to decide how to handle missing checks.

  - Sometimes it's obvious that a check should be added. Add it.

  - Sometimes it's obvious that a check isn't needed. E.g. a function with an |nsresult| return type that always returns |NS_OK|, or a function where failure doesn't really matter and won't cause you to act differently. In either case, you should probably remove the MOZ_MUST_USE annotation from the called function.

  - Sometimes it's harder and requires some judgment. Ask around if necessary.

Comment 6

[Nicholas Nethercote [inactive]]

Also: if a function is obviously non-fallible, often you can just change the return type to |void|.

Comment 7

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

How do we (intend to) handle classes that inherited from IPDL?
Most of these classes are used by generated code, should we do this at the code generator level?

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #7)
> How do we (intend to) handle classes that inherited from IPDL?
> Most of these classes are used by generated code, should we do this at the
> code generator level?

I haven't looked at this case. If it makes sense, then doing it at the code generator level would be best, because it would give the most coverage. But it's possible that not checking the result of some of the generated functions is reasonable.

Relatedly, I tried changing the definition of NS_IMETHOD from this:

> #define NS_IMETHOD          NS_IMETHOD_(nsresult)

to this:

> #define NS_IMETHOD          MOZ_MUST_USE NS_IMETHOD_(nsresult)

but it was too broad and there were *lots* of violations, plenty of which were reasonable.

Anyway, I suggest doing some experimenting.

Comment 9

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I've opened bugs for first-level folders, so that we can do this by divide and conquer.
If you want to work on a folder, just take the bug.
If a folder contains too many files, you could mark the bug as a meta bug, then open bugs to deal the subfolders.

There are some folders not on the list, they are either:

1. do not contain C++ code, or should not have problem:
addon-sdk/
b2g/
config/
gradle/
memory/
mozglue/
nsprpub/
probes/
python/
release/
taskcluster/
testing/
third_party/
tools/

2. external library only
db/
media/
modules/
other-licenses/

3. not sure whether they are done or not
js/
mfbt/

:njn, had we complete this for js/ and mfbt/ ?

Comment 10

[Nicholas Nethercote [inactive]]

Thank you for filing all these bugs. Very helpful! And don't forget about "[must_use]" with IDL methods :)

> :njn, had we complete this for js/ and mfbt/ ?

Bug 1268754 did mfbt/.

Bug 1267551 did some of js/. But then there is bug 1277368, which is a more general approach to fixing error handling within the JS engine. If completed, it will avoid completely the need for MOZ_MUST_USE. However it's currently in an unclear state -- no progress has been made for months and jorendorff hasn't responded to questions about it. So I would ignore js/ for now.

Comment 11

[Takanori MATSUURA]

Are depending bugs still valid?

Comment 12

[Andrew Overholt [:overholt]]

I think Chris may know.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Takanori MATSUURA from comment #11)

Are depending bugs still valid?

I think the depending bugs about "Use [[nodiscard]] in <some directory>" can be closed. The bugs are just suggesting we should use [[nodiscard]] more, but they're not really actionable. Someone would have to review all the code in each directory for interesting error cases. I don't think any anyone is going to do that, so we can close the bug.

Depending bug 1201643 about AppendElement is still actionable and valid.

Comment 14

[Takanori MATSUURA]

(In reply to Chris Peterson [:cpeterson] from comment #13)

I think the depending bugs about "Use [[nodiscard]] in <some directory>" can be closed. The bugs are just suggesting we should use [[nodiscard]] more, but they're not really actionable. Someone would have to review all the code in each directory for interesting error cases. I don't think any anyone is going to do that, so we can close the bug.

"Use [[nodiscard]] in <some directory>" bugs are now closed.

Comment 15

[Andrew McCreight [:mccr8]]

I think this is still a valid thing to pursue, but nobody has added a new blocking bug in more than 5 years, so I think this has served its purpose.

Comment 16

[Andrew McCreight [:mccr8]]

Well, I guess something did get added around 3 years ago, but still.