[clang] Diagnose [[nodiscard]] return types in Objective-C++ #142541

halbi2 · 2025-06-03T06:06:44Z

Fixes #141504. @AaronBallman please take a look

My solution was to copy-paste getUnusedResultAttr and hasUnusedResultAttr from CallExpr into ObjCMessageExpr too.

Fixes llvm#141504

llvmbot · 2025-06-03T06:07:19Z

@llvm/pr-subscribers-clang

Author: None (halbi2)

Changes

Fixes #141504. @aaronballman please take a look

My solution was to copy-paste getUnusedResultAttr and hasUnusedResultAttr from CallExpr into ObjCMessageExpr too.

Full diff: https://github.com/llvm/llvm-project/pull/142541.diff

6 Files Affected:

(modified) clang/include/clang/AST/ExprObjC.h (+11)
(modified) clang/lib/AST/Expr.cpp (+5-6)
(modified) clang/lib/AST/ExprObjC.cpp (+21)
(modified) clang/lib/Sema/SemaStmt.cpp (+6-7)
(modified) clang/test/SemaCXX/warn-unused-result.cpp (+25)
(added) clang/test/SemaObjCXX/attr-nodiscard.mm (+26)

diff --git a/clang/include/clang/AST/ExprObjC.h b/clang/include/clang/AST/ExprObjC.h
index f87fa85569c44..a6b2fca3df13a 100644
--- a/clang/include/clang/AST/ExprObjC.h
+++ b/clang/include/clang/AST/ExprObjC.h
@@ -1236,6 +1236,17 @@ class ObjCMessageExpr final
   /// of `instancetype` (in that case it's an expression type).
   QualType getCallReturnType(ASTContext &Ctx) const;
 
+  /// Returns the WarnUnusedResultAttr that is either declared on the called
+  /// method, or its return type declaration, together with a NamedDecl that
+  /// refers to the declaration the attribute is attached onto.
+  std::pair<const NamedDecl *, const Attr *>
+  getUnusedResultAttr(ASTContext &Ctx) const;
+
+  /// Returns true if this message send should warn on unused results.
+  bool hasUnusedResultAttr(ASTContext &Ctx) const {
+    return getUnusedResultAttr(Ctx).second != nullptr;
+  }
+
   /// Source range of the receiver.
   SourceRange getReceiverRange() const;
 
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index fe874ccd7b60f..1142745a25f1a 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -2869,12 +2869,11 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
       return true;
     }
 
-    if (const ObjCMethodDecl *MD = ME->getMethodDecl())
-      if (MD->hasAttr<WarnUnusedResultAttr>()) {
-        WarnE = this;
-        Loc = getExprLoc();
-        return true;
-      }
+    if (ME->hasUnusedResultAttr(Ctx)) {
+      WarnE = this;
+      Loc = getExprLoc();
+      return true;
+    }
 
     return false;
   }
diff --git a/clang/lib/AST/ExprObjC.cpp b/clang/lib/AST/ExprObjC.cpp
index 79b5db301d414..1de679ab87140 100644
--- a/clang/lib/AST/ExprObjC.cpp
+++ b/clang/lib/AST/ExprObjC.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/ComputeDependence.h"
 #include "clang/AST/SelectorLocationsKind.h"
 #include "clang/AST/Type.h"
@@ -272,6 +273,26 @@ QualType ObjCMessageExpr::getCallReturnType(ASTContext &Ctx) const {
   return Ctx.getReferenceQualifiedType(this);
 }
 
