Adding a -Wunused-value warning for expressions with side effects use…

…d in an unevaluated expression context, such as sizeof(), or decltype(). Also adds a similar warning when the expression passed to typeid() *is* evaluated, since it is equally likely that the user would expect the expression operand to be unevaluated in that case.

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

include/clang/AST/Expr.h

@@ -586,8 +586,13 @@ class Expr : public Stmt {
 
   /// HasSideEffects - This routine returns true for all those expressions
   /// which have any effect other than producing a value. Example is a function
-  /// call, volatile variable read, or throwing an exception.
-  bool HasSideEffects(const ASTContext &Ctx) const;
+  /// call, volatile variable read, or throwing an exception. If
+  /// IncludePossibleEffects is false, this call treats certain expressions with
+  /// potential side effects (such as function call-like expressions,
+  /// instantiation-dependent expressions, or invocations from a macro) as not
+  /// having side effects.
+  bool HasSideEffects(const ASTContext &Ctx,
+                      bool IncludePossibleEffects = true) const;
 
   /// \brief Determine whether this expression involves a call to any function
   /// that is not trivial.

include/clang/Basic/DiagnosticSemaKinds.td

@@ -6107,6 +6107,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_side_effects_unevaluated_context : Warning<
+  "expression with side effects has no effect in an unevaluated context">,
+  InGroup<UnusedValue>;
+def warn_side_effects_typeid : Warning<
+  "expression with side effects will be evaluated despite being used as an "
+  "operand to 'typeid'">, InGroup<UnusedValue>;
 def warn_unused_result : Warning<
   "ignoring return value of function declared with warn_unused_result "
   "attribute">, InGroup<DiagGroup<"unused-result">>;

include/clang/Sema/Sema.h

@@ -1399,7 +1399,10 @@ class Sema {
                              const CXXScopeSpec &SS, QualType T);
 
   QualType BuildTypeofExprType(Expr *E, SourceLocation Loc);
-  QualType BuildDecltypeType(Expr *E, SourceLocation Loc);
+  /// If AsUnevaluated is false, E is treated as though it were an evaluated
+  /// context, such as when building a type for decltype(auto).
+  QualType BuildDecltypeType(Expr *E, SourceLocation Loc,
+                             bool AsUnevaluated = true);
   QualType BuildUnaryTransformType(QualType BaseType,
                                    UnaryTransformType::UTTKind UKind,
                                    SourceLocation Loc);

lib/AST/Expr.cpp

@@ -2866,9 +2866,16 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   return false;
 }
 
