[clang] Add -Wunused-result-always warning

[gpanders]

Summary

This flag enables warnings for unused function return values
universally, even for functions which do not have a "warn_unused_result"
attribute.

This is disabled by default as it is far too noisy for most C/C++ code;
however, there are contexts where this is a desirable warning to have
(for example, in safety critical code where ignoring return values is
usually forbidden).

[gpanders]

Remove cast to (void) in unused-expr.c

This cast is not necessary for this test, because foo does not have a
"warn_unused_result" attribute and is not marked const or pure. Removing the
cast improves the usefuless of the test as it verifies that there is no warning
issued in such a case (but a warning *would* be issued with
-Wunused-result-always, so this also acts as a "negative case" to ensure no
warning is issued when that flag is not present).

[dblaikie]

Seems more stylistic/unlikely to meet the diagnostic bar (certtainly couldn't be on-by-default for instance) for clang to me (but could go into something like clang-tidy), at least.

[aaron.ballman]

In D149380#4311237, @dblaikie wrote:

Seems more stylistic/unlikely to meet the diagnostic bar (certtainly couldn't be on-by-default for instance) for clang to me (but could go into something like clang-tidy), at least.

+1 -- we expect new diagnostics to be ones we can enable by default (experience has shown us that off-by-default warnings don't typically get enabled often enough to be worth adding except for special circumstances), and there's basically no way to enable this by default. However, it does seem appropriate as a clang-tidy check: https://clang.llvm.org/extra/clang-tidy/Contributing.html

clang/include/clang/AST/Expr.h

public:
   /// in C++11 onwards. This applies to certain forms of volatile glvalues.
   bool isReadIfDiscardedInCPlusPlus11() const;
 
   /// isUnusedResultAWarning - Return true if this immediate expression should
   /// be warned about if the result is unused.  If so, fill in expr, location,
   /// and ranges with expr to warn on and source locations/ranges appropriate
   /// for a warning.
   bool isUnusedResultAWarning(const Expr *&WarnExpr, SourceLocation &Loc,
-                              SourceRange &R1, SourceRange &R2,
-                              ASTContext &Ctx) const;
+                              SourceRange &R1, SourceRange &R2, ASTContext &Ctx,
+                              bool Always) const;
 
   /// isLValue - True if this expression is an "l-value" according to
   /// the rules of the current language.  C and C++ give somewhat
   /// different rules for this concept, but in general, the result of
   /// an l-value expression identifies a specific object whereas the
   /// result of an r-value expression is a value detached from any
   /// specific storage.
   ///

clang/include/clang/Basic/DiagnosticGroups.td

 def UnusedTemplate : DiagGroup<"unused-template", [UnneededInternalDecl]>;
 def UnusedMemberFunction : DiagGroup<"unused-member-function",
                                      [UnneededMemberFunction]>;
 def UnusedLabel : DiagGroup<"unused-label">;
 def UnusedLambdaCapture : DiagGroup<"unused-lambda-capture">;
 def UnusedParameter : DiagGroup<"unused-parameter">;
 def UnusedButSetParameter : DiagGroup<"unused-but-set-parameter">;
 def UnusedResult : DiagGroup<"unused-result">;
+def UnusedResultAlways : DiagGroup<"unused-result-always">;
 def PotentiallyEvaluatedExpression : DiagGroup<"potentially-evaluated-expression">;
 def UnevaluatedExpression : DiagGroup<"unevaluated-expression",
                                       [PotentiallyEvaluatedExpression]>;
 def UnusedValue : DiagGroup<"unused-value", [UnusedComparison, UnusedResult,
                                              UnevaluatedExpression]>;
 def UnusedConstVariable : DiagGroup<"unused-const-variable">;
 def UnusedVariable : DiagGroup<"unused-variable",
                                [UnusedConstVariable]>;

clang/include/clang/Basic/DiagnosticSemaKinds.td

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">,
   InGroup<UnusedResult>;
 def warn_unused_result_msg : Warning<
   "ignoring return value of function declared with %0 attribute: %1">,
   InGroup<UnusedResult>;
+def warn_unused_result_always : Warning<
+  "ignoring return value of function">,
+  InGroup<UnusedResultAlways>, DefaultIgnore;
 def warn_unused_result_typedef_unsupported_spelling : Warning<
   "'[[%select{nodiscard|gnu::warn_unused_result}0]]' attribute ignored when "
   "applied to a typedef; consider using '__attribute__((warn_unused_result))' "
   "or '[[clang::warn_unused_result]]' instead">, InGroup<IgnoredAttributes>;
 def warn_unused_volatile : Warning<
   "expression result unused; assign into a variable to force a volatile load">,
   InGroup<DiagGroup<"unused-volatile-lvalue">>;
 

clang/lib/AST/Expr.cpp

 }
 
 /// isUnusedResultAWarning - Return true if this immediate expression should
 /// be warned about if the result is unused.  If so, fill in Loc and Ranges
 /// with location to warn on and the source range[s] to report with the
 /// warning.
 bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
                                   SourceRange &R1, SourceRange &R2,
-                                  ASTContext &Ctx) const {
+                                  ASTContext &Ctx, bool Always) const {
   // Don't warn if the expr is type dependent. The type could end up
   // instantiating to void.
   if (isTypeDependent())
     return false;
 
   switch (getStmtClass()) {
   default:
     if (getType()->isVoidType())
       return false;
     WarnE = this;
     Loc = getExprLoc();
     R1 = getSourceRange();
     return true;
   case ParenExprClass:
-    return cast<ParenExpr>(this)->getSubExpr()->
-      isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<ParenExpr>(this)->getSubExpr()->isUnusedResultAWarning(
+        WarnE, Loc, R1, R2, Ctx, Always);
   case GenericSelectionExprClass:
-    return cast<GenericSelectionExpr>(this)->getResultExpr()->
-      isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<GenericSelectionExpr>(this)
+        ->getResultExpr()
+        ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
   case CoawaitExprClass:
   case CoyieldExprClass:
-    return cast<CoroutineSuspendExpr>(this)->getResumeExpr()->
-      isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<CoroutineSuspendExpr>(this)
+        ->getResumeExpr()
+        ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
   case ChooseExprClass:
-    return cast<ChooseExpr>(this)->getChosenSubExpr()->
-      isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<ChooseExpr>(this)->getChosenSubExpr()->isUnusedResultAWarning(
+        WarnE, Loc, R1, R2, Ctx, Always);
   case UnaryOperatorClass: {
     const UnaryOperator *UO = cast<UnaryOperator>(this);
 
     switch (UO->getOpcode()) {
     case UO_Plus:
     case UO_Minus:
     case UO_AddrOf:
     case UO_Not:
case UnaryOperatorClass: {
     case UO_Real:
     case UO_Imag:
       // accessing a piece of a volatile complex is a side-effect.
       if (Ctx.getCanonicalType(UO->getSubExpr()->getType())
           .isVolatileQualified())
         return false;
       break;
     case UO_Extension:
-      return UO->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+      return UO->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                      Always);
     }
     WarnE = this;
     Loc = UO->getOperatorLoc();
     R1 = UO->getSubExpr()->getSourceRange();
     return true;
   }
   case BinaryOperatorClass: {
     const BinaryOperator *BO = cast<BinaryOperator>(this);
     switch (BO->getOpcode()) {
       default:
         break;
       // Consider the RHS of comma for side effects. LHS was checked by
       // Sema::CheckCommaOperands.
       case BO_Comma:
         // ((foo = <blah>), 0) is an idiom for hiding the result (and
         // lvalue-ness) of an assignment written in a macro.
         if (IntegerLiteral *IE =
               dyn_cast<IntegerLiteral>(BO->getRHS()->IgnoreParens()))
           if (IE->getValue() == 0)
             return false;
-        return BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+        return BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                    Always);
       // Consider '||', '&&' to have side effects if the LHS or RHS does.
       case BO_LAnd:
       case BO_LOr:
-        if (!BO->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx) ||
-            !BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx))
+        if (!BO->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                  Always) ||
+            !BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                  Always))
           return false;
         break;
     }
     if (BO->isAssignmentOp())
       return false;
     WarnE = this;
     Loc = BO->getOperatorLoc();
     R1 = BO->getLHS()->getSourceRange();
     R2 = BO->getRHS()->getSourceRange();
     return true;
   }
   case CompoundAssignOperatorClass:
   case VAArgExprClass:
   case AtomicExprClass:
     return false;
 
   case ConditionalOperatorClass: {
     // If only one of the LHS or RHS is a warning, the operator might
     // be being used for control flow. Only warn if both the LHS and
     // RHS are warnings.
     const auto *Exp = cast<ConditionalOperator>(this);
-    return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx) &&
-           Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                 Always) &&
+           Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                 Always);
   }
   case BinaryConditionalOperatorClass: {
     const auto *Exp = cast<BinaryConditionalOperator>(this);
-    return Exp->getFalseExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return Exp->getFalseExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                       Always);
   }
 
   case MemberExprClass:
     WarnE = this;
     Loc = cast<MemberExpr>(this)->getMemberLoc();
     R1 = SourceRange(Loc, Loc);
     R2 = cast<MemberExpr>(this)->getBase()->getSourceRange();
     return true;