+std::pair<const NamedDecl *, const Attr *>
+ObjCMessageExpr::getUnusedResultAttr(ASTContext &Ctx) const {
+  // If the callee is marked nodiscard, return that attribute
+  if (const ObjCMethodDecl *MD = getMethodDecl())
+    if (const auto *A = MD->getAttr<WarnUnusedResultAttr>())
+      return {nullptr, A};
+
+  // 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 {TD, A};
+
+  for (const auto *TD = getCallReturnType(Ctx)->getAs<TypedefType>(); TD;
+       TD = TD->desugar()->getAs<TypedefType>())
+    if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
+      return {TD->getDecl(), A};
+  return {nullptr, nullptr};
+}
+
 SourceRange ObjCMessageExpr::getReceiverRange() const {
   switch (getReceiverKind()) {
   case Instance:
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index c943645c3ab9d..91a3b3eb43b1e 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -344,13 +344,12 @@ void DiagnoseUnused(Sema &S, const Expr *E, std::optional<unsigned> DiagID) {
       S.Diag(Loc, diag::err_arc_unused_init_message) << R1;
       return;
     }
-    const ObjCMethodDecl *MD = ME->getMethodDecl();
-    if (MD) {
-      if (DiagnoseNoDiscard(S, nullptr, MD->getAttr<WarnUnusedResultAttr>(),
-                            Loc, R1, R2,
-                            /*isCtor=*/false))
-        return;
-    }
+
+    auto [OffendingDecl, A] = ME->getUnusedResultAttr(S.Context);
+    if (DiagnoseNoDiscard(S, OffendingDecl,
+                          cast_or_null<WarnUnusedResultAttr>(A), Loc, R1, R2,
+                          /*isCtor=*/false))
+      return;
   } else if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) {
     const Expr *Source = POE->getSyntacticForm();
     // Handle the actually selected call of an OpenMP specialized call.
diff --git a/clang/test/SemaCXX/warn-unused-result.cpp b/clang/test/SemaCXX/warn-unused-result.cpp
index 5105f347db8b5..a1d201ec861d5 100644
--- a/clang/test/SemaCXX/warn-unused-result.cpp
+++ b/clang/test/SemaCXX/warn-unused-result.cpp
@@ -364,3 +364,28 @@ void id_print_name() {
     ((int(*)())f)();
 }
 } // namespace GH117975
