Correctly handle function pointers returning a type marked nodiscard

[aaron.ballman]

Summary

When a function returns a type and that type was declared [[nodiscard]], we diagnose any unused results from that call as though the function were marked nodiscard. The same behavior should apply to calls through a function pointer.

This addresses PR31526.

[erik.pilkington]

LGTM!

lib/AST/Expr.cpp

2281–2286

This duplicates some logic from FunctionDecl::hasUnusedResultAttr(), maybe we should move that member function to the CallExpr?

test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp

51

This diagnostic should also probably be improved at some point, the function wasn't declared 'nodiscard', the type was.

[aaron.ballman]

Updated based on review feedback.

Moved FunctionDecl::hasUnusedResultAttr() and FunctionDecl::getUnusedResultAttr() to CallExpr to reduce logic duplication on checking.

[aaron.ballman]

There's enough churn based on the review feedback that this should probably have a second round of review just to be sure.

lib/AST/Expr.cpp

2281–2286

Good suggestion -- I think that cleans up the interface nicely.

test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp

51

Yeah, I noticed that as well.

[aaron.ballman]

Ping

[erik.pilkington]

Still LG, sorry for the delay!

[aaron.ballman]

Committed in r350317.

include/clang/AST/Decl.h

   SourceRange getExceptionSpecSourceRange() const;
 
   /// Determine the type of an expression that calls this function.
   QualType getCallResultType() const {
     return getType()->castAs<FunctionType>()->getCallResultType(
         getASTContext());
   }
 
-  /// Returns the WarnUnusedResultAttr that is either declared on this
-  /// function, or its return type declaration.
-  const Attr *getUnusedResultAttr() const;
-
-  /// Returns true if this function or its return type has the
-  /// warn_unused_result attribute.
-  bool hasUnusedResultAttr() const { return getUnusedResultAttr() != nullptr; }
-
   /// Returns the storage class as written in the source. For the
   /// computed linkage of symbol, see getLinkage.
   StorageClass getStorageClass() const {
     return static_cast<StorageClass>(FunctionDeclBits.SClass);
   }
 
   /// Sets the storage class as written in the source.
   void setStorageClass(StorageClass SClass) {

include/clang/AST/Expr.h

   /// evaluate side-effects within its arguments.
   bool isUnevaluatedBuiltinCall(const ASTContext &Ctx) const;
 
   /// getCallReturnType - Get the return type of the call expr. This is not
   /// always the type of the expr itself, if the return type is a reference
   /// type.
   QualType getCallReturnType(const ASTContext &Ctx) const;
 
+  /// Returns the WarnUnusedResultAttr that is either declared on the called
+  /// function, or its return type declaration.
+  const Attr *getUnusedResultAttr(const ASTContext &Ctx) const;
+
+  /// Returns true if this call expression should warn on unused results.
+  bool hasUnusedResultAttr(const ASTContext &Ctx) const {
+    return getUnusedResultAttr(Ctx) != nullptr;
+  }
+
   SourceLocation getRParenLoc() const { return RParenLoc; }
   void setRParenLoc(SourceLocation L) { RParenLoc = L; }
 
   SourceLocation getBeginLoc() const LLVM_READONLY;
   SourceLocation getEndLoc() const LLVM_READONLY;
 
   /// Return true if this is a call to __assume() or __builtin_assume() with
   /// a non-value-dependent constant parameter evaluating as false.

lib/AST/Decl.cpp

   FunctionTypeLoc FTL =
     TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
   if (!FTL)
     return SourceRange();
 
   return FTL.getExceptionSpecRange();
 }
 
-const Attr *FunctionDecl::getUnusedResultAttr() const {
-  QualType RetType = getReturnType();
-  if (const auto *Ret = RetType->getAsRecordDecl()) {
-    if (const auto *R = Ret->getAttr<WarnUnusedResultAttr>())
-      return R;
-  } else if (const auto *ET = RetType->getAs<EnumType>()) {
-    if (const EnumDecl *ED = ET->getDecl()) {
-      if (const auto *R = ED->getAttr<WarnUnusedResultAttr>())
-        return R;
-    }
-  }
-  return getAttr<WarnUnusedResultAttr>();
-}
-
 /// For an inline function definition in C, or for a gnu_inline function
 /// in C++, determine whether the definition will be externally visible.
 ///
 /// Inline function definitions are always available for inlining optimizations.
 /// However, depending on the language dialect, declaration specifiers, and
 /// attributes, the definition of an inline function may or may not be
 /// "externally" visible to other translation units in the program.
 ///

