Open
Bug 1268766
(use-nodiscard)
Opened 5 years ago
Updated 6 months ago
Use [[nodiscard]] everywhere
Categories
(Core :: General, task)
Core
General
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox49 | --- | affected |
People
(Reporter: n.nethercote, Unassigned)
References
(Depends on 27 open bugs, Blocks 1 open bug)
Details
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.
![]() |
Reporter | |
Updated•5 years ago
|
Alias: MOZ_MUST_USE
Comment 1•5 years ago
|
||
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."
![]() |
Reporter | |
Comment 2•5 years ago
|
||
(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?
Flags: needinfo?(continuation)
Comment 3•5 years ago
|
||
(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());
Flags: needinfo?(continuation)
![]() |
Reporter | |
Comment 4•5 years ago
|
||
> 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.
![]() |
Reporter | |
Comment 5•5 years ago
|
||
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.
![]() |
Reporter | |
Comment 6•5 years ago
|
||
Also: if a function is obviously non-fallible, often you can just change the return type to |void|.
Depends on: 1273079
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?
![]() |
Reporter | |
Comment 8•5 years ago
|
||
(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.
Depends on: 1293212
Depends on: 1303701
Depends on: 1310526
Depends on: 1310527
Depends on: 1310528
Depends on: 1310529
Depends on: 1310530
Depends on: 1310531
Depends on: 1310533
No longer depends on: 1297056
Depends on: 1310534
Depends on: 1310535
Depends on: 1310536
Depends on: 1310538
Depends on: 1310539
Depends on: 1310541
Depends on: 1310562
Depends on: 1310566
Depends on: 1310567
No longer depends on: 1303701
Depends on: 1310569
Depends on: 1310570
Depends on: 1310571
Depends on: 1310576
Depends on: 1310577
Depends on: 1310579
Depends on: 1310581
Depends on: 1310583
No longer depends on: 1268772
Depends on: 1310586
No longer depends on: 1297950
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/ ?
Flags: needinfo?(n.nethercote)
![]() |
Reporter | |
Comment 10•4 years ago
|
||
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.
Flags: needinfo?(n.nethercote)
Updated•6 months ago
|
Alias: MOZ_MUST_USE → use-nodiscard
Type: defect → task
Summary: Use MOZ_MUST_USE everywhere → Use [[nodiscard]] everywhere
You need to log in
before you can comment on or make changes to this bug.
Description
•