171

C++17 introduces the [[nodiscard]] attribute, which allows programmers to mark functions in a way that the compiler produces a warning if the returned object is discarded by a caller; the same attribute can be added to an entire class type.

I've read about the motivation for this feature in the original proposal, and I know that C++20 will add the attribute to standard functions like std::vector::empty, whose names do not convey an unambiguous meaning regarding the return value.

It's a cool and a useful feature. In fact, it almost seems too useful. Everywhere I read about [[nodiscard]], people discuss it as if you'd just add it to a select few functions or types and forget about the rest. But why should a non-discardable value be a special case, especially when writing new code? Isn't a discarded return value typically a bug or at least a waste of resources?

And isn't one of the design principles of C++ itself that the compiler should catch as many errors as possible?

If so, then why not add [[nodiscard]] in your own, non-legacy code to almost every single non-void function and almost every single class type?

I've tried to do that in my own code, and it works fine, except it's so terribly verbose that it starts to feel like Java. It would seem much more natural to make compilers warn about discarded return values by default except for the few other cases where you mark your intention[*].

As I've seen zero discussions about this possibility in standard proposals, blog entries, Stack Overflow questions or anywhere else on the internet, I must be missing something.

Why would such mechanics not make sense in new C++ code? Is verbosity the only reason not to use [[nodiscard]] almost everywhere?

[*] In theory, you may have something like a [[maydiscard]] attribute instead, which could also be retroactively added to functions like printf in standard-library implementations.

Christian Hackl
  • 1,884
  • 2
  • 11
  • 12
  • 28
    Well there's a whole lot of functions where the return value *can* be sensibly discarded. `operator =` for example. And `std::map::insert`. – user253751 Dec 30 '17 at 20:39
  • 5
    @immibis: True. The standard library is full of such functions. But in my own code, they tend to be extremely rare; do you think it's a matter of library code vs. application code? – Christian Hackl Dec 30 '17 at 20:47
  • Perhaps it is . – user253751 Dec 30 '17 at 20:57
  • GCC has had this feature forever, yet lots of value-returning functions don't use it for reasons detailed below https://stackoverflow.com/questions/9148134/declared-with-attribute-warn-unused-result-wunused-result – cat Dec 31 '17 at 01:45
  • 21
    *"...terribly verbose that it starts to feel like Java..."* - reading this in a question about C++ made me twitch a little :-/ – Marco13 Dec 31 '17 at 18:49
  • 1
    @Marco13: May I ask why? Java's verbosity has traditionally been one of the main complaints about the language, whereas C++ makes it easy to be brief about everything (arguably almost too brief). – Christian Hackl Dec 31 '17 at 19:01
  • 4
    @ChristianHackl The comments are likely not the right place for "language bashing", and of course, Java also is verbose compared to other languages. But header files, assignment operators, copy constructors and proper usage of `const` can bloat an otherwise "simple" class (or rather "plain old data object") *considerably* in C++. – Marco13 Jan 01 '18 at 01:39
  • 3
    @Marco13: I cannot address so many points in one comment. Let's make it brief: Java doesn't have header files, which reduces file count but increases coupling; OTOH it forces you to put every top-level class in its own file and every function in a class. In C++, you almost never implement assignment operators and copy constructors yourself; those functions are more relevant for standard-library classes like `std::vector` or `std::unique_ptr`, of which you just need data members in your class definition. I've worked with both languages; Java is an okayish language, but it is more verbose. – Christian Hackl Jan 01 '18 at 11:45
  • 1
    You might be interested in [Nim](https://nim-lang.org), a programming language where this is default for all functions. https://nim-lang.org/docs/manual.html#statements-and-expressions-discard-statement – dom96 Jan 01 '18 at 16:51
  • 1
    _"And isn't one of the design principles of C++ itself that the compiler should catch as many errors as possible?"_ No, that is not one of the design principles of C++. Only errors which are in the standard that are required to emit a diagnostic must be caught. Anything beyond that is left to the discretion of the compiler vendor, and they only catch ones that take negligible time to perform the static analysis, and only for ones that can be reliably detected without false positives. Historically, separate tools would do deeper static analysis. – Eljay May 27 '20 at 19:27
  • 2
    Because C++ has all the defaults wrong. :) – alfC Jul 20 '20 at 13:50

5 Answers5

131

In new code that need not be compatible with older standards, do use that attribute wherever it's sensible. But for C++, [[nodiscard]] makes a bad default. You suggest:

It would seem much more natural to make compilers warn about discarded return values by default except for the few other cases where you mark your intention.

That would suddenly cause existing, correct code to emit lots of warnings. While such a change could technically be considered to be backwards-compatible since any existing code still compiles successfully, that would be a huge change of semantics in practice.