+
+namespace inheritance {
+// Test that [[nodiscard]] is not inherited by derived class types,
+// but is inherited by member functions
+struct [[nodiscard]] E {
+  [[nodiscard]] explicit E(int);
+  explicit E(const char*);
+  [[nodiscard]] int f();
+};
+struct F : E {
+  using E::E;
+};
+E e();
+F f();
+void test() {
+  e();     // expected-warning {{ignoring return value of type 'E' declared with 'nodiscard' attribute}}
+  f();     // no warning: derived class type does not inherit the attribute
+  E(1);    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  E("x");  // expected-warning {{ignoring temporary of type 'E' declared with 'nodiscard' attribute}}
+  F(1);    // no warning: inherited constructor does not inherit the attribute either
+  F("x");  // no warning
+  e().f(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  f().f(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}
+} // namespace inheritance
diff --git a/clang/test/SemaObjCXX/attr-nodiscard.mm b/clang/test/SemaObjCXX/attr-nodiscard.mm
new file mode 100644
index 0000000000000..e1eefb74d3961
--- /dev/null
+++ b/clang/test/SemaObjCXX/attr-nodiscard.mm
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+template<class T>
+struct [[nodiscard]] expected {};
+
+using E = expected<int>;
+
+@interface INTF
+- (int) a [[nodiscard]];
++ (int) b [[nodiscard]];
+- (expected<int>) c;
++ (expected<int>) d;
+- (E) e;
++ (E) f;
+- (void) g [[nodiscard]]; // expected-warning {{attribute 'nodiscard' cannot be applied to Objective-C method without return value}}
+@end
+
+void foo(INTF *a) {
+  [a a]; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  [INTF b]; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  [a c]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
+  [INTF d]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
+  [a e]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
+  [INTF f]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
+  [a g];
+}

AaronBallman

Thank you for the fix! Can you also add some test coverage for Objective-C (the GNU spelling is always available in C mode and the [[nodiscard]] spelling is available in C23 and later). Also, the changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the fix.

AaronBallman · 2025-06-03T15:26:22Z

clang/lib/AST/ExprObjC.cpp

+  // If the callee is marked nodiscard, return that attribute
+  if (const ObjCMethodDecl *MD = getMethodDecl())
+    if (const auto *A = MD->getAttr<WarnUnusedResultAttr>())
+      return {nullptr, A};


Shouldn't this be {MD, A}?

It is this way in CallExpr::getUnusedResultAttribute too. Probably the NamedDecl is supposed to be set only when it is a type declaration.

Can you better explain that? What happens if this DOES set the MD correctly? Do we have a test that shows the behavior change?

Yes, if this is changed to return {MD, A} then SemaObjC/method-warn-unused-attribute.m fails with

ignoring return value of type 'fee' declared with 'warn_unused_result' attribute

instead of the expected

ignoring return value of function declared with 'warn_unused_result' attribute

For more context, see #112521, but tl;dr all of this is because we want to issue an unused result diagnostic in two different situations: either because the function is declared nodiscard or because the return type is declared nodiscard. If both are nodiscard, then we prefer printing the function as the cause instead of the type.

Specifically,

{null, Attribute} means ‘the function is nodiscard’;

{TD, Attribute} means ‘the type is nodiscard, please include TD in the diagnostic;

{null, null} means ‘neither is nodiscard’.

I thought there was a comment about this somewhere, but if not should probably add one

The attribute I believe is returned so we can get the diagnostic message from it but don’t quote me on that.

erichkeane · 2025-06-03T15:27:37Z

clang/lib/AST/ExprObjC.cpp

@@ -272,6 +273,26 @@ QualType ObjCMessageExpr::getCallReturnType(ASTContext &Ctx) const {
  return Ctx.getReferenceQualifiedType(this);
 }

+std::pair<const NamedDecl *, const Attr *>


Since we're casting back and forth always, should this be:

Suggested change

std::pair<const NamedDecl *, const Attr *>

std::pair<const NamedDecl *, const WarnUnusedResultAttr *>

?

It is this way in CallExpr::getUnusedResultAttr too. I would change it in both places?
Maybe is this way so that different Attrs can be supported in the future, not just WarnUnusedResultAttrs.

I think we're decades past 'maybe we'll support other kinds some day' :D

If you're willing to do the change on CallExpr at the same time, please do.

erichkeane · 2025-06-03T15:28:52Z

clang/lib/AST/ExprObjC.cpp

+  // If the callee is marked nodiscard, return that attribute
+  if (const ObjCMethodDecl *MD = getMethodDecl())
+    if (const auto *A = MD->getAttr<WarnUnusedResultAttr>())
+      return {nullptr, A};


Why can't this return the MethodDecl here?

rjmccall

Generally LGTM, although I agree with requests above.

Sirraide

I would prefer if the code from CallExpr::getUnusedResultAttr was factored out into a separate function and reused rather than copy-pasted here since the code is identical save for the class that this is a member of. Specifically, if we make it a function template somewhere (ASTContext?) and pass in the CallExpr/ObjCMessageExpr, we should just be able to reuse it as-is.

Sirraide · 2025-06-09T12:20:53Z

factored out into a separate function

Not so much factored out as just made into a template since it is already a separate function.

halbi2 added 2 commits May 26, 2025 15:35

[clang] [test] More coverage of [[nodiscard]]

c683b2a

[clang] Diagnose nodiscard return types in Objective-C++

0c5aa22

Fixes llvm#141504

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 3, 2025

AaronBallman requested review from rjmccall, AaronBallman and erichkeane June 3, 2025 15:24

AaronBallman added objective-c++ objective-c labels Jun 3, 2025

AaronBallman reviewed Jun 3, 2025

View reviewed changes

erichkeane reviewed Jun 3, 2025

View reviewed changes

rjmccall reviewed Jun 3, 2025

View reviewed changes

halbi2 added 2 commits June 5, 2025 22:38

Review comments

c0e5f84

Review comments

9bad608

Sirraide reviewed Jun 9, 2025

View reviewed changes

	std::pair<const NamedDecl , const Attr >
	std::pair<const NamedDecl , const WarnUnusedResultAttr >

[clang] Diagnose [[nodiscard]] return types in Objective-C++ #142541

Are you sure you want to change the base?

[clang] Diagnose [[nodiscard]] return types in Objective-C++ #142541

Uh oh!

Conversation

halbi2 commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sirraide Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sirraide Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Sirraide commented Jun 9, 2025

Uh oh!

Uh oh!

Sirraide Jun 9, 2025 •

edited

Loading

Sirraide Jun 9, 2025 •

edited

Loading