Properly diagnose [[nodiscard]] on the body of a range-based for loop

[aaron.ballman]

Summary

If the range-based for loop function body is not a compound statement, we would fail to diagnose discarded results from the statement. This only impacted range-based for loops because other kinds of for loops already manually check the body for unused expression results.

This addresses PR39837.

[rsmith]

lib/Sema/SemaStmt.cpp

2846

While this looks correct per the current approach to this function, we really shouldn't be duplicating calls to this everywhere. Can we move all the calls to a single call in ActOnFinishFullStmt?

[rsmith]

lib/Sema/SemaStmt.cpp

2846

Looks like that's not actually called from almost anywhere, but checking from ActOnFinishFullExpr in the case where DiscardedValue is true seems appropriate.

[aaron.ballman]

lib/Sema/SemaStmt.cpp

2846

That seems sensible, but how will that work with GNU statement expressions? If we check for unused expression results, then we will trigger on code like this:

int a = ({blah(); yada(); 0;});
// or
int b = ({blah(); yada(); some_no_discard_call();});

I don't know of a way to handle that case -- we call ActOnFinishFullExpr() while parsing the compound statement for the StmtExpr, so there's no way to know whether there's another statement coming (without lookahead) to determine whether to suppress the diagnostic for the last expression statement. Do you have ideas on how to handle that?

[rsmith]

lib/Sema/SemaStmt.cpp

2846

If we're calling ActOnFinishFullExpr there with DiscardedValue == true, that would be a bug that we should be fixing regardless. I don't think the lookahead is so bad itself -- it should just be a one-token lookahead for a } after the ; -- but it might be awkward to wire it into our compound-statement / expression-statement parsing. Still, it seems necessary for correctness.

[aaron.ballman]

Updated based on review feedback.

The lookahead wasn't too annoying to thread through, but I did run into surprises in TreeTransform where I can't use the lookahead to determine whether the expression statement should be warned on or not. The solution I have is serviceable, but perhaps there's a better approach to be taken.

The changes introduce warnings where they were previously missed, such as in init-statements. The only unfortunate behavioral side effect comes from range-based for loops that have an expression as the first statement -- in addition to the error, we now trigger an unused expression result warning, but restructuring the code to avoid it would be challenging for such an edge case (and the warning behavior seems reasonable).

[rsmith]

include/clang/Sema/Sema.h

5337

Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a discarded-value expression?

lib/Parse/ParseStmt.cpp

23

The parser shouldn't be looking inside Sema's data structures. Can you add a method to the Sema interface for the query you make below instead?

lib/Sema/SemaExprCXX.cpp

7785

It doesn't make sense for a WarnOn flag to control how we build the AST like this.

lib/Sema/TreeTransform.h

3297

What happens if the last statement in an expression statement contains a lambda? Will we consider all expression statements in it as not being discarded-value expressions? Alternate suggestion below.

6529