case CXXOperatorCallExprClass: {
     [[fallthrough]];
   }
   case CallExprClass:
   case CXXMemberCallExprClass:
   case UserDefinedLiteralClass: {
     // If this is a direct call, get the callee.
     const CallExpr *CE = cast<CallExpr>(this);
     if (const Decl *FD = CE->getCalleeDecl()) {
-      // If the callee has attribute pure, const, or warn_unused_result, warn
-      // about it. void foo() { strlen("bar"); } should warn.
+      // If the callee has attribute pure, const, or warn_unused_result, or if
+      // -Wunused-result-always is enabled, 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 (CE->hasUnusedResultAttr(Ctx) ||
-          FD->hasAttr<PureAttr>() || FD->hasAttr<ConstAttr>()) {
+      if (Always || CE->hasUnusedResultAttr(Ctx) || FD->hasAttr<PureAttr>() ||
+          FD->hasAttr<ConstAttr>()) {
         WarnE = this;
         Loc = CE->getCallee()->getBeginLoc();
         R1 = CE->getCallee()->getSourceRange();
 
         if (unsigned NumArgs = CE->getNumArgs())
           R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
                            CE->getArg(NumArgs - 1)->getEndLoc());
         return true;
if (auto *BO = dyn_cast<BinaryOperator>(POE->getSyntacticForm()))
       if (BO->isAssignmentOp())
         return false;
     if (auto *UO = dyn_cast<UnaryOperator>(POE->getSyntacticForm()))
       if (UO->isIncrementDecrementOp())
         return false;
 
     // Otherwise, warn if the result expression would warn.
     const Expr *Result = POE->getResultExpr();
-    return Result && Result->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return Result &&
+           Result->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
   }
 
   case StmtExprClass: {
     // Statement exprs don't logically have side effects themselves, but are
     // sometimes used in macros in ways that give them a type that is unused.
     // For example ({ blah; foo(); }) will end up with a type if foo has a type.
     // however, if the result of the stmt expr is dead, we don't want to emit a
     // warning.
     const CompoundStmt *CS = cast<StmtExpr>(this)->getSubStmt();
     if (!CS->body_empty()) {
       if (const Expr *E = dyn_cast<Expr>(CS->body_back()))
-        return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+        return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
       if (const LabelStmt *Label = dyn_cast<LabelStmt>(CS->body_back()))
         if (const Expr *E = dyn_cast<Expr>(Label->getSubStmt()))
-          return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+          return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
     }
 
     if (getType()->isVoidType())
       return false;
     WarnE = this;
     Loc = cast<StmtExpr>(this)->getLParenLoc();
     R1 = getSourceRange();
     return true;
if (CE->getCastKind() == CK_ToVoid) {
               return false;
 
         // The lvalue-to-rvalue conversion would have no effect for an array.
         // It's implausible that the programmer expected this to result in a
         // volatile array load, so don't warn.
         if (SubE->getType()->isArrayType())
           return false;
 
-        return SubE->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+        return SubE->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
       }
       return false;
     }
 
     // If this is a cast to a constructor conversion, check the operand.
     // Otherwise, the result of the cast is unused.
     if (CE->getCastKind() == CK_ConstructorConversion)
-      return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+      return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                      Always);
     if (CE->getCastKind() == CK_Dependent)
       return false;
 
     WarnE = this;
     if (const CXXFunctionalCastExpr *CXXCE =
             dyn_cast<CXXFunctionalCastExpr>(this)) {
       Loc = CXXCE->getBeginLoc();
       R1 = CXXCE->getSubExpr()->getSourceRange();
     } else {
       const CStyleCastExpr *CStyleCE = cast<CStyleCastExpr>(this);
       Loc = CStyleCE->getLParenLoc();
       R1 = CStyleCE->getSubExpr()->getSourceRange();
     }
     return true;
   }
   case ImplicitCastExprClass: {
     const CastExpr *ICE = cast<ImplicitCastExpr>(this);
 
     // lvalue-to-rvalue conversion on a volatile lvalue is a side-effect.
     if (ICE->getCastKind() == CK_LValueToRValue &&
         ICE->getSubExpr()->getType().isVolatileQualified())
       return false;
 
-    return ICE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return ICE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+                                                     Always);
   }
   case CXXDefaultArgExprClass:
-    return (cast<CXXDefaultArgExpr>(this)
-            ->getExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+    return (cast<CXXDefaultArgExpr>(this)->getExpr()->isUnusedResultAWarning(
+        WarnE, Loc, R1, R2, Ctx, Always));
   case CXXDefaultInitExprClass:
-    return (cast<CXXDefaultInitExpr>(this)
-            ->getExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+    return (cast<CXXDefaultInitExpr>(this)->getExpr()->isUnusedResultAWarning(
+        WarnE, Loc, R1, R2, Ctx, Always));
 
   case CXXNewExprClass:
     // FIXME: In theory, there might be new expressions that don't have side
     // effects (e.g. a placement new with an uninitialized POD).
   case CXXDeleteExprClass:
     return false;
   case MaterializeTemporaryExprClass:
     return cast<MaterializeTemporaryExpr>(this)
         ->getSubExpr()
-        ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+        ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
   case CXXBindTemporaryExprClass:
-    return cast<CXXBindTemporaryExpr>(this)->getSubExpr()
-               ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<CXXBindTemporaryExpr>(this)
+        ->getSubExpr()
+        ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, Always);
   case ExprWithCleanupsClass:
-    return cast<ExprWithCleanups>(this)->getSubExpr()
-               ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    return cast<ExprWithCleanups>(this)->getSubExpr()->isUnusedResultAWarning(
+        WarnE, Loc, R1, R2, Ctx, Always);
   }
 }
 
 /// isOBJCGCCandidate - Check if an expression is objc gc'able.
 /// returns true, if it is; false otherwise.
 bool Expr::isOBJCGCCandidate(ASTContext &Ctx) const {
   const Expr *E = IgnoreParens();
   switch (E->getStmtClass()) {

clang/lib/Sema/SemaStmt.cpp

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)
+  if (!A && S.Diags.isIgnored(diag::warn_unused_result_always, Loc))
     return false;
+
+  if (!A) {
+    return S.Diag(Loc, diag::warn_unused_result_always) << R1 << R2;
+  }
+
   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;
   }
 
