Implement P1771

As passed in the Cologne meeting and treated by Core as a DR, [[nodiscard]] was applied to constructors so that they can be diagnosed in cases where the user forgets a variable name for a type. The intent is to enable the library to start using this on the constructors of scope_guard/lock_guard. Differential Revision: https://reviews.llvm.org/D64914 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@367027 91177308-0d34-0410-b5e6-96231b3b80d8
vtkacenko · Jul 25, 2019 · 609d9a6 · 609d9a6
1 parent a6c6faa
commit 609d9a6
Show file tree

Hide file tree

Showing 9 changed files with 158 additions and 20 deletions.
diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
@@ -2335,12 +2335,19 @@ def WarnUnused : InheritableAttr {
 }
 
 def WarnUnusedResult : InheritableAttr {
-  let Spellings = [CXX11<"", "nodiscard", 201603>, C2x<"", "nodiscard">,
+  let Spellings = [CXX11<"", "nodiscard", 201907>, C2x<"", "nodiscard">,
                    CXX11<"clang", "warn_unused_result">,
                    GCC<"warn_unused_result">];
   let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
   let Args = [StringArgument<"Message", 1>];
   let Documentation = [WarnUnusedResultsDocs];
+  let AdditionalMembers = [{
+    // Check whether this the C++11 nodiscard version, even in non C++11
+    // spellings.
+    bool IsCXX11NoDiscard() const {
+      return this->getSemanticSpelling() == CXX11_nodiscard;
+    }
+  }];
 }
 
 def Weak : InheritableAttr {

diff --git a/include/clang/Basic/AttrDocs.td b/include/clang/Basic/AttrDocs.td
@@ -1500,6 +1500,33 @@ in any resulting diagnostics.
   }
   error_info &foo();
   void f() { foo(); } // Does not diagnose, error_info is a reference.
+
+Additionally, discarded temporaries resulting from a call to a constructor
+marked with ``[[nodiscard]]`` or a constructor of a type marked
+``[[nodiscard]]`` will also diagnose. This also applies to type conversions that
+use the annotated ``[[nodiscard]]`` constructor or result in an annotated type.
+
+.. code-block: c++
+  struct [[nodiscard]] marked_type {/*..*/ };
+  struct marked_ctor {
+    [[nodiscard]] marked_ctor();
+    marked_ctor(int);
+  };
+
+  struct S {
+    operator marked_type() const;
+    [[nodiscard]] operator int() const;
+  };
+
+  void usages() {
+    marked_type(); // diagnoses.
+    marked_ctor(); // diagnoses.
+    marked_ctor(3); // Does not diagnose, int constructor isn't marked nodiscard.
+
+    S s;
+    static_cast<marked_type>(s); // diagnoses
+    (int)s; // diagnoses
+  }
   }];
 }
 

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7430,6 +7430,12 @@ def warn_unused_container_subscript_expr : Warning<
 def warn_unused_call : Warning<
   "ignoring return value of function declared with %0 attribute">,
   InGroup<UnusedValue>;
+def warn_unused_constructor : Warning<
+  "ignoring temporary created by a constructor declared with %0 attribute">,
+  InGroup<UnusedValue>;
+def warn_unused_constructor_msg : Warning<
+  "ignoring temporary created by a constructor declared with %0 attribute: %1">,
+  InGroup<UnusedValue>;
 def warn_side_effects_unevaluated_context : Warning<
   "expression with side effects has no effect in an unevaluated context">,
   InGroup<UnevaluatedExpression>;

diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
@@ -2563,13 +2563,31 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
     if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-      if (Type->hasAttr<WarnUnusedAttr>()) {
+      const auto *WarnURAttr = Type->getAttr<WarnUnusedResultAttr>();
+      if (Type->hasAttr<WarnUnusedAttr>() ||
+          (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
         WarnE = this;
         Loc = getBeginLoc();
         R1 = getSourceRange();
         return true;
       }
     }
+
+    const auto *CE = cast<CXXConstructExpr>(this);
+    if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+      const auto *WarnURAttr = Ctor->getAttr<WarnUnusedResultAttr>();
+      if (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) {
+        WarnE = this;
+        Loc = getBeginLoc();
+        R1 = getSourceRange();
+
+        if (unsigned NumArgs = CE->getNumArgs())
+          R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
+                           CE->getArg(NumArgs - 1)->getEndLoc());
+        return true;
+      }
+    }
+
     return false;
   }
 

diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
@@ -2831,7 +2831,8 @@ static void handleSentinelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 
 static void handleWarnUnusedResult(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (D->getFunctionType() &&
-      D->getFunctionType()->getReturnType()->isVoidType()) {
+      D->getFunctionType()->getReturnType()->isVoidType() &&
+      !isa<CXXConstructorDecl>(D)) {
     S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;
     return;
   }

diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
@@ -196,6 +196,25 @@ static bool DiagnoseUnusedComparison(Sema &S, const Expr *E) {
   return true;
 }
 
+static bool DiagnoseNoDiscard(Sema &S, const WarnUnusedResultAttr *A,
+                              SourceLocation Loc, SourceRange R1,
+                              SourceRange R2, bool IsCtor) {
+  if (!A)
+    return false;
+  StringRef Msg = A->getMessage();
+
+  if (Msg.empty()) {
+    if (IsCtor)
+      return S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+    return S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+  }
+
+  if (IsCtor)
+    return S.Diag(Loc, diag::warn_unused_constructor_msg) << A << Msg << R1
+                                                          << R2;
+  return S.Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
+}
+
 void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   if (const LabelStmt *Label = dyn_cast_or_null<LabelStmt>(S))
     return DiagnoseUnusedExprResult(Label->getSubStmt());
@@ -254,19 +273,19 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
     return;
 
   E = WarnExpr;
+  if (const auto *Cast = dyn_cast<CastExpr>(E))
+    if (Cast->getCastKind() == CK_NoOp ||
+        Cast->getCastKind() == CK_ConstructorConversion)
+      E = Cast->getSubExpr()->IgnoreImpCasts();
+
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     if (E->getType()->isVoidType())
       return;
 
-    if (const auto *A = cast_or_null<WarnUnusedResultAttr>(
-            CE->getUnusedResultAttr(Context))) {
-      StringRef Msg = A->getMessage();
-      if (!Msg.empty())
-        Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
-      else
-        Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+    if (DiagnoseNoDiscard(*this, cast_or_null<WarnUnusedResultAttr>(
+                                     CE->getUnusedResultAttr(Context)),
+                          Loc, R1, R2, /*isCtor=*/false))
       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
@@ -284,24 +303,34 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
         return;
       }
     }
+  } else if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
+    if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+      const auto *A = Ctor->getAttr<WarnUnusedResultAttr>();
+      A = A ? A : Ctor->getParent()->getAttr<WarnUnusedResultAttr>();
+      if (DiagnoseNoDiscard(*this, A, Loc, R1, R2, /*isCtor=*/true))
+        return;
+    }
+  } else if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
+    if (const TagDecl *TD = ILE->getType()->getAsTagDecl()) {
+
+      if (DiagnoseNoDiscard(*this, TD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
+                            R2, /*isCtor=*/false))
+        return;
+    }
   } else if (ShouldSuppress)
     return;
 
+  E = WarnExpr;
   if (const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(E)) {
     if (getLangOpts().ObjCAutoRefCount && ME->isDelegateInitCall()) {
       Diag(Loc, diag::err_arc_unused_init_message) << R1;
       return;
     }
     const ObjCMethodDecl *MD = ME->getMethodDecl();
     if (MD) {
-      if (const auto *A = MD->getAttr<WarnUnusedResultAttr>()) {
-        StringRef Msg = A->getMessage();
-        if (!Msg.empty())
-          Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
-        else
-          Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+      if (DiagnoseNoDiscard(*this, MD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
+                            R2, /*isCtor=*/false))
         return;
-      }
     }
   } else if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) {
     const Expr *Source = POE->getSyntacticForm();

diff --git a/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp b/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -80,6 +80,50 @@ void cxx2a_use() {
   conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
 }
 
+namespace p1771 {
+struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
+struct S {
+  [[nodiscard]] S();
+  [[nodiscard("Don't let that S-Char go!")]] S(char);
+  S(int);
+  [[gnu::warn_unused_result]] S(double);
+  operator ConvertTo();
+  [[nodiscard]] operator int();
+  [[nodiscard("Don't throw away as a double")]] operator double();
+};
+
+struct[[nodiscard("Don't throw me away either!")]] Y{};
+
+void usage() {
+  S();    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+  S(1);
+  S(2.2);
+  Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
+  S s;
+  ConvertTo{}; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+
+// AST is different in C++20 mode, pre-2017 a move ctor for ConvertTo is there
+// as well, hense the constructor warning.
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  (ConvertTo) s;
+  (int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  (S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
+#if __cplusplus >= 201703L
+// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
+#else
+// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+#endif
+  static_cast<ConvertTo>(s);
+  static_cast<int>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  static_cast<double>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw away as a double}}
+}
+}; // namespace p1771
+
 #ifdef EXT
 // expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +135,10 @@ void cxx2a_use() {
 // expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
 // expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@84 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@87 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@91 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-warning@92 {{use of the 'nodiscard' attribute is a C++2a extension}}
+// expected-warning@95 {{use of the 'nodiscard' attribute is a C++2a extension}}
 #endif
diff --git a/test/Preprocessor/has_attribute.cpp b/test/Preprocessor/has_attribute.cpp
@@ -63,7 +63,7 @@ CXX11(unlikely)
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
 // WINDOWS: no_unique_address: 0
-// CHECK: nodiscard: 201603L
+// CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // FIXME(201803L) CHECK: unlikely: 0
 

diff --git a/www/cxx_status.html b/www/cxx_status.html
@@ -664,7 +664,7 @@ <h2 id="cxx17">C++17 implementation status</h2>
     </tr>
       <tr> <!-- from Cologne 2019 -->
         <td><a href="http://wg21.link/p1771r1">P1771R1</a> (<a href="#dr">DR</a>)</td>
-        <td class="none" align="center">No</td>
+        <td class="none" align="center">SVN</td>
       </tr>
     <tr>
       <td><tt>[[maybe_unused]]</tt> attribute</td>