Instead of this counter mechanism (which doesn't seem reliable), can you just pass a flag to TransformStmt?

[aaron.ballman]

include/clang/Sema/Sema.h

5337

It probably can; but then it feels like the logic is backwards from the suggested changes as I understood them. If it's a discarded value expression, then the value being unused should *not* be diagnosed because the expression only exists for its side effects (not its value computations), correct?

lib/Parse/ParseStmt.cpp

23

Can do.

lib/Sema/TreeTransform.h

6529

I'll give it a shot, it seems plausible.

[• Quuxplusone]

include/clang/Sema/Sema.h

5337

Peanut gallery says: There are at least three things that need to be computed somewhere: (1) Is this expression's value discarded? (2) Is this expression the result of a [[nodiscard]] function? (3) Is the diagnostic enabled? It is unclear to me who's responsible for computing which of these things. I.e., it is unclear to me whether WarnOnDiscardedValue=true means "Hey ActOnFinishFullExpr, please give a warning because this value is being discarded" (conjunction of 1,2, and maybe 3) or "Hey ActOnFinishFullExpr, please give a warning if this value is being discarded" (conjunction of 2 and maybe 3).

I also think it is needlessly confusing that ActOnFinishFullExpr gives WarnOnDiscardedValue a defaulted value of false but ActOnExprStmt gives WarnOnDiscardedValue a defaulted value of true. Defaulted values (especially of boolean type) are horrible, but context-dependent defaulted values are even worse.

[rsmith]

include/clang/Sema/Sema.h

5337

I don't think it makes sense for ActOnFinishFullExpr to have a default argument for DiscardedValue, because there's really no reason to assume one way or the other -- the values of some full-expressions are used, and the values of others are not. A default of false certainly seems wrong.

For ActOnExprStmt, the default argument makes sense to me: the expression in an expression-statement is by definition a discarded-value expression (http://eel.is/c++draft/stmt.stmt#stmt.expr-1.sentence-2) -- it's only the weird special case for a final expression-statement in an statement-expression that bucks the trend here.

If it's a discarded value expression, then the value being unused should *not* be diagnosed because the expression only exists for its side effects (not its value computations), correct?

No. If it's a discarded-value expression, that means the value of the full-expression is not being used, so it should be diagnosed. If it's not a discarded-value expression, then the value of the full-expression is used for something (eg, it's a condition or an array bound or a template argument) and so we should not warn. Indeed, the wording for [[nodiscard]] suggests to warn (only) on potentially-evaluated discarded-value expressions.

Discarded-value expressions are things like expression-statements, the left-hand-side of a comma operator, and the operands of casts to void. (Note in the cast-to-void case is explicitly called out by the [[nodiscard]] wording as a discarded-value expression that should not warn: http://eel.is/c++draft/dcl.attr.nodiscard#2.sentence-2)

[aaron.ballman]

Updated based on review feedback. The vast majority of the changes are from the removal of a default argument to ActOnFullExpr().

[aaron.ballman]

include/clang/Sema/Sema.h

5337

Thank you for the explanation @rsmith, my mental model was exactly backwards. :-D I was thrown off by http://eel.is/c++draft/expr.prop#expr.context-2.sentence-1 because I read it as saying the discarded nature of the results are *desired* rather than problematic. e.g., some statements only appear for their side effects, do not warn about such statements because the context makes it obvious that this is expected. I should have looked it it in the context of the nodiscard attribute wording rather than in isolation.

[rsmith]

Thanks, this looks great. A couple of the changes to the tests look like the diagnostic output is slightly worse in some error recovery conditions, but generally this is a nice improvement.

test/Parser/switch-recovery.cpp

108

Hmm, why do we only get one warning here? I'd expect one warning for the 8; and one for the x; (after applying the fixes from the errors).

test/SemaCXX/for-range-examples.cpp

181

The new warnings here aren't ideal; do you know why they show up?

[aaron.ballman]

test/Parser/switch-recovery.cpp

108

We get the one for the 8; but the other one is a TypoExpr and it claims it's type dependent, and we don't warn on type dependent so we bail out pretty early in Expr::isUnusedResultAWarning().

test/SemaCXX/for-range-examples.cpp

181

Because the first part in a for loop can be an expression, but only if it's not a range-based for loop. However, I was able to do some lookahead to retain the old behavior here.

[aaron.ballman]

Committed with minor modifications for the range-based for loop changes in r350404.

include/clang/Parse/Parser.h

   /// declarations/definitions when indexing.
   bool SkipFunctionBodies;
 
   /// The location of the expression statement that is being parsed right now.
   /// Used to determine if an expression that is being parsed is a statement or
   /// just a regular sub-expression.
   SourceLocation ExprStatementTokLoc;
 
+  bool warnOnDiscardedValueExpr();
+
 public:
   Parser(Preprocessor &PP, Sema &Actions, bool SkipFunctionBodies);
   ~Parser() override;
 
   const LangOptions &getLangOpts() const { return PP.getLangOpts(); }
   const TargetInfo &getTargetInfo() const { return PP.getTargetInfo(); }
   Preprocessor &getPreprocessor() const { return PP; }
   Sema &getActions() const { return Actions; }

include/clang/Sema/Sema.h

   }
   FullExprArg MakeFullDiscardedValueExpr(Expr *Arg) {
     ExprResult FE =
       ActOnFinishFullExpr(Arg, Arg ? Arg->getExprLoc() : SourceLocation(),
                           /*DiscardedValue*/ true);
     return FullExprArg(FE.get());
   }
 
-  StmtResult ActOnExprStmt(ExprResult Arg);
+  StmtResult ActOnExprStmt(ExprResult Arg, bool WarnOnDiscardedValue = true);
   StmtResult ActOnExprStmtError();
 
   StmtResult ActOnNullStmt(SourceLocation SemiLoc,
                            bool HasLeadingEmptyMacro = false);
 
   void ActOnStartOfCompoundStmt(bool IsStmtExpr);
   void ActOnFinishOfCompoundStmt();
   StmtResult ActOnCompoundStmt(SourceLocation L, SourceLocation R,

   CreateMaterializeTemporaryExpr(QualType T, Expr *Temporary,
                                  bool BoundToLvalueReference);
 
   ExprResult ActOnFinishFullExpr(Expr *Expr) {
     return ActOnFinishFullExpr(Expr, Expr ? Expr->getExprLoc()
                                           : SourceLocation());
   }
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
-                                 bool DiscardedValue = false,
+                                 bool WarnOnDiscardedValue = false,
                                  bool IsConstexpr = false);
   StmtResult ActOnFinishFullStmt(Stmt *Stmt);
 
   // Marks SS invalid if it represents an incomplete type.
   bool RequireCompleteDeclContext(CXXScopeSpec &SS, DeclContext *DC);
 
   DeclContext *computeDeclContext(QualType T);
   DeclContext *computeDeclContext(const CXXScopeSpec &SS,

lib/Parse/ParseObjc.cpp

     // doing this opens us up to the possibility of infinite loops if
     // ParseExpression does not consume any tokens.
     SkipUntil(tok::semi);
     return StmtError();
   }
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Res);
+  return Actions.ActOnExprStmt(Res, warnOnDiscardedValueExpr());
 }
 
 ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) {
   switch (Tok.getKind()) {
   case tok::code_completion:
     Actions.CodeCompleteObjCAtExpression(getCurScope());
     cutOffParsing();
     return ExprError();

lib/Parse/ParseOpenMP.cpp

   unsigned I = 0, E = ReductionTypes.size();
   for (Decl *D : DRD.get()) {
     TentativeParsingAction TPA(*this);
     ParseScope OMPDRScope(this, Scope::FnScope | Scope::DeclScope |
                                     Scope::CompoundStmtScope |
                                     Scope::OpenMPDirectiveScope);
     // Parse <combiner> expression.
     Actions.ActOnOpenMPDeclareReductionCombinerStart(getCurScope(), D);
-    ExprResult CombinerResult =
-        Actions.ActOnFinishFullExpr(ParseAssignmentExpression().get(),
-                                    D->getLocation(), /*DiscardedValue=*/true);
+    ExprResult CombinerResult = Actions.ActOnFinishFullExpr(
+        ParseAssignmentExpression().get(), D->getLocation());
     Actions.ActOnOpenMPDeclareReductionCombinerEnd(D, CombinerResult.get());
 
     if (CombinerResult.isInvalid() && Tok.isNot(tok::r_paren) &&
         Tok.isNot(tok::annot_pragma_openmp_end)) {
       TPA.Commit();
       IsCorrect = false;
       break;
     }

         VarDecl *OmpPrivParm =
             Actions.ActOnOpenMPDeclareReductionInitializerStart(getCurScope(),
                                                                 D);
         // Check if initializer is omp_priv <init_expr> or something else.
         if (Tok.is(tok::identifier) &&
             Tok.getIdentifierInfo()->isStr("omp_priv")) {
           if (Actions.getLangOpts().CPlusPlus) {
             InitializerResult = Actions.ActOnFinishFullExpr(
-                ParseAssignmentExpression().get(), D->getLocation(),
-                /*DiscardedValue=*/true);
+                ParseAssignmentExpression().get(), D->getLocation());
           } else {
             ConsumeToken();
             ParseOpenMPReductionInitializerForDecl(OmpPrivParm);
           }
         } else {
           InitializerResult = Actions.ActOnFinishFullExpr(
-              ParseAssignmentExpression().get(), D->getLocation(),
-              /*DiscardedValue=*/true);
+              ParseAssignmentExpression().get(), D->getLocation());
         }
         Actions.ActOnOpenMPDeclareReductionInitializerEnd(
             D, InitializerResult.get(), OmpPrivParm);
         if (InitializerResult.isInvalid() && Tok.isNot(tok::r_paren) &&
             Tok.isNot(tok::annot_pragma_openmp_end)) {
           TPA.Commit();
           IsCorrect = false;
           break;

lib/Parse/ParseStmt.cpp

 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/Attributes.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Parse/LoopHint.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/TypoCorrection.h"
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 // C99 6.8: Statements and Blocks.
 //===----------------------------------------------------------------------===//
 
 /// Parse a standalone statement (for instance, as the body of an 'if',

       << FixItHint::CreateInsertion(OldToken.getLocation(), "case ");
 
     // Recover parsing as a case statement.
     return ParseCaseStatement(/*MissingCase=*/true, Expr);
   }
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Expr);
+  return Actions.ActOnExprStmt(Expr, warnOnDiscardedValueExpr());
 }
 
 /// ParseSEHTryBlockCommon
 ///
 /// seh-try-block:
 ///   '__try' compound-statement seh-handler
 ///
 /// seh-handler:

   if (EndLoc.isInvalid())
     return false;
 
   Diag(StartLoc, diag::warn_null_statement)
       << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
   return true;
 }
 
