[Sema] -Wunused-value: diagnose unused std::move() call results.

[lebedev.ri]

Summary

I have seen such a problem when reviewing D43341.
https://godbolt.org/g/aJYcaa

#include <utility>

struct S {};

void test(S a) {
    std::move(a);
}

Since std::move() is not marked with nodiscard attribute in the standard
(should it be? how complicated would it be to write such a proposal?),
nothing diagnoses such code. But i really don't see why one would intentionally write that.
You have either forgot to assign/pass the result, or you wanted to cast to void.

Stage-2 self-hosting is green, no preparatory changes needed!

[rjmccall]

LGTM. I think it wouldn't be unreasonable to ask standard library maintainers to add [[nodiscard]] to std::move, but it's also not unreasonable for us to special-case some functions.

[thakis]

(See also https://bugs.llvm.org/show_bug.cgi?id=10011 for a somewhat related discussion.)

[rjmccall]

Yeah, actually, I'm second-guessing myself. Maybe this should just be a libc++ / libstdc++ bug.

[mclow.lists]

So.
https://bugs.llvm.org/show_bug.cgi?id=10011 was resolved by https://wg21.link/P0600 (which added [[nodiscard]] to string.empty()

We can do the same for move.
However, I have been promised a comprehensive paper listing all the (100s?) of places in the standard library which should be so marked, and I'm inclined to wait for it (but not for too long).
[ The next WG21 meeting is in June ]

[rjmccall]

That seems reasonable. And this summer would still be in time for the next release.

[• Quuxplusone]

I think it wouldn't be unreasonable to ask standard library maintainers to add [[nodiscard]] to std::move

+1. Also std::forward, for sure. Basically any metaprogramming function that is statically known to return a reference to its argument.
This knowledge is already in the brains of library vendors; they ought to just write it down in the form of [[nodiscard]] annotations. This PR smells like Clang is trying to "cut out the middleman" and just make up its own proprietary list of metaprogramming functions that are statically known to return references to arguments, which is not the most effective or futureproof way to go about it. Let the library vendors deal with it, please, says me.

[lebedev.ri]

In D45163#1054925, @Quuxplusone wrote:

I think it wouldn't be unreasonable to ask standard library maintainers to add [[nodiscard]] to std::move

+1. Also std::forward, for sure. Basically any metaprogramming function that is statically known to return a reference to its argument.
This knowledge is already in the brains of library vendors; they ought to just write it down in the form of [[nodiscard]] annotations. This PR smells like Clang is trying to "cut out the middleman" and just make up its own proprietary list of metaprogramming functions that are statically known to return references to arguments, which is not the most effective or futureproof way to go about it. Let the library vendors deal with it, please, says me.

A few updates:

• std::move() *should* be a part of the next paper about [[nodiscard]]. Yay!
• D45179 *should* make those attributes usable in pre-C++17 code. For people using libc++.

But, some points still remain:

• std::move() is not [[nodiscard]] yet
• D45179 is not merged yet
• Even when either/both things happen, it won't really help those not using libc++ [trunk] / C++17-or-later. That is a *lot* of people.

I do agree with @Quuxplusone, we wouldn't want to pull the list of all the [[nodiscard]] functions from the standard into clang just because. (we could do that with clang-tidy)
But maybe std::move() is a bit special in this regard... ?

[• Quuxplusone]

std::move would definitely be special in this regard if there were a pressing benefit to be gained — i.e., if people were currently getting bitten by accidentally discarded calls of std::move(x). But you haven't shown that people are getting bitten today; in fact I think you said the opposite, right? that there were *no* instances of this happening in the real codebases you tested? So in that case, this diagnostic doesn't have a pressing benefit IMHO, and Clang could safely wait for the library vendors to do the work.

[lebedev.ri]

In D45163#1054994, @Quuxplusone wrote:

std::move would definitely be special in this regard if there were a pressing benefit to be gained — i.e., if people were currently getting bitten by accidentally discarded calls of std::move(x). But you haven't shown that people are getting bitten today; in fact I think you said the opposite, right? that there were *no* instances of this happening in the real codebases you tested? So in that case, this diagnostic doesn't have a pressing benefit IMHO, and Clang could safely wait for the library vendors to do the work.

@Quuxplusone Like i said in the differential's description,

I have seen such a problem when reviewing D43341.

https://reviews.llvm.org/D43341?vs=137245&id=139869&whitespace=ignore-most#toc

Please see clang-doc/Representation.cpp, left hand side of that page, line 21:

static void mergeInfoBase(Info &L, Info &R) {
  assert(L.USR == R.USR);
  assert(L.Name == R.Name);
  if (L.Namespace.empty()) std::move(R.Namespace); // <- here
...

[rjmccall]

Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime. But I'm not sure what the rush is if it doesn't change what goes into any concrete release of the compiler + library.

[lebedev.ri]

In D45163#1055856, @rjmccall wrote:

Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper,

but he might consider taking a patch in the meantime.

That has been discussed, and it's not going to happen. Ask him if you want more info.

But I'm not sure what the rush is if it doesn't change what goes into any concrete release of the compiler + library.

TLDW: std::move() is "The Assign Operator (a = b;) of RAII". It would be good to diagnose such a problem, and not just rely that some day the std libs will mark it with the attribute.
BTW, even when they do, even for libcxx, it won't be of any immediate use to LLVM, we will need to provide a define (see D45179) to actually enable that attribute, because LLVM is using C++11, not C++17/something...
<insert "not everyone uses clang with libc++" rant>

[rjmccall]

Trust me, I understand that this is an important function, but a lot of functions are important, and we're not going to hardcode knowledge about all of them in the compiler.

It seems reasonable to ask libc++ to use __attribute__((warn_unused_result)) if [[nodiscard]] is unavailable.

[• Quuxplusone]

In D45163#1055957, @lebedev.ri wrote:

In D45163#1055856, @rjmccall wrote:

Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime.

That has been discussed, and it's not going to happen. Ask him if you want more info.

You say a library patch is not going to happen, but isn't that library patch literally D45179, which has been accepted (although I just left a comment explaining why the current macro-soup is suboptimal)?

TLDW: std::move() is "The Assign Operator (a = b;) of RAII". It would be good to diagnose such a problem, and not just rely that some day the std libs will mark it with the attribute.
BTW, even when they do, even for libcxx, it won't be of any immediate use to LLVM, we will need to provide a define (see D45179) to actually enable that attribute, because LLVM is using C++11, not C++17/something...

Yes, but that patch (which you've written in D45179) is essentially just "if C++17, [[nodiscard]], else __attribute__((warn_unused_result))." The library vendor is completely free to mark nodiscard-ish function with [[nodiscard]] or any other attribute they want to. And if I'm not mistaken, D45179 is a step in that direction.

[lebedev.ri]

In D45163#1056044, @rjmccall wrote:

It seems reasonable to ask libc++ to use __attribute__((warn_unused_result)) if [[nodiscard]] is unavailable.

D45179 should hopefully add an opt-in define to allow doing just that. But it will then have to be enabled in the LLVM buildsystem.

In D45163#1056074, @Quuxplusone wrote:

In D45163#1055957, @lebedev.ri wrote:

In D45163#1055856, @rjmccall wrote:

Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime.

That has been discussed, and it's not going to happen. Ask him if you want more info.

You say a library patch is not going to happen, but isn't that library patch literally D45179, which has been accepted (although I just left a comment explaining why the current macro-soup is suboptimal)?

We may be talking about different things here.
I'm simply pointing out that libc++ will not be adding nodiscard attribute to functions unless it is mandated by the standard,
and that it *has* to be opt-in for earlier standards. I'm not saying that just to argue, i'm literally retranslating what @mclow.lists has said, it's his words.

docs/ReleaseNotes.rst

 - ``-Wc++98-compat-extra-semi`` is a new flag, which was previously inseparable
   from ``-Wc++98-compat-pedantic``. The latter still controls the new flag.
 
 - ``-Wextra-semi`` now also controls ``-Wc++98-compat-extra-semi``.
   Please do note that if you pass ``-Wno-c++98-compat-pedantic``, it implies
   ``-Wno-c++98-compat-extra-semi``, so if you want that diagnostic, you need
   to explicitly re-enable it (e.g. by appending ``-Wextra-semi``).
 
+- ``-Wunused-value`` diagnostic was taught to detect unused return value of
+  ``std::move()`` function.
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - clang binary and libraries have been renamed from 7.0 to 7.
   For example, the clang binary will be called clang-7 instead of clang-7.0.
 
 - ...
 

include/clang/Basic/DiagnosticSemaKinds.td

  "property access result unused - getters should not be used for side effects">,
   InGroup<UnusedGetterReturnValue>;
 def warn_unused_container_subscript_expr : Warning<
  "container access result unused - container access should not be used for side effects">,
   InGroup<UnusedValue>;
 def warn_unused_call : Warning<
   "ignoring return value of function declared with %0 attribute">,
   InGroup<UnusedValue>;
+def warn_unused_stdmove : Warning<
+  "ignoring return value of std::move() function returning rvalue reference of type %0">,
+  InGroup<UnusedValue>;
+def warn_unused_xvalue_cast : Warning<
+  "ignoring return value of cast to rvalue reference of type %0">,
+  InGroup<UnusedValue>;
 def warn_side_effects_unevaluated_context : Warning<
   "expression with side effects has no effect in an unevaluated context">,
   InGroup<UnevaluatedExpression>;
 def warn_side_effects_typeid : Warning<
   "expression with side effects will be evaluated despite being used as an "
   "operand to 'typeid'">, InGroup<PotentiallyEvaluatedExpression>;
 def warn_unused_result : Warning<
   "ignoring return value of function declared with %0 attribute">,

lib/AST/Expr.cpp

if (const Decl *FD = CE->getCalleeDecl()) {
       bool HasWarnUnusedResultAttr = Func ? Func->hasUnusedResultAttr()
                                           : FD->hasAttr<WarnUnusedResultAttr>();
 
       // If the callee has attribute pure, const, or warn_unused_result, warn
       // about it. void foo() { strlen("bar"); } should warn.
       //
       // Note: If new cases are added here, DiagnoseUnusedExprResult should be
       // updated to match for QoI.
-      if (HasWarnUnusedResultAttr ||
+      if (CE->isCallToStdMove() || HasWarnUnusedResultAttr ||
           FD->hasAttr<PureAttr>() || FD->hasAttr<ConstAttr>()) {
         WarnE = this;
         Loc = CE->getCallee()->getLocStart();
         R1 = CE->getCallee()->getSourceRange();
 
         if (unsigned NumArgs = CE->getNumArgs())
           R2 = SourceRange(CE->getArg(0)->getLocStart(),
                            CE->getArg(NumArgs-1)->getLocEnd());

lib/Sema/SemaStmt.cpp

if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     if (E->getType()->isVoidType())
       return;
 
     // If the callee has attribute pure, const, or warn_unused_result, warn with
     // a more specific message to make it clear what is happening. If the call
     // is written in a macro body, only warn if it has the warn_unused_result
     // attribute.
     if (const Decl *FD = CE->getCalleeDecl()) {
+      if (CE->isCallToStdMove()) {
+        Diag(Loc, diag::warn_unused_stdmove) << E->getType().getCanonicalType();
+        return;
+      }
       if (const Attr *A = isa<FunctionDecl>(FD)
                               ? cast<FunctionDecl>(FD)->getUnusedResultAttr()
                               : FD->getAttr<WarnUnusedResultAttr>()) {
         Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
         return;
       }
       if (ShouldSuppress)
         return;
if (T == Context.VoidPtrTy) {
       PointerTypeLoc TL = TI->getTypeLoc().castAs<PointerTypeLoc>();
 
       Diag(Loc, diag::warn_unused_voidptr)
         << FixItHint::CreateRemoval(TL.getStarLoc());
       return;
     }
   }
 
+  if (E->isXValue() && isa<CastExpr>(E)) {
+    Diag(Loc, diag::warn_unused_xvalue_cast) << E->getType().getCanonicalType();
+    return;
+  }
+
   if (E->isGLValue() && E->getType().isVolatileQualified()) {
     Diag(Loc, diag::warn_unused_volatile) << R1 << R2;
     return;
   }
 
   DiagRuntimeBehavior(Loc, nullptr, PDiag(DiagID) << R1 << R2);
 }
 

test/Analysis/MisusedMovedObject.cpp

void reinitializationTest(int i) {
     if (i == 2) { // expected-note {{Assuming 'i' is not equal to 2}} expected-note {{Taking false branch}}
       //expected-note@-1 {{Assuming 'i' is not equal to 2}} expected-note@-1 {{Taking false branch}}
       a.foo();    // no-warning
     }
   }
   {
     A a;
     if (i == 1) { // expected-note {{Taking false branch}} expected-note {{Taking false branch}}
-      std::move(a);
+      std::move(a); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'A'}}
     }
     if (i == 2) { // expected-note {{Taking false branch}} expected-note {{Taking false branch}}
       a = A();
       a.foo();
     }
   }
   // The built-in assignment operator should also be recognized as a
   // reinitialization. (std::move() may be called on built-in types in template
void templateArgIsNotUseTest() {
     A a;
     functionTemplate<sizeof(A(std::move(a)))>(std::move(a)); // no-warning
   }
 }
 
 // Moves of global variables are not reported.
 A global_a;
 void globalVariablesTest() {
-  std::move(global_a);
+  std::move(global_a); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'A'}}
   global_a.foo(); // no-warning
 }
 
 // Moves of member variables.
 class memberVariablesTest {
   A a;
   static A static_a;
 

test/SemaCXX/warn-unused-result-nodiscard.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+// definitions for somthing similar to the std::move.
+namespace notstd {
+inline namespace foo {
+template <class T>
+struct remove_reference { typedef T type; };
+template <class T>
+struct remove_reference<T &> { typedef T type; };
+template <class T>
+struct remove_reference<T &&> { typedef T type; };
+
+// This is just like the std::move(), but marked with warn_unused_result attribute
+template <class T>
+__attribute__((warn_unused_result)) typename remove_reference<T>::type &&notmove1(T &&t);
+template <class T>
+[[clang::warn_unused_result]] typename remove_reference<T>::type &&notmove2(T &&t);
+template <class T>
+[[nodiscard]] typename remove_reference<T>::type &&notmove3(T &&t);
+} // namespace foo
+} // namespace notstd
+
+// definitions for std::move, but actually marked with nodiscard attribute
+namespace std {
+inline namespace foo {
+template <class T>
+struct remove_reference { typedef T type; };
+template <class T>
+struct remove_reference<T &> { typedef T type; };
+template <class T>
+struct remove_reference<T &&> { typedef T type; };
+
+template <class T>
+[[nodiscard]] typename remove_reference<T>::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+void arg(int a) {
+  notstd::notmove1(a); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
+  notstd::notmove2(a); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
+  notstd::notmove3(a); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+
+  std::move(a); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+}

test/SemaCXX/warn-unused-xvalue.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template <class T>
+struct remove_reference { typedef T type; };
+template <class T>
+struct remove_reference<T &> { typedef T type; };
+template <class T>
+struct remove_reference<T &&> { typedef T type; };
+
+template <class T>
+typename remove_reference<T>::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+int f0();
+int &&f1();
+
+int global;
+
+void test_negative() {
+  // Ok. Should not warn
+  f0();
+  f1();
+
+  static_cast<int>(f0()); // expected-warning {{expression result unused}}
+  static_cast<int>(f1()); // expected-warning {{expression result unused}}
+
+  static_cast<int>(global); // expected-warning {{expression result unused}}
+
+  (void)std::move(global);
+  (void)noexcept(std::move(global));
+  (void)sizeof(std::move(global));
+}
+
+// Unevaluated contexts should not trigger unused result warnings.
+template <typename T>
+auto foo(T) -> decltype(std::move(global), bool()) { // Should not warn.
+  return true;
+}
+
+void test_simple() {
+  int x = 1;
+
+  static_cast<int &&>(x); // expected-warning {{ignoring return value of cast to rvalue reference of type 'int'}}
+  std::move(x);           // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+
+  using std::move;
+  move(x); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+}
+
+void test_global() {
+  static_cast<int &&>(global); // expected-warning {{ignoring return value of cast to rvalue reference of type 'int'}}
+  std::move(global);           // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+
+  using std::move;
+  move(global); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+}
+
+void test_call() {
+  static_cast<int &&>(f0());   // expected-warning {{ignoring return value of cast to rvalue reference of type 'int'}}
+  static_cast<int &&>(f1());   // expected-warning {{ignoring return value of cast to rvalue reference of type 'int'}}
+  std::move(f0());             // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+  std::move(f1());             // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+
+  using std::move;
+  move(f0()); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+  move(f1()); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+}
+
+void arg(int a) {
+  static_cast<int &&>(a); // expected-warning {{ignoring return value of cast to rvalue reference of type 'int'}}
+
+  std::move(a); // expected-warning {{ignoring return value of std::move() function returning rvalue reference of type 'int'}}
+}
+
+struct A {};
+struct B {
+  A a;
+};
+struct C {
+  C(){};
+  ~C() {}
+};
+void struct_test() {
+  A a;
+  std::move(a); // expected-warning{{ignoring return value of std::move() function returning rvalue reference of type 'A'}}
+
+  B b;
+  std::move(b);   // expected-warning{{ignoring return value of std::move() function returning rvalue reference of type 'B'}}
+  std::move(b.a); // expected-warning{{ignoring return value of std::move() function returning rvalue reference of type 'A'}}
+
+  C c;
+  std::move(c); // expected-warning{{ignoring return value of std::move() function returning rvalue reference of type 'C'}}
+}