Design decisions for an existing, mature language with a very large code base are necessarily different from design decisions for a completely new language. If this were a new language, then warning by default would be sensible. For example, the Nim language requires unneeded values to be discarded explicitly – this would be similar to wrapping every expression statement in C++ with a cast (void)(...).

A [[nodiscard]] attribute is most useful in two cases:

  • if a function has no effects beyond returning a certain result, i.e. is pure. If the result is not used, the call is certainly useless. On the other hand, discarding the result would not be incorrect.

  • if the return value must be checked, e.g. for a C-like interface that returns error codes instead of throwing. This is the primary use case. For idiomatic C++, that's going to be quite rare.

These two cases leave a huge space of impure functions that do return a value, where such a warning would be misleading. For example:

  • Consider a queue data type with a .pop() method that removes an element and returns a copy of the removed element. Such a method is often convenient. However, there are some cases where we only want to remove the element, without getting a copy. That is perfectly legitimate, and a warning would not be helpful. A different design (such as std::vector) splits these responsibilities, but that has other tradeoffs.

    Note that in some cases, a copy has to be made anyway so thanks to RVO returning the copy would be free.

  • Consider fluent interfaces, where each operation returns the object so that further operations can be performed. In C++, the most common example is the stream insertion operator <<. It would be extremely cumbersome to add a [[nodiscard]] attribute to each and every << overload.

These examples demonstrate that making idiomatic C++ code compile without warnings under a “C++17 with nodiscard by default” language would be quite tedious.

Note that your shiny new C++17 code (where you can use these attributes) may still be compiled together with libraries that target older C++ standards. This backwards compatibility is crucial for the C/C++ ecosystem. So making nodiscard the default would result in many warnings for typical, idiomatic use cases – warnings which you cannot fix without far-reaching changes to the library's source code.

Arguably, the problem here isn't the change of semantics but that the features of each C++ standard apply on a per-compilation-unit scope and not on a per-file scope. If/when some future C++ standard moves away from header files, such a change would be more realistic.

Malice
  • 129
  • 7