-bool Expr::HasSideEffects(const ASTContext &Ctx) const {
+bool Expr::HasSideEffects(const ASTContext &Ctx,
+                          bool IncludePossibleEffects) const {
+  // In circumstances where we care about definite side effects instead of
+  // potential side effects, we want to ignore expressions that are part of a
+  // macro expansion as a potential side effect.
+  if (!IncludePossibleEffects && getExprLoc().isMacroID())
+    return false;
+
   if (isInstantiationDependent())
-    return true;
+    return IncludePossibleEffects;
 
   switch (getStmtClass()) {
   case NoStmtClass:
@@ -2921,21 +2928,27 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
     return false;
 
   case CallExprClass:
+  case CXXOperatorCallExprClass:
+  case CXXMemberCallExprClass:
+  case CUDAKernelCallExprClass:
+  case BlockExprClass:
+  case CXXBindTemporaryExprClass:
+  case UserDefinedLiteralClass:
+    // We don't know a call definitely has side effects, but we can check the
+    // call's operands.
+    if (!IncludePossibleEffects)
+      break;
+    return true;
+
   case MSPropertyRefExprClass:
   case CompoundAssignOperatorClass:
   case VAArgExprClass:
   case AtomicExprClass:
   case StmtExprClass:
-  case CXXOperatorCallExprClass:
-  case CXXMemberCallExprClass:
-  case UserDefinedLiteralClass:
   case CXXThrowExprClass:
   case CXXNewExprClass:
   case CXXDeleteExprClass:
   case ExprWithCleanupsClass:
-  case CXXBindTemporaryExprClass:
-  case BlockExprClass:
-  case CUDAKernelCallExprClass:
     // These always have a side-effect.
     return true;
 
@@ -2971,24 +2984,26 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case InitListExprClass:
     // FIXME: The children for an InitListExpr doesn't include the array filler.
     if (const Expr *E = cast<InitListExpr>(this)->getArrayFiller())
-      if (E->HasSideEffects(Ctx))
+      if (E->HasSideEffects(Ctx, IncludePossibleEffects))
         return true;
     break;
 
   case GenericSelectionExprClass:
     return cast<GenericSelectionExpr>(this)->getResultExpr()->
-        HasSideEffects(Ctx);
+        HasSideEffects(Ctx, IncludePossibleEffects);
 
   case ChooseExprClass:
-    return cast<ChooseExpr>(this)->getChosenSubExpr()->HasSideEffects(Ctx);
+    return cast<ChooseExpr>(this)->getChosenSubExpr()->HasSideEffects(
+        Ctx, IncludePossibleEffects);
 
   case CXXDefaultArgExprClass:
-    return cast<CXXDefaultArgExpr>(this)->getExpr()->HasSideEffects(Ctx);
+    return cast<CXXDefaultArgExpr>(this)->getExpr()->HasSideEffects(
+        Ctx, IncludePossibleEffects);
 
   case CXXDefaultInitExprClass: {
     const FieldDecl *FD = cast<CXXDefaultInitExpr>(this)->getField();
     if (const Expr *E = FD->getInClassInitializer())
-      return E->HasSideEffects(Ctx);
+      return E->HasSideEffects(Ctx, IncludePossibleEffects);
     // If we've not yet parsed the initializer, assume it has side-effects.
     return true;
   }
@@ -3021,7 +3036,7 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case CXXConstructExprClass:
   case CXXTemporaryObjectExprClass: {
     const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);
-    if (!CE->getConstructor()->isTrivial())
+    if (!CE->getConstructor()->isTrivial() && IncludePossibleEffects)
       return true;
     // A trivial constructor does not add any side-effects of its own. Just look
     // at its arguments.
@@ -3049,7 +3064,7 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
       const Expr *Subexpr = *I;
       if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(Subexpr))
         Subexpr = OVE->getSourceExpr();
-      if (Subexpr->HasSideEffects(Ctx))
+      if (Subexpr->HasSideEffects(Ctx, IncludePossibleEffects))
         return true;
     }
     return false;
@@ -3058,22 +3073,24 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case ObjCBoxedExprClass:
   case ObjCArrayLiteralClass:
   case ObjCDictionaryLiteralClass:
-  case ObjCMessageExprClass:
   case ObjCSelectorExprClass:
   case ObjCProtocolExprClass:
-  case ObjCPropertyRefExprClass:
   case ObjCIsaExprClass:
   case ObjCIndirectCopyRestoreExprClass:
   case ObjCSubscriptRefExprClass:
   case ObjCBridgedCastExprClass:
-    // FIXME: Classify these cases better.
-    return true;
+  case ObjCMessageExprClass:
+  case ObjCPropertyRefExprClass:
+  // FIXME: Classify these cases better.
+    if (IncludePossibleEffects)
+      return true;
+    break;
   }
 
   // Recurse to children.
   for (const_child_range SubStmts = children(); SubStmts; ++SubStmts)
     if (const Stmt *S = *SubStmts)
-      if (cast<Expr>(S)->HasSideEffects(Ctx))
+      if (cast<Expr>(S)->HasSideEffects(Ctx, IncludePossibleEffects))
         return true;
 
   return false;

lib/Sema/SemaExpr.cpp

@@ -1290,6 +1290,13 @@ Sema::CreateGenericSelectionExpr(SourceLocation KeyLoc,
     ControllingExpr = result.get();
   }
 
+  // The controlling expression is an unevaluated operand, so side effects are
+  // likely unintended.
+  if (ActiveTemplateInstantiations.empty() &&
+      ControllingExpr->HasSideEffects(Context, false))
+    Diag(ControllingExpr->getExprLoc(),
+         diag::warn_side_effects_unevaluated_context);
+
   bool TypeErrorFound = false,
        IsResultDependent = ControllingExpr->isTypeDependent(),
        ContainsUnexpandedParameterPack
@@ -3525,6 +3532,12 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
     return true;
   }
 
+  // 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) &&
+      ActiveTemplateInstantiations.empty() && E->HasSideEffects(Context, false))
+    Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+
   if (CheckObjCTraitOperandConstraints(*this, ExprTy, E->getExprLoc(),
                                        E->getSourceRange(), ExprKind))
     return true;

lib/Sema/SemaExprCXX.cpp