lib/AST/Expr.cpp

     // This should never be overloaded and so should never return null.
     CalleeType = Expr::findBoundMemberType(Callee);
   }
 
   const FunctionType *FnType = CalleeType->castAs<FunctionType>();
   return FnType->getReturnType();
 }
 
+const Attr *CallExpr::getUnusedResultAttr(const ASTContext &Ctx) const {
+  // If the return type is a struct, union, or enum that is marked nodiscard,
+  // then return the return type attribute.
+  if (const TagDecl *TD = getCallReturnType(Ctx)->getAsTagDecl())
+    if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())
+      return A;
+
+  // Otherwise, see if the callee is marked nodiscard and return that attribute
+  // instead.
+  const Decl *D = getCalleeDecl();
+  return D ? D->getAttr<WarnUnusedResultAttr>() : nullptr;
+}
+
 SourceLocation CallExpr::getBeginLoc() const {
   if (isa<CXXOperatorCallExpr>(this))
     return cast<CXXOperatorCallExpr>(this)->getBeginLoc();
 
   SourceLocation begin = getCallee()->getBeginLoc();
   if (begin.isInvalid() && getNumArgs() > 0 && getArg(0))
     begin = getArg(0)->getBeginLoc();
   return begin;

           Op->getCallReturnType(Ctx)->isVoidType())
         break;
       WarnE = this;
       Loc = Op->getOperatorLoc();
       R1 = Op->getSourceRange();
       return true;
     }
 
     // Fallthrough for generic call handling.
     LLVM_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()) {
-      const FunctionDecl *Func = dyn_cast<FunctionDecl>(FD);
-      bool HasWarnUnusedResultAttr = Func ? Func->hasUnusedResultAttr()
-                                          : FD->hasAttr<WarnUnusedResultAttr>();
-
       // If the callee has attribute pure, const, or warn_unused_result, 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 (HasWarnUnusedResultAttr ||
+      if (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());

lib/Sema/SemaStmt.cpp

   if (DiagnoseUnusedComparison(*this, E))
     return;
 
   E = WarnExpr;
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     if (E->getType()->isVoidType())
       return;
 
+    if (const Attr *A = CE->getUnusedResultAttr(Context)) {
+      Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+      return;
+    }
+
     // If the callee has attribute pure, const, or warn_unused_result, warn with
     // a more specific message to make it clear what is happening. If the call
     // is written in a macro body, only warn if it has the warn_unused_result
     // attribute.
     if (const Decl *FD = CE->getCalleeDecl()) {
-      if (const Attr *A = isa<FunctionDecl>(FD)
-                              ? cast<FunctionDecl>(FD)->getUnusedResultAttr()
-                              : FD->getAttr<WarnUnusedResultAttr>()) {
-        Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
-        return;
-      }
       if (ShouldSuppress)
         return;
       if (FD->hasAttr<PureAttr>()) {
         Diag(Loc, diag::warn_unused_call) << R1 << R2 << "pure";
         return;
       }
       if (FD->hasAttr<ConstAttr>()) {
         Diag(Loc, diag::warn_unused_call) << R1 << R2 << "const";

test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp

 
 [[nodiscard]] volatile char &(*fp)();
 void g() {
   fp(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 
   // OK, warning suppressed.
   (void)fp();
 }
+
+namespace PR31526 {
+typedef E (*fp1)();
+typedef S (*fp2)();
+
+typedef S S_alias;
+typedef S_alias (*fp3)();
+
+typedef fp2 fp2_alias;
+
+void f() {
+  fp1 one;
+  fp2 two;
+  fp3 three;
+  fp2_alias four;
+
+  one(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  two(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  three(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  four(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+
+  // These are all okay because of the explicit cast to void.
+  (void)one();
+  (void)two();
+  (void)three();
+  (void)four();
+}
+} // namespace PR31526
+
 #ifdef EXT
 // expected-warning@4 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@8 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@11 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@12 {{use of the 'nodiscard' attribute is a C++17 extension}}
 // expected-warning@28 {{use of the 'nodiscard' attribute is a C++17 extension}}
 #endif