Corrrect warn_unused_result attribute

[erichkeane]

Summary

The original idea was that if the attribute on an operator, that the return-value unused-ness wouldn't matter. However, all of the operators except postfix inc/dec return references! References don't result in this warning anyway, so those are already excluded.

However, the existing patch(in addition to missing a bunch of valid cases, such as a function returning a copy) would still hit non-member operators anyway! This patch removes the previous condition (if our current function is in the warn-unused-result'ed type) and replaces it with one that explicitly checks for post inc/dec (since these are likely going to be SO common that warning on them would be extreme).

[aaron.ballman]

lib/AST/Decl.cpp

3007 ↗

(On Diff #95664)

Please don't use auto here.

3010 ↗

(On Diff #95664)

CXXMethodDecl represents a static or instance method, so I'm not certain this logic is quite correct.

test/SemaCXX/warn-unused-result.cpp

166 ↗

(On Diff #95664)

Can you also add a test with [[nodiscard]] and verify that the uses of increment/decrement *are* diagnosed?

[erichkeane]

lib/AST/Decl.cpp

3010 ↗

(On Diff #95664)

I actually stole this logic from lib/Sema/SemaDeclCXX.cpp lines 12708 and 12754. I believe the logic here is that it operators aren't allowed to be static? Since it is illegal to do so, I would expect that I'd simply need to do an assert to prevent it?

I'm open for ideas if you have one here, I'm not sure what the 'right' thing to do is.

test/SemaCXX/warn-unused-result.cpp

166 ↗

(On Diff #95664)

I can add that, it will obviously cause a slight change in logic since I didn't handle that separately. Are we surewe WANT [[nodiscard]] to warn in this case?

[erichkeane]

1 more thing: I excepted post-increment/decrement from this warning because I figured it would give a TON of warnings from people using post-inc/dec in for loops.

However, after further discussion with Craig (who brought this up on the mailing list), I severely wonder if that is the 'right' thing to do. Someone who is going to mark their struct in this way would likely ALSO mean it in this case, right?

Aaron: Should I simply remove the special casing all together, so that it warns on post inc/dec? That would definitely solve all your concerns in the review so far.

[aaron.ballman]

In D32207#730681, @erichkeane wrote:

1 more thing: I excepted post-increment/decrement from this warning because I figured it would give a TON of warnings from people using post-inc/dec in for loops.

However, after further discussion with Craig (who brought this up on the mailing list), I severely wonder if that is the 'right' thing to do. Someone who is going to mark their struct in this way would likely ALSO mean it in this case, right?

Aaron: Should I simply remove the special casing all together, so that it warns on post inc/dec? That would definitely solve all your concerns in the review so far.

I think that would make sense, yes.

lib/AST/Decl.cpp

3010 ↗

(On Diff #95664)

I was wondering more how a friend declaration of the operator might show up. e.g.,

struct S {
  friend S operator++(S Operand, int);
};

test/SemaCXX/warn-unused-result.cpp

166 ↗

(On Diff #95664)

I think so, yes. The C++ standard doesn't provide normative requirements here, but its encouragement to implementers doesn't mention this kind of behavior. [dcl.attr.nodiscard]p2:

A nodiscard call is a function call expression that calls a function previously declared nodiscard, or
whose return type is a possibly cv-qualified class or enumeration type marked nodiscard. Appearance of a nodiscard call as a potentially-evaluated discarded-value expression (Clause 8) is discouraged unless explicitly cast to void. Implementations are encouraged to issue a warning in such cases. This is typically because discarding the return value of a nodiscard call has surprising consequences.

Since this is returning a class type that is marked as nodiscard, I would expect it to be diagnosed with the standard attribute spelling.

[erichkeane]

As discussed, removed the exception for postfix operators. It seems like the right thing to do.I didn't add the [[nodiscard]] test, since it is the same intended behavior now.

[aaron.ballman]

I'm surprised this change didn't cause any other tests to need to be updated. A few small formatting nits and a request for another test, but otherwise looking good.

test/SemaCXX/warn-unused-result.cpp

166 ↗

(On Diff #95797)

Add a space between struct and the attribute introducer.

175 ↗

(On Diff #95797)

Same here.

200 ↗

(On Diff #95797)

Can you add some tests showing that casting to void still silence the diagnostic even for these operator cases? It doesn't need to happen for all of them, but it's good to ensure that still behaves as expected.

[erichkeane]

Added the tests, plus the two formatting changes.

[aaron.ballman]

LGTM!

cfe/trunk/include/clang/AST/Decl.h

QualType getCallResultType() const {
     return getType()->getAs<FunctionType>()->getCallResultType(getASTContext());
   }
 
   /// \brief Returns the WarnUnusedResultAttr that is either declared on this
   /// function, or its return type declaration.
   const Attr *getUnusedResultAttr() const;
 
   /// \brief Returns true if this function or its return type has the
-  /// warn_unused_result attribute. If the return type has the attribute and
-  /// this function is a method of the return type's class, then false will be
-  /// returned to avoid spurious warnings on member methods such as assignment
-  /// operators.
+  /// warn_unused_result attribute.
   bool hasUnusedResultAttr() const { return getUnusedResultAttr() != nullptr; }
 
   /// \brief Returns the storage class as written in the source. For the
   /// computed linkage of symbol, see getLinkage.
   StorageClass getStorageClass() const { return StorageClass(SClass); }
 
   /// \brief Determine whether the "inline" keyword was specified for this
   /// function.

cfe/trunk/lib/AST/Decl.cpp

if (!FTL)
     return SourceRange();
 
   return FTL.getExceptionSpecRange();
 }
 
 const Attr *FunctionDecl::getUnusedResultAttr() const {
   QualType RetType = getReturnType();
   if (RetType->isRecordType()) {
-    const CXXRecordDecl *Ret = RetType->getAsCXXRecordDecl();
-    const auto *MD = dyn_cast<CXXMethodDecl>(this);
-    if (Ret && !(MD && MD->getCorrespondingMethodInClass(Ret, true))) {
+    if (const CXXRecordDecl *Ret = RetType->getAsCXXRecordDecl()) {
       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;
     }

cfe/trunk/test/SemaCXX/warn-unused-result.cpp

void g() {
 
   // The sizeof expression operand is never evaluated.
   (void)sizeof(f(), c); // Should not warn.
 
    // The noexcept expression operand is never evaluated.
   (void)noexcept(f(), false); // Should not warn.
 }
 }
+
+namespace {
+// C++ Methods should warn even in their own class.
+struct [[clang::warn_unused_result]] S {
+  S DoThing() { return {}; };
+  S operator++(int) { return {}; };
+  S operator--(int) { return {}; };
+  // Improperly written prefix.
+  S operator++() { return {}; };
+  S operator--() { return {}; };
+};
+
+struct [[clang::warn_unused_result]] P {
+  P DoThing() { return {}; };
+};
+
+P operator++(const P &, int) { return {}; };
+P operator--(const P &, int) { return {}; };
+// Improperly written prefix.
+P operator++(const P &) { return {}; };
+P operator--(const P &) { return {}; };
+
+void f() {
+  S s;
+  P p;
+  s.DoThing(); // expected-warning {{ignoring return value}}
+  p.DoThing(); // expected-warning {{ignoring return value}}
+  // Only postfix is expected to warn when written correctly.
+  s++; // expected-warning {{ignoring return value}}
+  s--; // expected-warning {{ignoring return value}}
+  p++; // expected-warning {{ignoring return value}}
+  p--; // expected-warning {{ignoring return value}}
+  // Improperly written prefix operators should still warn.
+  ++s; // expected-warning {{ignoring return value}}
+  --s; // expected-warning {{ignoring return value}}
+  ++p; // expected-warning {{ignoring return value}}
+  --p; // expected-warning {{ignoring return value}}
+
+  // Silencing the warning by cast to void still works.
+  (void)s.DoThing();
+  (void)s++;
+  (void)p++;
+  (void)++s;
+  (void)++p;
+}
+} // namespace