[c++20] P1152R4: warn on any simple-assignment to a volatile lvalue

whose value is not ignored.

We don't warn on all the cases that are deprecated: specifically, we
choose to not warn for now if there are parentheses around the
assignment but its value is not actually used. This seems like a more
defensible rule, particularly for cases like sizeof(v = a), where the
parens are part of the operand rather than the sizeof syntax.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@374135 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticSemaKinds.td

@@ -6654,6 +6654,9 @@ def err_increment_decrement_enum : Error<
 def warn_deprecated_increment_decrement_volatile : Warning<
   "%select{decrement|increment}0 of object of volatile-qualified type %1 "
   "is deprecated">, InGroup<DeprecatedVolatile>;
+def warn_deprecated_simple_assign_volatile : Warning<
+  "use of result of assignment to object of volatile-qualified type %0 "
+  "is deprecated">, InGroup<DeprecatedVolatile>;
 def warn_deprecated_compound_assign_volatile : Warning<
   "compound assignment to object of volatile-qualified type %0 is deprecated">,
   InGroup<DeprecatedVolatile>;

include/clang/Sema/Sema.h

@@ -1062,6 +1062,11 @@ public:
 
     llvm::SmallPtrSet<const Expr *, 8> PossibleDerefs;
 
+    /// Expressions appearing as the LHS of a volatile assignment in this
+    /// context. We produce a warning for these when popping the context if
+    /// they are not discarded-value expressions nor unevaluated operands.
+    SmallVector<Expr*, 2> VolatileAssignmentLHSs;
+
     /// \brief Describes whether we are in an expression constext which we have
     /// to handle differently.
     enum ExpressionKind {
@@ -4248,6 +4253,9 @@ public:
   ExprResult TransformToPotentiallyEvaluated(Expr *E);
   ExprResult HandleExprEvaluationContextForTypeof(Expr *E);
 
+  ExprResult CheckUnevaluatedOperand(Expr *E);
+  void CheckUnusedVolatileAssignment(Expr *E);
+
   ExprResult ActOnConstantExpression(ExprResult Res);
 
   // Functions for marking a declaration referenced.  These functions also

lib/Sema/SemaExpr.cpp

@@ -3801,6 +3801,16 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
   QualType ExprTy = E->getType();
   assert(!ExprTy->isReferenceType());
 
+  bool IsUnevaluatedOperand =
+      (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
+       ExprKind == UETT_PreferredAlignOf);
+  if (IsUnevaluatedOperand) {
+    ExprResult Result = CheckUnevaluatedOperand(E);
+    if (Result.isInvalid())
+      return true;
+    E = Result.get();
+  }
+
   if (ExprKind == UETT_VecStep)
     return CheckVecStepTraitOperandType(*this, ExprTy, E->getExprLoc(),
                                         E->getSourceRange());
@@ -3838,9 +3848,8 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
 
   // The operand for sizeof and alignof is in an unevaluated expression context,
   // so side effects could result in unintended consequences.
-  if ((ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
-       ExprKind == UETT_PreferredAlignOf) &&
-      !inTemplateInstantiation() && E->HasSideEffects(Context, false))
+  if (IsUnevaluatedOperand && !inTemplateInstantiation() &&
+      E->HasSideEffects(Context, false))
     Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
 
   if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(),
@@ -3939,8 +3948,6 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(QualType ExprType,
 }
 
 static bool CheckAlignOfExpr(Sema &S, Expr *E, UnaryExprOrTypeTrait ExprKind) {
-  E = E->IgnoreParens();
-
   // Cannot know anything else if the expression is dependent.
   if (E->isTypeDependent())
     return false;
@@ -3952,9 +3959,10 @@ static bool CheckAlignOfExpr(Sema &S, Expr *E, UnaryExprOrTypeTrait ExprKind) {
   }
 
   ValueDecl *D = nullptr;
-  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
+  Expr *Inner = E->IgnoreParens();
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Inner)) {
     D = DRE->getDecl();
-  } else if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
+  } else if (MemberExpr *ME = dyn_cast<MemberExpr>(Inner)) {
     D = ME->getMemberDecl();
   }
 
@@ -11944,7 +11952,7 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
       //   A simple-assignment whose left operand is of a volatile-qualified
       //   type is deprecated unless the assignment is either a discarded-value
       //   expression or an unevaluated operand
