[libc++] Document guidelines for applying [[nodiscard]]

[philnik777]

We've been applying [[nodiscard]] more liberally recently, but we don't have any documented guidance on when it's correct to add it. This patch adds that guidance. Follow-up patches will gradually apply it to the code base.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

We've been applying [[nodiscard]] more liberally recently, but we don't have any documented guidance on when it's correct to add it. This patch adds that guidance. Follow-up patches will gradually apply this to the code base.

---

Full diff: https://github.com/llvm/llvm-project/pull/84000.diff

2 Files Affected:

• (added) libcxx/docs/DesignDocs/NodiscardExtensions.rst (+34)
• (modified) libcxx/docs/index.rst (+1)

diff --git a/libcxx/docs/DesignDocs/NodiscardExtensions.rst b/libcxx/docs/DesignDocs/NodiscardExtensions.rst
new file mode 100644
index 00000000000000..ff595b689a7529
--- /dev/null
+++ b/libcxx/docs/DesignDocs/NodiscardExtensions.rst
@@ -0,0 +1,34 @@
+======================================
+``[[nodiscard]]`` extensions in libc++
+======================================
+
+Libc++ adds ``[[nodiscard]]`` to functions in a lot more places than the
+standard does. Any applications of ``[[nodiscard]]`` that aren't required by the
+standard written as ``_LIBCPP_NODISCARD_EXT`` to make it possible to disable
+them. This can be done by defining ``_LIBCPP_DISABLE_NODISCARD_EXT``.
+
+When should ``[[nodiscard]]`` be added to functions?
+====================================================
+
+``[[nodiscard]]`` should be applied to functions
+
+- where it is most likely a correctness issue when discarding the return value.
+  For example a locking constructor in ``unique_lock``.
+- where most likely something similar was meant if the return value is
+  discarded. For example ``vector::empty()``, which probably should have
+  been ``clear()``.
+
+  This can help spotting bugs easily which otherwise may take a very long time
+  to find.
+
+- which return a constant. For example ``numeric_limits::min()``.
+- which only observe a value. For example ``string::size()``.
+
+  Code that discards values from these kinds of functions is dead code. It can
+  either be removed, or the programmer meant to do something different.
+
+- where discarding the value is most likely a misuse of the function. For
+  example ``find``.
+
+  This protects programmers from assuming too much about how the internals of
+  a function work, resulting in less code breaking as optimizations are applied.
diff --git a/libcxx/docs/index.rst b/libcxx/docs/index.rst
index aa1bd4b83b265b..b3c4be1c43e602 100644
--- a/libcxx/docs/index.rst
+++ b/libcxx/docs/index.rst
@@ -189,6 +189,7 @@ Design Documents
    DesignDocs/FeatureTestMacros
    DesignDocs/FileTimeType
    DesignDocs/HeaderRemovalPolicy
+   DesignDocs/NodiscardExtensions
    DesignDocs/NoexceptPolicy
    DesignDocs/PSTLIntegration
    DesignDocs/ThreadingSupportAPI

[philnik777]

CC @ldionne @mordante @cjdb I'm sure you have some thoughts on this.

[mordante]

First of all thanks for working on this!
As mentioned in #83695 there is a paper P3122R0 "[[nodiscard]] should be Recommended Practice" by @jwakely and there is another paper P2422R0 "Remove nodiscard annotations from the standard library specification" that proposes to drop all [[nodiscard]]s from the draft.

These papers are likely to be discussed in a few weeks during the Tokyo meeting. So I would propose to wait until that has been discussed; it might affect this RFC. Even when these papers are not accepted we should look at P3122R0 it has recommended practices we could use as guideline in our documentation.

Note I'm in favour of using [[nodiscard]] more in libc++ and the direction of these papers.

[philnik777]

First of all thanks for working on this! As mentioned in #83695 there is a paper P3122R0 "[[nodiscard]] should be Recommended Practice" by @jwakely and there is another paper P2422R0 "Remove nodiscard annotations from the standard library specification" that proposes to drop all [[nodiscard]]s from the draft.

These papers are likely to be discussed in a few weeks during the Tokyo meeting. So I would propose to wait until that has been discussed; it might affect this RFC. Even when these papers are not accepted we should look at P3122R0 it has recommended practices we could use as guideline in our documentation.

Note I'm in favour of using [[nodiscard]] more in libc++ and the direction of these papers.

Thanks for bringing them up. I agree that this may change things, but I also think that discussing this beforehand within the library also makes a lot of sense, since "look, one implementation already does it, the second one is working on getting [[nodiscard]] in lots of places, and the third one's main maintainer is proposing this stuff" is a really good argument (got a bit wordy). If we talk about it beforehand, we have thought about it already quite a bit and can hopefully give good reasons to add [[nodiscard]] to some functions and not to others.

From looking at the list of P3122R0 I think we want to add comparison operators. Everything else should already be covered™.

FWIW I also very much agree with the sentiment of the papers.

[jwakely]

The whole point of my P3122R0 paper is that I think implementations should make their own decisions about where to use nodiscard. The standard should do no more than make some vague recommendations, and definitely not try to standardize where it should be used (or even worse, standardize where it should not be used).

So I am very pleased to see this change and the start of a discussion. Please don't wait for WG21 to specify nodiscard on individual functions or on some minimal "low effort" set of functions.

[mordante]

The whole point of my P3122R0 paper is that I think implementations should make their own decisions about where to use nodiscard. The standard should do no more than make some vague recommendations, and definitely not try to standardize where it should be used (or even worse, standardize where it should not be used).

True, but your paper has some nice suggestions and when that becomes recommended practice I would like to see that as minimal set in our documentation. After which we can look whether more places would be useful to have nodiscards.

So I am very pleased to see this change and the start of a discussion. Please don't wait for WG21 to specify nodiscard on individual functions or on some minimal "low effort" set of functions.

At the moment all our nodiscards not specified in the Standard can be disabled. The disabling mechanism tests all occurrences. Depending on the outcome in discussion Tokyo I want to discuss internally whether we want to keep testing all disabling occurrences or not. (We already removed the documentation for them.)

[ldionne]

LGTM with some comments. I'd also expect another PR that cleans up the current state of _LIBCPP_NODISCARD_EXT.

[philnik777]

For reference, microsoft/STL#206 lists when MSVC applies [[nodiscard]], which is very similar to the list proposed here.

[philnik777]

If anybody wants to add any more places we should add [[nodiscard]] feel free to open a new PR. I didn't see any feedback against the places proposed here, so I'm landing this now.