amon
  • 132,749
  • 27
  • 279
  • 375
  • 10
    I've upvoted this, as it contains useful insights. Not yet sure if it really answers the question, though. Re impure functions like `pop` or `operator<<`, those would be explicitly marked by my imaginary `[[maydiscard]]` attribute, so no warnings would be generated. Are you saying such functions are generally much more *common* than I'd like to believe, or much more common in general than in my own codebases? Your first paragraph about existing code is certainly true, but still makes me wonder if anyone else is currently peppering all their *new* code with `[[nodiscard]]`, and if not, why not. – Christian Hackl Dec 30 '17 at 18:22
  • 1
    @ChristianHackl My answer mostly addresses your idea that nodiscard could be a default. Because of compatibility with existing code, that's not the case. Even new code may link with older libraries, and would therefore have to compile old C++ headers under these changed defaults. I'm not saying that these cases are overwhelmingly common, but that they can occur in idiomatic code which would lead to undesirable warnings. – amon Dec 30 '17 at 19:46
  • 33
    @ChristianHackl: There was a time in C++'s history where it was considered good practice to return values as const. You can't say `returns_an_int() = 0`, so why should `returns_a_str() = ""` work? C++11 showed this was actually a terrible idea, because now all this code was prohibiting moves because the values were const. I think the proper take-away from that lesson is "it's not up to you what your callers do with your result". Ignoring the result is a perfectly valid thing that callers might want to do, and unless you _know_ it's wrong (a very high bar), you shouldn't block it. – GManNickG Dec 30 '17 at 20:34
  • 2
    @GManNickG: So you think adding `[[nodiscard]]` to functions like `std::vector::empty` is actually a bad decision, and the attribute should be reserved for critical stuff like `std::async`? I don't think the comparison with `const` return values holds, though. To my knowledge, there is absolutely nothing you as a caller can do about a `const` return value preventing moves, but you *can* always do something about a wrong `[[nodiscard]]` warning; you just have to add a cast to `void` at the call site. – Christian Hackl Dec 30 '17 at 20:45
  • 2
    `std::vector::empty()` is pure, so it falls under the category of "you know the use is erroneous when the result is discarded": At least in my book, doing something pointless *is* a bug. And the analogy with `const` return values is better than you think: You could cast away constness the same way that you can cast away the `[[nodiscard]]` attribute - in both cases, the resulting code has a strong stench, though. – cmaster - reinstate monica Dec 30 '17 at 21:58
  • 17
    A nodiscard on `empty()` is also sensible because the name is quite ambiguous: is it a predicate `is_empty()` or is it a mutator `clear()`? You wouldn't typically expect such a mutator to return anything, so a warning about a return value can alert the programmer to a problem. With a clearer name, this attribute would not be as necessary. – amon Dec 30 '17 at 22:02
  • 15
    @ChristianHackl: As others have said, I think pure functions a good example of when this should be used. I would go a step further and say there should be a `[[pure]]` attribute, and it should imply `[[nodiscard]]`. That way it's clear _why_ this result should not be discarded. But other than pure functions, I can't think of another class of functions that should have this behavior. And most functions aren't pure, hence I don't think nodiscard as a default is the right choice. – GManNickG Dec 30 '17 at 22:46
  • 1
    Agreed, this would be extremely annoying to apply in a widespread manner to even `std::` functions. Just as an example (someone's already mentioned `std::map::insert`), algorithms like `rotate` return an iterator, but most times I won't need the return value. – Will Crawford Dec 31 '17 at 01:57
  • @GManNickG: There's a ton of code out there that uses error codes instead of exceptions for a variety of reasons. – Mooing Duck Dec 31 '17 at 07:34
  • @MooingDuck: Agreed. Pure functions are an easy target because we know that if the compiler were smart enough to see it was pure (or notified by an attribute), the optimizer would simply remove the call. Hence a pure function call without using the result is certainly pointless at best, and a bug at worst. But are you willing to say that _all_, or even a majority of error codes must be checked by default? I know I try to in my own code, but I don't know if it's possible to have surveyed enough code to be able to justify that as a default. – GManNickG Dec 31 '17 at 07:38
  • 9
    @GManNickG: Having attempted and failed to debug code that ignored error codes, I _strongly_ believe the vast majority of error codes need to be checked by default. – Mooing Duck Dec 31 '17 at 09:10
  • 4
    Op<< is a bad example, since you always have one unused return at the end. The attribute is for when it does harm to not save the return, i.e. when the dtor of the returned object does something – PlasmaHH Dec 31 '17 at 11:34
  • @cmaster: A `const_cast`, if this is what you're referring to, may result in undefined behaviour, though. A cast to `void` for a `[[nodiscard]]` return value doesn't have that problem. – Christian Hackl Dec 31 '17 at 12:36
  • I've accepted this as the answer, although I'm afraid the question itself has turned out to be a slightly more opinion-based topic than anticipated. However, my personal conclusion from all answers and comments is that (1) `[[nodiscard]]` in public APIs is almost always a bad idea, (2) C++20 `[[nodiscard]]`s in standard-library functions will likely be a disruptive change for some, (3) verbosity seems to be the only tangible disadvantage of `[[nodiscard]]` in internal code with few pure functions, (4) actual module support in some future C++ standard may make the attribute easier to apply. – Christian Hackl Dec 31 '17 at 12:46
  • 1
    @GManNickG: That's a pretty good idea. I've always thought there should be a [[pure]] attribute as an optimizer hint, to the point where you could place it on any conceptually pure function and the compiler would treat it as pure (hoist the call out of loops, etc.) even though it can see it is not bitwise pure. – Joshua Dec 31 '17 at 19:58
  • @ChristianHackl Likewise, the `(void)` cast may lead to resources being leaked, or data not being properly flushed to disk. In short, anything that people may want to write into a destructor may be skipped due to the caller ignoring a returned object pointer. True, this is not quite as bad as getting UB, yet it can become rather nasty, anyway. In both cases, the erroneous behavior, be it UB, leaked resources, or other incomplete cleanup, is masked by an explicit instruction to the compiler to act against its own better knowledge. As such, I'd try to avoid those explicit instructions (= casts). – cmaster - reinstate monica Jan 01 '18 at 01:28
  • 1
    Another important place to put `[[nodiscard]]`: if a function is dynamically allocating a resource that the caller needs to later free. It is nearly never appropriate to discard the return value from a `malloc` or `new`. – AJMansfield Jan 01 '18 at 05:27
  • 1
    @cmaster: I mostly agree, but I don't think it has something to do with destructors. The destructor is invoked anyway for a discarded value; I think it's more of a problem if you as the caller are *not* supposed to rely on the destructor but should manually release a resource (as is typically the case with C APIs). – Christian Hackl Jan 01 '18 at 11:55
  • 1
    @ChristianHackl We seem to be agreeing more than you think. I said "anything that people **may want** to write into a destructor", not "anything that's written into a destructor". Meaning, any cleanup code that people using RAII would always put into their destructors to allow C++ to automatically clean up, but which cannot be written into an automatically called destructor for one reason or the other. Think interfacing with C libraries, etc. If your C library returns a pointer to some object, ignoring the return value would mean not calling its destructing code, whatever that may be. – cmaster - reinstate monica Jan 01 '18 at 13:52
  • 1
    When two bullet points explain `[[nodiscard]]` way better than a full C++ reference page … +1 for your answer, @ChristianHackl – Simon A. Eugster Mar 30 '22 at 19:51
23

Reasons I wouldn't [[nodiscard]] almost everywhere:

  1. (major:) This would introduce way too much noise in my headers.
  2. (major:) I don't feel like I should make strong speculative assumptions about other people's code. You wanna discard the return value I give you? Fine, knock yourself out.
  3. (minor:) You would be guaranteeing incompatibility with C++14

Now, if you made it default, you would be forcing all library developers to force all their users to not-discard return values. That would be horrible. Or you force them to add [[maydiscard]] in innumerably many functions, which is also kind of horrible.

einpoklum
  • 2,478
  • 1
  • 13
  • 30
  • 1
    point 2 is countered by `(void)` – Noone AtAll Aug 15 '21 at 17:34
  • 1
    @NooneAtAll: It is not. – einpoklum Aug 15 '21 at 17:49
  • 3
    @einpoklum Respectfully, it is. `[[nodiscard]]` provides a strong suggestion that the value should not be discarded. If the end-user *really* wants to discard they can suppress the warning with a void cast. The point in the top answer about functions with no effects beyond returning a result is really important: Discarding such a result is almost certainly a mistake and `[[nodiscard]]` will catch it. Lastly, you don't guarantee incompatibility with C++14: https://godbolt.org/z/WzaoasEjo. Since GCC 7, Clang 3.9, and MSVC v19.24 the attribute is silently ignored if compiling with < C++17. – qz- Feb 20 '22 at 18:28
  • @qz That "strong suggest" is there whether the user is able to ignore the suggestion or not. (void) lets the user ignore the suggestion - but I would still be making the suggestion, and like I said - I don't believe I have a basis making it – einpoklum Mar 06 '23 at 21:37
  • "You wanna discard the return value" makes it sound like discarding the return value is a conscious decision on the programmer's part, when the problem is that it is often an error/oversight, unless there is a `(void)` tag explicitly added to the call, in which case you can be pretty sure it was a conscious decision. – Jeremy Friesner Apr 14 '23 at 19:49
  • @JeremyFriesner: Sometimes it is - and I don't want to make that call. – einpoklum Apr 15 '23 at 07:53
  • @einpoklum I agree, the author of the calling code should be the one to make that call. The way they make the call is by either using the return value, or prepending `(void)`. Otherwise there's no way to tell what their decision was, or if they even made a decision. – Jeremy Friesner Apr 15 '23 at 14:07
10

My opinion on [[nodiscard]] is that it should be used in places where lack of taking the result likely leads to an incorrect behavior of a program, and not just some redundant computation.

That's why, I wouldn't put [[nodiscard]] on a getter or a result of a pure function. But I would put on those functions where the lifetime of the result matters. malloc-like function comes to mind, but most likely also most of the functions returning a new unique-pointer, or other RAII objects.

Case in point: I was working with a highly reactive code. On every observable you could attach an arbitrary lambda:

value.observe([](auto value) {...})

The semantic was - whenever the content of value was changed, that lambda was called. The catch was - observe was returning a Handle which controlled how long the lambda was observing the value. When the handle was destroyed, the lambda was detaching itself from the observed value.

It was very easy to forget about keeping the handle. But calling observe without keeping the result effectively had no effect - a major semantic departure from the intention, beyond just a minor inefficiency. In this scenaro [[nodiscard]] was a godsend, and I believe these are the scenarios where it should be used. Not everywhere.

CygnusX1
  • 201
  • 2
  • 3
6

Most C++ compilers have an option that can be used to enable a warning every time a returned value is ignored. For instance, in g++ you can use -Wunused-result to get these warnings.

The [[nodiscard]] attribute has been added to allow a middle ground between always producing a warning and never producing one, which is why most descriptions of how to use it take the approach of being selective with it.

occipita
  • 209
  • 2
  • 5
  • `-Wunused-result` is turned on by default. And it is not "universal", it works only for functions marked with the attribute `warn_unused_result`. Therefore it is not better in any way than `[[ nodiscard ]]`. https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html – MacDada Apr 15 '23 at 22:13
1

As an example: operator<< has a return value that is depending on the call either absolutely needed or absolutely useless. (std::cout << x << y, the first is needed because it returns the stream, the second is of no use at all). Now compare with printf where everyone discards the return value which is there for error checking, but then operator<< has no error checking so it can’t have been that useful in the first place. So in both cases nodiscard would just be damaging.

gnasher729
  • 42,090
  • 4
  • 59
  • 119