void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
   // checks need to happen, we factor out the macro-related test here.
   bool ShouldSuppress =
       SourceMgr.isMacroBodyExpansion(ExprLoc) ||
       SourceMgr.isInSystemMacro(ExprLoc);
 
   const Expr *WarnExpr;
   SourceLocation Loc;
   SourceRange R1, R2;
-  if (!E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Context))
+  bool WarnUnusedResultAlways =
+      !Diags.isIgnored(diag::warn_unused_result_always, ExprLoc);
+  if (!E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Context,
+                                 WarnUnusedResultAlways))
     return;
 
   // If this is a GNU statement expression expanded from a macro, it is probably
   // unused because it is a function-like macro that can be used as either an
   // expression or statement.  Don't warn, because it is almost certainly a
   // false positive.
   if (isa<StmtExpr>(E) && Loc.isMacroID())
     return;

clang/test/Sema/unused-result-always.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wno-unreachable-code -Wno-strict-prototypes -Wunused-result-always
+
+int t1(int X, int Y);
+void t2();
+void *t3();
+
+#define M1(a, b) t1(a, b)
+
+void bar() {
+  t1(1, 2);       // expected-warning {{ignoring return value of function}}
+
+  t2();           // no warning.
+
+  (void)t1(1, 2); // no warning;
+
+  t3();           // expected-warning {{ignoring return value of function}}
+
+  M1(1, 2);       // expected-warning {{ignoring return value of function}}
+}