microsoft
STL: Finish marking functions as _NODISCARD
We love [[nodiscard]] in the STL. We love it so much that we've marked over 3,000 functions, and gotten special compiler support to enable it in C++14 mode (even though it's a C++17 attribute). This finds real bugs that are hard to find otherwise. (We're using a _NODISCARD macro in order to provide an escape hatch and support compiler front-ends without [[nodiscard]], but we'll probably remove that macro someday and just use the attribute directly.)
However, while our usage of [[nodiscard]] is vast, it isn't complete. Aside from #63 (WG21-P1771 [[nodiscard]] For Constructors), there are Standard functions that we haven't marked, like the operator functors (classic and transparent):
https://github.com/microsoft/STL/blob/447f879b136950baf3ca35dfb54c359494fa2a77/stl/inc/xstddef#L48-L57
https://github.com/microsoft/STL/blob/447f879b136950baf3ca35dfb54c359494fa2a77/stl/inc/xstddef#L156-L166
We also haven't comprehensively marked our internal helper functions.
For reference, we use the following criteria for marking Standard functions:
- Pure observers. (This is the most common reason.) If a function has no side effects, and simply inspects state in order to return a value, then discarding that value is extremely likely to be a bug. For example,
vector::size()is a pure observer. This also includes functions that are pure observers except in rare cases when user-provided machinery has side effects. For example, we can consider thecount_if()algorithm to be a pure observer, even though the user-provided function object might have side effects. - Functions that return raw resources. (Usually raw pointers to freshly allocated memory.) Discarding such return values is extremely likely to be a resource/memory leak.
- Functions where discarding the return value is extremely likely to be a correctness bug. This is uncommon; good examples are the
remove(),remove_if(), andunique()algorithms (where the return value should be passed to a containererase()call).
Because we mark so many functions as [[nodiscard]], it's possible for a large project to encounter many warnings when upgrading their toolset and STL. We want all of these warnings to indicate actual bugs in user code (or at least unnecessary function calls that should be removed), with virtually no false positives. To preserve the high quality of these warnings, we avoid marking functions as [[nodiscard]] when they can be used correctly while discarding, even if such use is uncommon (e.g. 10% of calls). Currently, the best example is unique_ptr::release(). Discarding this is likely to be a resource/memory leak, but not always - e.g. ownership of the raw pointer may have been previously handed off via unique_ptr::get(). Ideally, we could mark unique_ptr::release() as [[nodiscard]], and that 10% of valid calls could add (void) casts to indicate that they're doing something unusual. However, we have currently chosen not to mark it, prioritizing the quality of this warning over detecting bogus code here. This similarly applied to condition_variable::wait_for() and condition_variable::wait_until(), where valid algorithms can discard the return values. Fortunately, tricky scenarios like these are rare.
For another example, <charconv> from_chars() and to_chars() aren't marked [[nodiscard]]. While they return from_chars_result and to_chars_result containing an errc and a pointer, which should typically be checked, it is possible to call these functions knowing that they'll succeed and how much they'll read/write. Perhaps someday we could have multiple levels of [[nodiscard]] strictness, but for now, we aren't marking these functions.
For internal helper functions, we can and should be much more aggressive about [[nodiscard]].
After WG21-P2422R1, the Standard no longer depicts any functions as [[nodiscard]]. We're still allowed to be awesome.
exchange() is an interesting case:
https://github.com/microsoft/STL/blob/8f9431931b1e1f02d24d518452ae447a843c6121/stl/inc/utility#L568-L573
It has the side effect of performing an assignment, but if someone discards the return value, then it's just more expensive syntax for an assignment.
It's tempting to mark this as [[nodiscard]], although I suspect that our caution regarding false positives should probably prevail here.
Related: #1742 is a case where marking lock_guard as [[nodiscard]] warns for valid code like m_val{ (std::lock_guard{my_mutex}, obj.m_val) } in a mem-initializer-list. Although we remain very cautious about false positives, I feel that this is so rare (1% of usage or less), and that the danger of discarding guard temporaries is so extreme, that it's reasonable to use [[nodiscard]] here. As @AdamBucior suggested there, we'll look into using [[nodiscard("reason")]] to provide guidance.
(For lock_guard, we marked the entire class as [[nodiscard]]. For unique_lock and shared_lock, we encountered some code returning them from functions, so we marked their lock-acquiring constructors as [[nodiscard]] but not the entire classes.)
Related: #1742 is a case where marking
lock_guardas[[nodiscard]]warns for valid code likem_val{ (std::lock_guard{my_mutex}, obj.m_val) }in a mem-initializer-list. Although we remain very cautious about false positives, I feel that this is so rare (1% of usage or less), and that the danger of discarding guard temporaries is so extreme, that it's reasonable to use[[nodiscard]]here. As @AdamBucior suggested there, we'll look into using[[nodiscard("reason")]]to provide guidance.(For
lock_guard, we marked the entire class as[[nodiscard]]. Forunique_lockandshared_lock, we encountered some code returning them from functions, so we marked their lock-acquiring constructors as[[nodiscard]]but not the entire classes.)
That 1% usage happened to me two weeks ago. :) Agree with the decision given the danger of discarding temporaries.
Just a data point: updating to a Visual Studio version with [[nodiscard]] on std::lock_guard immediately identified a few bugs in our own code, and one in clang-tools-extra. (And zero of the "1%" cases.) Thanks!
