[libc++] Mark std::unique_ptr::release() as nodiscard

[ldionne]

It's more often than not a bug to call release() while discarding the value, since the resource would leak. While it's technically possible for the user to purposefully do that, I think [[nodiscard]] in that location will do far more good than harm.

Folks can always explicitly discard the result of the expression or use [[maybe_unused]] if the discarding is intended.

Fixes #30246

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

It's more often than not a bug to call release() while discarding the value, since the resource would leak. While it's technically possible for the user to purposefully do that, I think [[nodiscard]] in that location will do far more good than harm.

Folks can always explicitly discard the result of the expression or use [[maybe_unused]] if the discarding is intended.

Fixes #30246

---

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

5 Files Affected:

• (modified) libcxx/include/__memory/unique_ptr.h (+1-1)
• (modified) libcxx/include/deque (+1-1)
• (modified) libcxx/include/locale (+1-1)
• (added) libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp (+21)
• (modified) libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp (+3-3)

diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 11215dc111e36a..38375518551b4d 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -278,7 +278,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
     return __ptr_ != nullptr;
   }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer release() _NOEXCEPT {
+  [[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer release() _NOEXCEPT {
     pointer __t = __ptr_;
     __ptr_      = pointer();
     return __t;
diff --git a/libcxx/include/deque b/libcxx/include/deque
index bab0526629f0f8..481837260cbd60 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -2187,7 +2187,7 @@ void deque<_Tp, _Allocator>::__add_back_capacity() {
     typedef __allocator_destructor<_Allocator> _Dp;
     unique_ptr<pointer, _Dp> __hold(__alloc_traits::allocate(__a, __block_size), _Dp(__a, __block_size));
     __buf.push_back(__hold.get());
-    __hold.release();
+    (void)__hold.release();
 
     for (__map_pointer __i = __map_.end(); __i != __map_.begin();)
       __buf.push_front(*--__i);
diff --git a/libcxx/include/locale b/libcxx/include/locale
index 573910a85bef54..90265d49ebc328 100644
--- a/libcxx/include/locale
+++ b/libcxx/include/locale
@@ -2459,7 +2459,7 @@ _LIBCPP_HIDE_FROM_ABI void __double_or_nothing(unique_ptr<_Tp, void (*)(void*)>&
   if (__t == 0)
     __throw_bad_alloc();
   if (__owns)
-    __b.release();
+    (void)__b.release();
   __b = unique_ptr<_Tp, void (*)(void*)>(__t, free);
   __new_cap /= sizeof(_Tp);
   __n = __b.get() + __n_off;
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp
new file mode 100644
index 00000000000000..93131a09f2b64f
--- /dev/null
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp
@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: stdlib=libc++
+
+// <memory>
+//
+// unique_ptr
+//
+// constexpr pointer release() noexcept; // nodiscard as an extension
+
+#include <memory>
+
+void f(std::unique_ptr<int> p) {
+  p.release(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
index 7aedac99030cba..0bb906a13dd3df 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
@@ -7,10 +7,10 @@
 //===----------------------------------------------------------------------===//
 
 // <memory>
-
+//
 // unique_ptr
-
-// test release
+//
+// constexpr pointer release() noexcept;
 
 #include <memory>
 #include <cassert>

[ldionne]

CC @llvm/libcxx-vendors since this may cause new diagnostics to be emitted and I'm not sure on what scale we're talking about.

@alexfh Would you be able to take this change for a spin just to see if we get tons of spurious diagnostics?

If this causes too many spurious diagnostics, I'd also be OK with closing #30246 as "wontfix".

[zmodem]

In (Linux) Chromium we hit 116 instances
(list.txt).
That's not an impossible number to fix, but it does suggest that this will be pretty noisy in the wild, so I'm not sure it's worth it?

[davidben]

For the instances in BoringSSL, the context is that we need to interface with C APIs, and our C APIs often have this kinda annoying pattern:

// takes_ownership does some stuff. On success, it takes ownership
// of `thingy` and returns true. On failure, it returns false and the
// caller retains ownership of `thingy`.
bool takes_ownership(Thingy *thingy);

In C++, one would probably write this as taking unique_ptr<Thingy>&&, or just take unique_ptr<Thingy> by value and unconditionally take ownership. (It usually doesn't matter who owns it on error, just someone does. APIs often look like this in C just because it's more natural to write it that way. Either way, the API is what it is and we're stuck with it.)

The best strategy I've come up with for that is:

unique_ptr<Thingy> thingy;
if (!takes_ownership(thingy.get())) {
  return Error();
}
thingy.release();  // takes_ownership took ownership

Definitely not great, but works OK and does the right thing in both cases. But I don't mind adding a (void) in front or perhaps even a wrapper like bssl::TookOwnership(thingy), I dunno. 🤷

[davidben]

Spot-checking another random one, it seems Skia has the same sort of C/C++ boundary to contend with:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkJpegCodec.cpp;drc=ade36690942cc1126513eb4ee58b98f4352a4b55;l=187

[ldionne]

Ack. Just Chromium hitting 116 issues of this tells me that this is going to be way too noisy. We're aiming to add [[nodiscard]] when it's clearly a bug, and this doesn't seem to be a great candidate for that.

I was trying to close a few easy issues and I thought this one would be a good candidate, but looks like this should be closed as NTBF instead.

Thanks for the engagement folks.