-      // FIXME: Implement checks for this.
+      ExprEvalContexts.back().VolatileAssignmentLHSs.push_back(LHSExpr);
     } else {
       // C++2a [expr.ass]p6:
       //   [Compound-assignment] expressions are deprecated if E1 has
@@ -15082,6 +15090,26 @@ void Sema::WarnOnPendingNoDerefs(ExpressionEvaluationContextRecord &Rec) {
   Rec.PossibleDerefs.clear();
 }
 
+/// Check whether E, which is either a discarded-value expression or an
+/// unevaluated operand, is a simple-assignment to a volatlie-qualified lvalue,
+/// and if so, remove it from the list of volatile-qualified assignments that
+/// we are going to warn are deprecated.
+void Sema::CheckUnusedVolatileAssignment(Expr *E) {
+  if (!E->getType().isVolatileQualified() || !getLangOpts().CPlusPlus2a)
+    return;
+
+  // Note: ignoring parens here is not justified by the standard rules, but
+  // ignoring parentheses seems like a more reasonable approach, and this only
+  // drives a deprecation warning so doesn't affect conformance.
+  if (auto *BO = dyn_cast<BinaryOperator>(E->IgnoreParenImpCasts())) {
+    if (BO->getOpcode() == BO_Assign) {
+      auto &LHSs = ExprEvalContexts.back().VolatileAssignmentLHSs;
+      LHSs.erase(std::remove(LHSs.begin(), LHSs.end(), BO->getLHS()),
+                 LHSs.end());
+    }
+  }
+}
+
 void Sema::PopExpressionEvaluationContext() {
   ExpressionEvaluationContextRecord& Rec = ExprEvalContexts.back();
   unsigned NumTypos = Rec.NumTypos;
@@ -15116,6 +15144,13 @@ void Sema::PopExpressionEvaluationContext() {
 
   WarnOnPendingNoDerefs(Rec);
 
+  // Warn on any volatile-qualified simple-assignments that are not discarded-
+  // value expressions nor unevaluated operands (those cases get removed from
+  // this list by CheckUnusedVolatileAssignment).
+  for (auto *BO : Rec.VolatileAssignmentLHSs)
+    Diag(BO->getBeginLoc(), diag::warn_deprecated_simple_assign_volatile)
+        << BO->getType();
+
   // When are coming out of an unevaluated context, clear out any
   // temporaries that we may have created as part of the evaluation of
   // the expression in that context: they aren't relevant because they

lib/Sema/SemaExprCXX.cpp

@@ -499,6 +499,11 @@ ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType,
       }
     }
 
+    ExprResult Result = CheckUnevaluatedOperand(E);
+    if (Result.isInvalid())
+      return ExprError();
+    E = Result.get();
+
     // C++ [expr.typeid]p4:
     //   [...] If the type of the type-id is a reference to a possibly
     //   cv-qualified type, the result of the typeid expression refers to a
@@ -6609,6 +6614,11 @@ ExprResult Sema::ActOnDecltypeExpression(Expr *E) {
   ExprEvalContexts.back().ExprContext =
       ExpressionEvaluationContextRecord::EK_Other;
 
+  Result = CheckUnevaluatedOperand(E);
+  if (Result.isInvalid())
+    return ExprError();
+  E = Result.get();
+
   // In MS mode, don't perform any extra checking of call return types within a
   // decltype expression.
   if (getLangOpts().MSVCCompat)
@@ -7233,7 +7243,10 @@ ExprResult Sema::BuildCXXNoexceptExpr(SourceLocation KeyLoc, Expr *Operand,
   if (R.isInvalid())
     return R;
 
-  // The operand may have been modified when checking the placeholder type.
+  R = CheckUnevaluatedOperand(R.get());
+  if (R.isInvalid())
+    return ExprError();
+
   Operand = R.get();
 
   if (!inTemplateInstantiation() && Operand->HasSideEffects(Context, false)) {
@@ -7337,12 +7350,17 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) {
     // volatile lvalue with a special form, we perform an lvalue-to-rvalue
     // conversion.
     if (getLangOpts().CPlusPlus11 && E->isGLValue() &&
-        E->getType().isVolatileQualified() &&
-        IsSpecialDiscardedValue(E)) {
-      ExprResult Res = DefaultLvalueConversion(E);
-      if (Res.isInvalid())
-        return E;
-      E = Res.get();
+        E->getType().isVolatileQualified()) {
+       if (IsSpecialDiscardedValue(E)) {
+        ExprResult Res = DefaultLvalueConversion(E);
+        if (Res.isInvalid())
+          return E;
+        E = Res.get();
+      } else {
+        // Per C++2a [expr.ass]p5, a volatile assignment is not deprecated if
+        // it occurs as a discarded-value expression.
+        CheckUnusedVolatileAssignment(E);
+      }
     }
 
     // C++1z:
@@ -7377,6 +7395,14 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) {
   return E;
 }
 
+ExprResult Sema::CheckUnevaluatedOperand(Expr *E) {
+  // Per C++2a [expr.ass]p5, a volatile assignment is not deprecated if
+  // it occurs as an unevaluated operand.
+  CheckUnusedVolatileAssignment(E);
+
+  return E;
+}
+
 // If we can unambiguously determine whether Var can never be used
 // in a constant expression, return true.
 //  - if the variable and its initializer are non-dependent, then

test/SemaCXX/deprecated.cpp

@@ -1,13 +1,17 @@
-// RUN: %clang_cc1 -std=c++98 %s -Wdeprecated -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++14 %s -Wdeprecated -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++17 %s -Wdeprecated -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++2a %s -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++98 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++11 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++17 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++2a %s -Wno-parentheses -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu
 
-// RUN: %clang_cc1 -std=c++14 %s -Wdeprecated -verify -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS
+// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS
 
 #include "Inputs/register.h"
 
+namespace std {
+  struct type_info {};
+}
+
 void g() throw();
 void h() throw(int);
 void i() throw(...);
@@ -133,17 +137,36 @@ namespace DeprecatedVolatile {
     n = 5; // ok
 #if __cplusplus >= 201103L
     decltype(n = 5) m = n; // ok expected-warning {{side effects}}
-    m = sizeof(n = 5); // ok expected-warning {{side effects}}
+    (void)noexcept(n = 5); // ok expected-warning {{side effects}}
 #endif
+    (void)typeid(n = 5); // ok expected-warning {{side effects}}
     (n = 5, 0); // ok
-    use(n = 5); // FIXME: deprecated
-    (n = 5); // FIXME: deprecated
-    int q = n = 5; // FIXME: deprecated
-    q = n = 5; // FIXME: deprecated
+    use(n = 5); // cxx20-warning {{use of result of assignment to object of volatile-qualified type 'volatile int' is deprecated}}
+    int q = n = 5; // cxx20-warning {{deprecated}}
+    q = n = 5; // cxx20-warning {{deprecated}}
 #if __cplusplus >= 201103L
-    decltype(q = n = 5) m2 = q; // FIXME: deprecated expected-warning {{side effects}}
+    decltype(q = n = 5) m2 = q; // cxx20-warning {{deprecated}} expected-warning {{side effects}}
+    (void)noexcept(q = n = 5); // cxx20-warning {{deprecated}} expected-warning {{side effects}}
 #endif
-    q = sizeof(q = n = 5); // FIXME: deprecated expected-warning {{side effects}}
+    (void)sizeof(q = n = 5); // cxx20-warning {{deprecated}} expected-warning {{side effects}}
+    (void)typeid(use(n = 5)); // cxx20-warning {{deprecated}} expected-warning {{side effects}}
+    (void)__alignof(+(n = 5)); // cxx20-warning {{deprecated}} expected-warning {{side effects}}
+
+    // FIXME: These cases are technically deprecated because the parens are
+    // part of the operand, but we choose to not diagnose for now.
+    (void)sizeof(n = 5); // expected-warning {{side effects}}
+    (void)__alignof(n = 5); // expected-warning {{side effects}}
+    // Similarly here.
+    (n = 5);
+
+    volatile bool b = true;
+    if (b = true) {} // cxx20-warning {{deprecated}}
+    for (b = true;
+         b = true; // cxx20-warning {{deprecated}}
+         b = true) {}
+    for (volatile bool x = true;
+         volatile bool y = true; // ok despite volatile load from volatile initialization
+        ) {}
 
     // inc / dec / compound assignments are always deprecated
     ++n; // cxx20-warning {{increment of object of volatile-qualified type 'volatile int' is deprecated}}

www/cxx_status.html

@@ -1103,7 +1103,7 @@ as the draft C++2a standard evolves.
     <tr>
       <td>Deprecate some problematic uses of <tt>volatile</tt></td>
       <td><a href="http://wg21.link/p1152r4">P1152R4</a></td>
-      <td class="partial" align="center">Partial</td>
+      <td class="svn" align="center">SVN</td>
     </tr>
     <tr>
       <td><tt>[[nodiscard("with reason")]]</tt></td>