+bool Parser::warnOnDiscardedValueExpr() {
+  if (Actions.getCurCompoundScope().IsStmtExpr) {
+    // Look to see if the next two tokens close the statement expression;
+    // if so, this expression statement is the last statement in a
+    // statment expression.
+    return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren);
+  }
+  return true;
+}
+
 /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
 /// ActOnCompoundStmt action.  This expects the '{' to be the current token, and
 /// consume the '}' at the end of the block.  It does not manipulate the scope
 /// stack.
 StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
   PrettyStackTraceLoc CrashInfo(PP.getSourceManager(),
                                 Tok.getLocation(),
                                 "in compound statement ('{}')");

           SkipUntil(tok::semi);
           continue;
         }
 
         // FIXME: Use attributes?
         // Eat the semicolon at the end of stmt and convert the expr into a
         // statement.
         ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-        R = Actions.ActOnExprStmt(Res);
+        R = Actions.ActOnExprStmt(Res, warnOnDiscardedValueExpr());
       }
     }
 
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
   SourceLocation CloseLoc = Tok.getLocation();

lib/Sema/SemaExprCXX.cpp

       return Result;
     }
     assert(TyposResolved == 0 && "Corrected typo but got same Expr back?");
   }
   return E;
 }
 
 ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,
-                                     bool DiscardedValue,
+                                     bool WarnOnDiscardedValue,
                                      bool IsConstexpr) {
   ExprResult FullExpr = FE;
 
   if (!FullExpr.get())
     return ExprError();
 
   if (DiagnoseUnexpandedParameterPack(FullExpr.get()))
     return ExprError();
 
-  if (DiscardedValue) {
+  if (WarnOnDiscardedValue) {
     // Top-level expressions default to 'id' when we're in a debugger.
     if (getLangOpts().DebuggerCastResultToId &&
         FullExpr.get()->getType() == Context.UnknownAnyTy) {
       FullExpr = forceUnknownAnyToType(FullExpr.get(), Context.getObjCIdType());
       if (FullExpr.isInvalid())
         return ExprError();
     }
 
     FullExpr = CheckPlaceholderExpr(FullExpr.get());
     if (FullExpr.isInvalid())
       return ExprError();
 
     FullExpr = IgnoredValueConversions(FullExpr.get());
     if (FullExpr.isInvalid())
       return ExprError();
+
+    DiagnoseUnusedExprResult(FullExpr.get());
   }
 
   FullExpr = CorrectDelayedTyposInExpr(FullExpr.get());
   if (FullExpr.isInvalid())
     return ExprError();
 
   CheckCompletedExpr(FullExpr.get(), CC, IsConstexpr);
 

lib/Sema/SemaOpenMP.cpp

                      D->hasAttrs() ? &D->getAttrs() : nullptr);
     DeclRefExpr *PseudoDstExpr = buildDeclRefExpr(*this, DstVD, Type, ELoc);
     // For arrays generate assignment operation for single element and replace
     // it by the original array element in CodeGen.
     ExprResult AssignmentOp = BuildBinOp(/*S=*/nullptr, ELoc, BO_Assign,
                                          PseudoDstExpr, PseudoSrcExpr);
     if (AssignmentOp.isInvalid())
       continue;
-    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc,
-                                       /*DiscardedValue=*/true);
+    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc);
     if (AssignmentOp.isInvalid())
       continue;
 
     DeclRefExpr *Ref = nullptr;
     if (!VD && !CurContext->isDependentContext()) {
       if (TopDVar.CKind == OMPC_firstprivate) {
         Ref = TopDVar.PrivateCopy;
       } else {

     // Build update: Var = InitExpr + IV * Step
     ExprResult Update;
     if (!Info.first)
       Update =
           buildCounterUpdate(SemaRef, S, RefExpr->getExprLoc(), *CurPrivate,
                              InitExpr, IV, Step, /* Subtract */ false);
     else
       Update = *CurPrivate;
-    Update = SemaRef.ActOnFinishFullExpr(Update.get(), DE->getBeginLoc(),
-                                         /*DiscardedValue=*/true);
+    Update = SemaRef.ActOnFinishFullExpr(Update.get(), DE->getBeginLoc());
 
     // Build final: Var = InitExpr + NumIterations * Step
     ExprResult Final;
     if (!Info.first)
       Final =
           buildCounterUpdate(SemaRef, S, RefExpr->getExprLoc(), CapturedRef,
                              InitExpr, NumIterations, Step, /*Subtract=*/false);
     else
       Final = *CurPrivate;
-    Final = SemaRef.ActOnFinishFullExpr(Final.get(), DE->getBeginLoc(),
-                                        /*DiscardedValue=*/true);
+    Final = SemaRef.ActOnFinishFullExpr(Final.get(), DE->getBeginLoc());
 
     if (!Update.isUsable() || !Final.isUsable()) {
       Updates.push_back(nullptr);
       Finals.push_back(nullptr);
       HasErrors = true;
     } else {
       Updates.push_back(Update.get());
       Finals.push_back(Final.get());

         buildDeclRefExpr(*this, DstVD, ElemType, DE->getExprLoc());
     // For arrays generate assignment operation for single element and replace
     // it by the original array element in CodeGen.
     ExprResult AssignmentOp =
         BuildBinOp(/*S=*/nullptr, DE->getExprLoc(), BO_Assign, PseudoDstExpr,
                    PseudoSrcExpr);
     if (AssignmentOp.isInvalid())
       continue;
-    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), DE->getExprLoc(),
-                                       /*DiscardedValue=*/true);
+    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), DE->getExprLoc());
     if (AssignmentOp.isInvalid())
       continue;
 
     DSAStack->addDSA(VD, DE, OMPC_copyin);
     Vars.push_back(DE);
     SrcExprs.push_back(PseudoSrcExpr);
     DstExprs.push_back(PseudoDstExpr);
     AssignmentOps.push_back(AssignmentOp.get());

     VarDecl *DstVD =
         buildVarDecl(*this, RefExpr->getBeginLoc(), Type, ".copyprivate.dst",
                      D->hasAttrs() ? &D->getAttrs() : nullptr);
     DeclRefExpr *PseudoDstExpr = buildDeclRefExpr(*this, DstVD, Type, ELoc);
     ExprResult AssignmentOp = BuildBinOp(
         DSAStack->getCurScope(), ELoc, BO_Assign, PseudoDstExpr, PseudoSrcExpr);
     if (AssignmentOp.isInvalid())
       continue;