@@ -396,6 +396,7 @@ ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType,
                                 SourceLocation TypeidLoc,
                                 Expr *E,
                                 SourceLocation RParenLoc) {
+  bool WasEvaluated = false;
   if (E && !E->isTypeDependent()) {
     if (E->getType()->isPlaceholderType()) {
       ExprResult result = CheckPlaceholderExpr(E);
@@ -425,6 +426,7 @@ ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType,
 
         // We require a vtable to query the type at run time.
         MarkVTableUsed(TypeidLoc, RecordD);
+        WasEvaluated = true;
       }
     }
 
@@ -444,6 +446,14 @@ ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType,
   if (E->getType()->isVariablyModifiedType())
     return ExprError(Diag(TypeidLoc, diag::err_variably_modified_typeid)
                      << E->getType());
+  else if (ActiveTemplateInstantiations.empty() &&
+           E->HasSideEffects(Context, WasEvaluated)) {
+    // The expression operand for typeid is in an unevaluated expression
+    // context, so side effects could result in unintended consequences.
+    Diag(E->getExprLoc(), WasEvaluated
+                              ? diag::warn_side_effects_typeid
+                              : diag::warn_side_effects_unevaluated_context);
+  }
 
   return new (Context) CXXTypeidExpr(TypeInfoType.withConst(), E,
                                      SourceRange(TypeidLoc, RParenLoc));
@@ -5622,7 +5632,8 @@ ExprResult Sema::ActOnPseudoDestructorExpr(Scope *S, Expr *Base,
   if (CheckArrow(*this, ObjectType, Base, OpKind, OpLoc))
     return ExprError();
 
-  QualType T = BuildDecltypeType(DS.getRepAsExpr(), DS.getTypeSpecTypeLoc());
+  QualType T = BuildDecltypeType(DS.getRepAsExpr(), DS.getTypeSpecTypeLoc(),
+                                 false);
 
   TypeLocBuilder TLB;
   DecltypeTypeLoc DecltypeTL = TLB.push<DecltypeTypeLoc>(T);
@@ -5690,6 +5701,13 @@ ExprResult Sema::BuildCXXMemberCallExpr(Expr *E, NamedDecl *FoundDecl,
 
 ExprResult Sema::BuildCXXNoexceptExpr(SourceLocation KeyLoc, Expr *Operand,
                                       SourceLocation RParen) {
+  if (ActiveTemplateInstantiations.empty() &&
+      Operand->HasSideEffects(Context, false)) {
+    // The expression operand for noexcept is in an unevaluated expression
+    // context, so side effects could result in unintended consequences.
+    Diag(Operand->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+  }
+
   CanThrowResult CanThrow = canThrow(Operand);
   return new (Context)
       CXXNoexceptExpr(Context.BoolTy, Operand, CanThrow, KeyLoc, RParen);

lib/Sema/SemaTemplateDeduction.cpp

@@ -3989,7 +3989,7 @@ Sema::DeduceAutoType(TypeLoc Type, Expr *&Init, QualType &Result) {
         return DAR_FailedAlreadyDiagnosed;
       }
 
-      QualType Deduced = BuildDecltypeType(Init, Init->getLocStart());
+      QualType Deduced = BuildDecltypeType(Init, Init->getLocStart(), false);
       // FIXME: Support a non-canonical deduced type for 'auto'.
       Deduced = Context.getCanonicalType(Deduced);
       Result = SubstituteAutoTransform(*this, Deduced).Apply(Type);

lib/Sema/SemaType.cpp

@@ -5518,11 +5518,19 @@ static QualType getDecltypeForExpr(Sema &S, Expr *E) {
   return T;
 }
 
-QualType Sema::BuildDecltypeType(Expr *E, SourceLocation Loc) {
+QualType Sema::BuildDecltypeType(Expr *E, SourceLocation Loc,
+                                 bool AsUnevaluated) {
   ExprResult ER = CheckPlaceholderExpr(E);
   if (ER.isInvalid()) return QualType();
   E = ER.get();
 
+  if (AsUnevaluated && ActiveTemplateInstantiations.empty() &&
+      E->HasSideEffects(Context, false)) {
+    // The expression operand for decltype is in an unevaluated expression
+    // context, so side effects could result in unintended consequences.
+    Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
+  }
+
   return Context.getDecltypeType(E, getDecltypeForExpr(*this, E));
 }
 

test/CXX/expr/expr.prim/expr.prim.lambda/p2-generic-lambda-1y.cpp

@@ -18,7 +18,7 @@ void unevaluated_operand(P &p, int i) { //expected-note{{declared here}}
   // FIXME: this should only emit one error.
   int i2 = sizeof([](auto a, auto b)->void{}(3, '4')); // expected-error{{lambda expression in an unevaluated operand}} \
                                                        // expected-error{{invalid application of 'sizeof'}}
-  const std::type_info &ti1 = typeid([](auto &a) -> P& { static P p; return p; }(i));
+  const std::type_info &ti1 = typeid([](auto &a) -> P& { static P p; return p; }(i)); // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}}
   const std::type_info &ti2 = typeid([](auto) -> int { return i; }(i));  // expected-error{{lambda expression in an unevaluated operand}}\
                                                                          // expected-error{{cannot be implicitly captured}}\
                                                                          // expected-note{{begins here}}

test/CXX/expr/expr.prim/expr.prim.lambda/p2.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wno-unused-value %s -verify
 
 // prvalue
 void prvalue() {

test/CXX/expr/expr.unary/expr.unary.noexcept/sema.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 -fms-extensions -Wno-delete-incomplete %s
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 -fms-extensions -Wno-delete-incomplete -Wno-unused-value %s
 // expected-no-diagnostics
 
 #define P(e) static_assert(noexcept(e), "expected nothrow")

test/Sema/bitfield.c

@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c11
+// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c11 -Wno-unused-value
+
 enum e0; // expected-note{{forward declaration of 'enum e0'}}
 
 struct a {

test/Sema/expr-comma-c99.c

@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c99 -Wno-sizeof-array-decay
-// expected-no-diagnostics
 // rdar://6095180
 
 struct s { char c[17]; };
@@ -14,5 +13,5 @@ int B[sizeof((a.c)) == 17 ? 1 : -1];
 // comma does array/function promotion in c99.
 int X[sizeof(0, (foo().c)) == sizeof(char*) ? 1 : -1];
 int Y[sizeof(0, (a,b).c) == sizeof(char*) ? 1 : -1];
-int Z[sizeof(0, (a=b).c) == sizeof(char*) ? 1 : -1];
+int Z[sizeof(0, (a=b).c) == sizeof(char*) ? 1 : -1]; // expected-warning {{expression with side effects has no effect in an unevaluated context}}
 

test/Sema/expr-comma.c

@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c89 -Wno-sizeof-array-decay
-// expected-no-diagnostics
 // rdar://6095180
 
 struct s { char c[17]; };
@@ -15,4 +14,4 @@ int B[sizeof((a.c)) == 17 ? 1 : -1];
 int W[sizeof(0, a.c) == sizeof(char*) ? 1 : -1];
 int X[sizeof(0, (foo().c)) == 17 ? 1 : -1];
 int Y[sizeof(0, (a,b).c) == 17 ? 1 : -1];
-int Z[sizeof(0, (a=b).c) == 17 ? 1 : -1];
+int Z[sizeof(0, (a=b).c) == 17 ? 1 : -1]; // expected-warning {{expression with side effects has no effect in an unevaluated context}}

test/Sema/warn-unused-value.c

@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wunused-value -Wunused-label %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wunused %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wunused-value -Wunused-label %s
+// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wunused %s
+// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wall %s
 
 int i = 0;
 int j = 0;
@@ -88,3 +88,22 @@ void f1(struct s0 *a) {
   // rdar://8139785
   f0((int)(a->f0 + 1, 10)); // expected-warning {{expression result unused}}
 }
+
+void blah(int a);
+#define GenTest(x) _Generic(x, default : blah)(x)
+
+void unevaluated_operands(void) {
+  int val = 0;
+
+  (void)sizeof(++val); // expected-warning {{expression with side effects has no effect in an unevaluated context}}
+  (void)_Generic(val++, default : 0); // expected-warning {{expression with side effects has no effect in an unevaluated context}}
+  (void)_Alignof(val++);  // expected-warning {{expression with side effects has no effect in an unevaluated context}} expected-warning {{'_Alignof' applied to an expression is a GNU extension}}
+
+  // VLAs can have side effects so long as it's part of the type and not
+  // an expression.
+  (void)sizeof(int[++val]); // Ok
+  (void)_Alignof(int[++val]); // Ok
+
+  // Side effects as part of macro expansion are ok.
+  GenTest(val++);
+}

test/SemaCXX/constant-expression-cxx11.cpp

@@ -1613,7 +1613,8 @@ namespace TypeId {
   A &g();
   constexpr auto &x = typeid(f());
   constexpr auto &y = typeid(g()); // expected-error{{constant expression}} \
-  // expected-note{{typeid applied to expression of polymorphic type 'TypeId::A' is not allowed in a constant expression}}
+  // expected-note{{typeid applied to expression of polymorphic type 'TypeId::A' is not allowed in a constant expression}} \
+  // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}}
 }
 
 namespace PR14203 {

test/SemaCXX/runtimediag-ppe.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value %s
 
 // Make sure diagnostics that we don't print based on runtime control
 // flow are delayed correctly in cases where we can't immediately tell whether

test/SemaCXX/undefined-internal.cpp

@@ -163,7 +163,7 @@ namespace cxx11_odr_rules {
 
     // Check that the checks work with unevaluated contexts
     (void)sizeof(p(A::used1));
-    (void)typeid(p(A::used1)); // xpected-note {{used here}}
+    (void)typeid(p(A::used1)); // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}} xpected-note {{used here}}
 
     // Misc other testing
     a(A::unused, 1 ? A::used2 : A::used2); // xpected-note {{used here}}