-    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc,
-                                       /*DiscardedValue=*/true);
+    AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc);
     if (AssignmentOp.isInvalid())
       continue;
 
     // No need to mark vars as copyprivate, they are already threadprivate or
     // implicitly private.
     assert(VD || isOpenMPCapturedDecl(D));
     Vars.push_back(
         VD ? RefExpr->IgnoreParens()

lib/Sema/SemaStmt.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 
 using namespace clang;
 using namespace sema;
 
-StmtResult Sema::ActOnExprStmt(ExprResult FE) {
+StmtResult Sema::ActOnExprStmt(ExprResult FE,
+                               bool WarnOnDiscardedValue) {
   if (FE.isInvalid())
     return StmtError();
 
   FE = ActOnFinishFullExpr(FE.get(), FE.get()->getExprLoc(),
-                           /*DiscardedValue*/ true);
+                           WarnOnDiscardedValue);
   if (FE.isInvalid())
     return StmtError();
 
   // C99 6.8.3p2: The expression in an expression statement is evaluated as a
   // void expression for its side effects.  Conversion to void allows any
   // operand, even incomplete types.
 
   // Same thing in for stmt first clause (when expr) and third clause.

     for (; i != NumElts && !isa<DeclStmt>(Elts[i]); ++i)
       /*empty*/;
 
     if (i != NumElts) {
       Decl *D = *cast<DeclStmt>(Elts[i])->decl_begin();
       Diag(D->getLocation(), diag::ext_mixed_decls_code);
     }
   }
-  // Warn about unused expressions in statements.
-  for (unsigned i = 0; i != NumElts; ++i) {
-    // Ignore statements that are last in a statement expression.
-    if (isStmtExpr && i == NumElts - 1)
-      continue;
-
-    DiagnoseUnusedExprResult(Elts[i]);
-  }
 
   // Check for suspicious empty body (null statement) in `for' and `while'
   // statements.  Don't do anything for template instantiations, this just adds
   // noise.
   if (NumElts != 0 && !CurrentInstantiationScope &&
       getCurCompoundScope().HasEmptyLoopBodies) {
     for (unsigned i = 0; i != NumElts - 1; ++i)
       DiagnoseEmptyLoopBody(Elts[i], Elts[i + 1]);

   auto *CS = CaseStmt::Create(Context, LHSVal.get(), RHSVal.get(),
                               CaseLoc, DotDotDotLoc, ColonLoc);
   getCurFunction()->SwitchStack.back().getPointer()->addSwitchCase(CS);
   return CS;
 }
 
 /// ActOnCaseStmtBody - This installs a statement as the body of a case.
 void Sema::ActOnCaseStmtBody(Stmt *S, Stmt *SubStmt) {
-  DiagnoseUnusedExprResult(SubStmt);
   cast<CaseStmt>(S)->setSubStmt(SubStmt);
 }
 
 StmtResult
 Sema::ActOnDefaultStmt(SourceLocation DefaultLoc, SourceLocation ColonLoc,
                        Stmt *SubStmt, Scope *CurScope) {
-  DiagnoseUnusedExprResult(SubStmt);
-
   if (getCurFunction()->SwitchStack.empty()) {
     Diag(DefaultLoc, diag::err_default_not_in_switch);
     return SubStmt;
   }
 
   DefaultStmt *DS = new (Context) DefaultStmt(DefaultLoc, ColonLoc, SubStmt);
   getCurFunction()->SwitchStack.back().getPointer()->addSwitchCase(DS);
   return DS;

                              Stmt *thenStmt, SourceLocation ElseLoc,
                              Stmt *elseStmt) {
   if (Cond.isInvalid())
     return StmtError();
 
   if (IsConstexpr || isa<ObjCAvailabilityCheckExpr>(Cond.get().second))
     setFunctionHasBranchProtectedScope();
 
-  DiagnoseUnusedExprResult(thenStmt);
-  DiagnoseUnusedExprResult(elseStmt);
-
   return IfStmt::Create(Context, IfLoc, IsConstexpr, InitStmt, Cond.get().first,
                         Cond.get().second, thenStmt, ElseLoc, elseStmt);
 }
 
 namespace {
   struct CaseCompareFunctor {
     bool operator()(const std::pair<llvm::APSInt, CaseStmt*> &LHS,
                     const llvm::APSInt &RHS) {

 
   auto CondVal = Cond.get();
   CheckBreakContinueBinding(CondVal.second);
 
   if (CondVal.second &&
       !Diags.isIgnored(diag::warn_comma_operator, CondVal.second->getExprLoc()))
     CommaVisitor(*this).Visit(CondVal.second);
 
-  DiagnoseUnusedExprResult(Body);
-
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();
 
   return WhileStmt::Create(Context, CondVal.first, CondVal.second, Body,
                            WhileLoc);
 }
 
 StmtResult

     return StmtError();
   Cond = CondResult.get();
 
   // Only call the CommaVisitor for C89 due to differences in scope flags.
   if (Cond && !getLangOpts().C99 && !getLangOpts().CPlusPlus &&
       !Diags.isIgnored(diag::warn_comma_operator, Cond->getExprLoc()))
     CommaVisitor(*this).Visit(Cond);
 
-  DiagnoseUnusedExprResult(Body);
-
   return new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen);
 }
 
 namespace {
   // Use SetVector since the diagnostic cares about the ordering of the Decl's.
   using DeclSetVector =
       llvm::SetVector<VarDecl *, llvm::SmallVector<VarDecl *, 8>,
                       llvm::SmallPtrSet<VarDecl *, 8>>;

   CheckForRedundantIteration(*this, third.get(), Body);
 
   if (Second.get().second &&
       !Diags.isIgnored(diag::warn_comma_operator,
                        Second.get().second->getExprLoc()))
     CommaVisitor(*this).Visit(Second.get().second);
 
   Expr *Third  = third.release().getAs<Expr>();
-
-  DiagnoseUnusedExprResult(First);
-  DiagnoseUnusedExprResult(Third);
-  DiagnoseUnusedExprResult(Body);
-
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();
 
   return new (Context)
       ForStmt(Context, First, Second.get().second, Second.get().first, Third,
               Body, ForLoc, LParenLoc, RParenLoc);
 }
 

     Init = Res.get();
   }
 
   // Deduce the type for the iterator variable now rather than leaving it to
   // AddInitializerToDecl, so we can produce a more suitable diagnostic.
   QualType InitType;
   if ((!isa<InitListExpr>(Init) && Init->getType()->isVoidType()) ||
       SemaRef.DeduceAutoType(Decl->getTypeSourceInfo(), Init, InitType) ==
           Sema::DAR_Failed)

lib/Sema/TreeTransform.h

       Old = Self.ForgetPartiallySubstitutedPack();
     }
 
     ~ForgetPartiallySubstitutedPackRAII() {
       Self.RememberPartiallySubstitutedPack(Old);
     }
   };
 
+  unsigned SuppressWarnOnUnusedExpr = 0;
+
 protected:
   Sema &SemaRef;
 
   /// The set of local declarations that have been transformed, for
   /// cases where we are forced to build new declarations within the transformer
   /// rather than in the subclass (e.g., lambda closure types).
   llvm::DenseMap<Decl *, Decl *> TransformedLocalDecls;
 

 #define ABSTRACT_STMT(Stmt)
 #define EXPR(Node, Parent) case Stmt::Node##Class:
 #include "clang/AST/StmtNodes.inc"
     {
       ExprResult E = getDerived().TransformExpr(cast<Expr>(S));
       if (E.isInvalid())
         return StmtError();
 
-      return getSema().ActOnExprStmt(E);
+      return getSema().ActOnExprStmt(E, SuppressWarnOnUnusedExpr == 0);
     }
   }
 
   return S;
 }
 
 template<typename Derived>
 OMPClause *TreeTransform<Derived>::TransformOMPClause(OMPClause *S) {

 TreeTransform<Derived>::TransformCompoundStmt(CompoundStmt *S,
                                               bool IsStmtExpr) {
   Sema::CompoundScopeRAII CompoundScope(getSema());
 
   bool SubStmtInvalid = false;
   bool SubStmtChanged = false;
   SmallVector<Stmt*, 8> Statements;
   for (auto *B : S->body()) {
+    bool TransformingLastStmt = B == S->body_back();
+    if (IsStmtExpr && TransformingLastStmt)
+      ++SuppressWarnOnUnusedExpr;
     StmtResult Result = getDerived().TransformStmt(B);
+    if (IsStmtExpr && TransformingLastStmt)
+      --SuppressWarnOnUnusedExpr;
+    
     if (Result.isInvalid()) {
       // Immediately fail if this was a DeclStmt, since it's very
       // likely that this will cause problems for future statements.
       if (isa<DeclStmt>(B))
         return StmtError();
 
       // Otherwise, just keep processing substatements and fail later.
       SubStmtInvalid = true;

                         S->getAtLoc(), Body.get());
 }
 
 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformObjCForCollectionStmt(
                                                   ObjCForCollectionStmt *S) {
   // Transform the element statement.
+  ++SuppressWarnOnUnusedExpr;
   StmtResult Element = getDerived().TransformStmt(S->getElement());
+  --SuppressWarnOnUnusedExpr;
   if (Element.isInvalid())
     return StmtError();
 
   // Transform the collection expression.
+  ++SuppressWarnOnUnusedExpr;
   ExprResult Collection = getDerived().TransformExpr(S->getCollection());
+  --SuppressWarnOnUnusedExpr;
   if (Collection.isInvalid())
     return StmtError();
 
   // Transform the body.
   StmtResult Body = getDerived().TransformStmt(S->getBody());
   if (Body.isInvalid())
     return StmtError();
 

test/CXX/stmt.stmt/stmt.select/p3.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fsyntax-only -std=c++1z -Wc++14-compat -verify %s -DCPP17
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -std=c++1z -Wc++14-compat -verify %s -DCPP17
 
 int f();
 
 void g() {
   if (int x = f()) { // expected-note 2{{previous definition}}
     int x; // expected-error{{redefinition of 'x'}}
   } else {
     int x; // expected-error{{redefinition of 'x'}}
void whileInitStatement() {
   // expected-warning@-4 {{while loop has empty body}}
   // expected-note@-5 {{put the semicolon on a separate line to silence this warning}}
 }
 
 // TODO: This is needed because clang can't seem to diagnose invalid syntax after the
 // last loop above. It would be nice to remove this.
 void whileInitStatement2() {
   while (; false) {} // expected-error {{expected expression}}
-  // expected-warning@-1 {{expression result unused}}
-  // expected-error@-2 {{expected ';' after expression}}
-  // expected-error@-3 {{expected expression}}
+  // expected-error@-1 {{expected ';' after expression}}
+  // expected-error@-2 {{expected expression}}
 }

test/CodeCompletion/pragma-macro-token-caching.c

 
 #define Outer(action) action
 
 void completeParam(int param) {
     ;
     Outer(__extension__({ _Pragma("clang diagnostic push") }));
     param;
 }
 
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:7:1 %s | FileCheck %s
 // CHECK: param : [#int#]param
 
 void completeParamPragmaError(int param) {
     Outer(__extension__({ _Pragma(2) })); // expected-error {{_Pragma takes a parenthesized string literal}}
-    param;
+    param; // expected-warning {{expression result unused}}
 }
 
 // RUN: %clang_cc1 -fsyntax-only -verify -code-completion-at=%s:16:1 %s | FileCheck %s

test/Parser/cxx1z-init-statement.cpp

 // RUN: %clang_cc1 -std=c++1z -verify %s -Wno-vexing-parse
 
 int g, h;
 typedef int T;
 int f() {
   // init-statement declarations
   if (T n = 0; n != 0) {}
   if (T f(); f()) {}
   if (T(f()); f()) {}
   if (T(f()), g, h; f()) {}
   if (T f(); f()) {}
   if (T f(), g, h; f()) {}
   if (T(n) = 0; n) {}
 
   // init-statement expressions
-  if (T{f()}; f()) {}
-  if (T{f()}, g, h; f()) {} // expected-warning 2{{unused}}
-  if (T(f()), g, h + 1; f()) {} // expected-warning 2{{unused}}
+  if (T{f()}; f()) {} // expected-warning {{expression result unused}}
+  if (T{f()}, g, h; f()) {} // expected-warning 2{{unused}} expected-warning {{expression result unused}}
+  if (T(f()), g, h + 1; f()) {} // expected-warning 2{{unused}} expected-warning {{expression result unused}}
 
   // condition declarations
   if (T(n){g}) {}
   if (T f()) {} // expected-error {{function type}}
   if (T f(), g, h) {} // expected-error {{function type}}
   if (T(n) = 0) {}
 
   // condition expressions
   if (T(f())) {}
   if (T{f()}) {}
   if (T(f()), g, h) {} // expected-warning 2{{unused}}
   if (T{f()}, g, h) {} // expected-warning 2{{unused}}
 
   // none of the above, disambiguated as expression (can't be a declaration)
   if (T(n)(g)) {} // expected-error {{undeclared identifier 'n'}}
   if (T(n)(int())) {} // expected-error {{undeclared identifier 'n'}}
 
   // Likewise for 'switch'
   switch (int n; n) {}
-  switch (g; int g = 5) {}
+  switch (g; int g = 5) {} // expected-warning {{expression result unused}}
 
   if (int a, b; int c = a) { // expected-note 6{{previous}}
     int a; // expected-error {{redefinition}}
     int b; // expected-error {{redefinition}}
     int c; // expected-error {{redefinition}}
   } else {
     int a; // expected-error {{redefinition}}
     int b; // expected-error {{redefinition}}
     int c; // expected-error {{redefinition}}
   }
 
   return 0;
 }

test/Parser/switch-recovery.cpp

switch(x) {
     case 1: return;
     2: case; // expected-error {{expected 'case' keyword before expression}} \
                 expected-error {{expected expression}}
     4:5:6: return; // expected-error 3{{expected 'case' keyword before expression}}
     7: :x; // expected-error {{expected 'case' keyword before expression}} \
               expected-error {{expected expression}}
     8:: x; // expected-error {{expected ';' after expression}} \
               expected-error {{no member named 'x' in the global namespace; did you mean simply 'x'?}} \
-              expected-warning 2 {{expression result unused}}
+              expected-warning {{expression result unused}}
     9:: :y; // expected-error {{expected ';' after expression}} \
                expected-error {{expected unqualified-id}} \
                expected-warning {{expression result unused}}
     :; // expected-error {{expected expression}}
     ::; // expected-error {{expected unqualified-id}}
   }
 }
 

test/SemaCXX/cxx1z-init-statement.cpp

-// RUN: %clang_cc1 -std=c++1z -verify %s
-// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++1z -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -std=c++17 -Wno-unused-value -verify %s
 
 void testIf() {
   int x = 0;
   if (x; x) ++x;
   if (int t = 0; t) ++t; else --t;
 
   if (int x, y = 0; y) // expected-note 2 {{previous definition is here}}
     int x = 0; // expected-error {{redefinition of 'x'}}
   else
     int x = 0; // expected-error {{redefinition of 'x'}}
 
   if (x; int a = 0) ++a;
-  if (x, +x; int a = 0) // expected-note 2 {{previous definition is here}} expected-warning {{unused}}
+  if (x, +x; int a = 0) // expected-note 2 {{previous definition is here}}
     int a = 0; // expected-error {{redefinition of 'a'}}
   else
     int a = 0; // expected-error {{redefinition of 'a'}}
 
   if (int b = 0; b)
     ;
   b = 2; // expected-error {{use of undeclared identifier}}
 }
case 1:
       int y = 0; // expected-error {{redefinition of 'y'}}
   };
 
   switch (x; int a = 0) {
     case 0:
       ++a;
   }
 
-  switch (x, +x; int a = 0) { // expected-note {{previous definition is here}} expected-warning {{unused}}
+  switch (x, +x; int a = 0) { // expected-note {{previous definition is here}}
     case 0:
       int a = 0; // expected-error {{redefinition of 'a'}} // expected-note {{previous definition is here}}
     case 1:
       int a = 0; // expected-error {{redefinition of 'a'}}
   }
 
   switch (int b = 0; b) {
     case 0:

test/SemaCXX/for-range-examples.cpp

 }
 
 namespace test4 {
   void f() {
     int y;
 
     // Make sure these don't crash. Better diagnostics would be nice.
     for (: {1, 2, 3}) {} // expected-error {{expected expression}} expected-error {{expected ';'}}
-    for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+    for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
     for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} expected-error {{expected ';'}}
-    for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+    for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
   }
 }
 
 namespace test5 {
   // Test error-recovery.
   void f() {
     for (auto x : undeclared_identifier) // expected-error {{undeclared identifier}}
       for (auto y : x->foo)

 }
 
 namespace PR32933 {
 // https://bugs.llvm.org/show_bug.cgi?id=32933
 void foo ()
 { 
   int b = 1, a[b];
   a[0] = 0;
-  [&] { for (int c : a) 0; } ();
+  [&] { for (int c : a) 0; } (); // expected-warning {{expression result unused}}
 }
 
 
 int foo(int b) {
   int varr[b][(b+=8)];
   b = 15; 
   [&] {
     int i = 0;

test/SemaCXX/warn-unused-result.cpp

void test() {
 
   int i = f();
   S s1 = g1();
   S *s2 = g2();
   S &s3 = g3();
   const S &s4 = g1();
 }
 
+void testSubstmts(int i) {
+  switch (i) {
+  case 0:
+    f(); // expected-warning {{ignoring return value}}
+  default:
+    f(); // expected-warning {{ignoring return value}}
+  }
+
+  if (i)
+    f(); // expected-warning {{ignoring return value}}
+  else
+    f(); // expected-warning {{ignoring return value}}
+
+  while (i)
+    f(); // expected-warning {{ignoring return value}}
+
+  do
+    f(); // expected-warning {{ignoring return value}}
+  while (i);
+
+  for (f(); // expected-warning {{ignoring return value}}
+       ;
+       f() // expected-warning {{ignoring return value}}
+      )
+    f(); // expected-warning {{ignoring return value}}
+
+  f(),  // expected-warning {{ignoring return value}}
+  (void)f();
+}
+
 struct X {
  int foo() __attribute__((warn_unused_result));
 };
 
 void bah() {
   X x, *x2;
   x.foo(); // expected-warning {{ignoring return value}}
   x2->foo(); // expected-warning {{ignoring return value}}
void f() {
   // Silencing the warning by cast to void still works.
   (void)s.DoThing();
   (void)s++;
   (void)p++;
   (void)++s;
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+    f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837

test/SemaObjCXX/foreach.mm

 // RUN: %clang_cc1 -fsyntax-only -fblocks -verify -std=c++11 %s
 // rdar://9293227
 
 @class NSArray;
 
 void f(NSArray *a) {
     id keys;
     for (int i : a); // expected-error{{selector element type 'int' is not a valid object}} 
-    for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}}
-    for (2 : a); // expected-error {{for range declaration must declare a variable}}
+    for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
+    for (2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
   
   for (id thisKey : keys);
 
   for (auto thisKey : keys) { } // expected-warning{{'auto' deduced as 'id' in declaration of 'thisKey'}}
 }
 
 void for_init_stmt() {
   for (id keys; id key : keys) {} // expected-warning{{extension}} expected-error{{not supported}}

 }
 
 /* rdar://problem/11068137 */
 @interface Test2
 @property (assign) id prop;
 @end
 void test2(NSObject<NSFastEnumeration> *collection) {
   Test2 *obj;
-  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}}
+  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}} expected-warning {{property access result unused}}
   }
 }
 
 void testErrors(NSArray *array) {
   typedef int fn(int);
 
   for (fn x in array) { } // expected-error{{non-variable declaration in 'for' loop}}
 }