[Sema] fix false -Wcomma being emitted from void returning functions

[inclyc]

Summary

Fixes https://github.com/llvm/llvm-project/issues/57151

[inclyc]

comments

[inclyc]

Sync comments of function IgnoreCommaOperand

[inclyc]

typo

[aaron.ballman]

Thank you for working on this fix! I think it's quite close to finished, but it needs some additional test coverage. Also, please add a release note about the fix so users know what's going on.

clang/lib/Sema/SemaExpr.cpp

13978–13980

clang/test/SemaCXX/warn-comma-operator.cpp

151

Can you also add test cases like:

struct S {
  void mem();
};

typedef void Void;
Void typedef_func();

void whatever() {
  S s;
  
  // Member function calls also work as expected.
  s.mem(), int_func();
  // As do lambda calls.
  []() { return; }(), int_func();
  // And we don't get confused about type aliases.
  typedef_func(), int_func();
  // Even function pointers don't confuse us.
  void (*fp)() = void_func;
  fp(), int_func();
}

[inclyc]

Address comments.

Thanks a lot for your suggestion, I noticed that the regression test tested both
C and C++, so I split the test mentioned in the comment into two parts.

[inclyc]

clang/docs/ReleaseNotes.rst

74

Do we need an NFC commit to unify the format of issues mentioned here? (Maybe it can be scheduled for a later release). The different ways of citing issues here can be confusing. Issue xxxx # xxx in this context.

[aaron.ballman]

LGTM!

clang/docs/ReleaseNotes.rst

74

If we wanted to do something like that, I think we should probably do that as part of the release process as a checklist item (CC @tstellar) but I don't have strong opinions either. We can certainly do it with NFC fixes as folks notice issues if we're fine still having some inconsistency when we release (which I'm fine with, personally).

clang/docs/ReleaseNotes.rst

- Fix a crash when evaluating a multi-dimensional array's array filler
   expression is element-dependent. This fixes
   `Issue 50601 <https://github.com/llvm/llvm-project/issues/56016>`_.
 - Fixed a crash-on-valid with consteval evaluation of a list-initialized
   constructor for a temporary object. This fixes
   `Issue 55871 <https://github.com/llvm/llvm-project/issues/55871>`_.
 - Fix `#57008 <https://github.com/llvm/llvm-project/issues/57008>`_ - Builtin
   C++ language extension type traits instantiated by a template with unexpected
   number of arguments cause an assertion fault.
+- Fix `#57151 <https://github.com/llvm/llvm-project/issues/57151>`_.
+  ``-Wcomma`` is emitted for void returning functions.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
   the enumeration's values. Due to the extended period of time this bug was
   present in major C++ implementations (including Clang), this error has the
   ability to be downgraded into a warning (via: -Wno-error=enum-constexpr-conversion)

clang/lib/Sema/SemaExpr.cpp

QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
   // qualified type, the value has the unqualified version of the type of the
   // lvalue; additionally, if the lvalue has atomic type, the value has the
   // non-atomic version of the type of the lvalue.
   // C++ 5.17p1: the type of the assignment expression is that of its left
   // operand.
   return getLangOpts().CPlusPlus ? LHSType : LHSType.getAtomicUnqualifiedType();
 }
 
-// Only ignore explicit casts to void.
-static bool IgnoreCommaOperand(const Expr *E) {
+// Scenarios to ignore if expression E is:
+// 1. an explicit cast expression into void
+// 2. a function call expression that returns void
+static bool IgnoreCommaOperand(const Expr *E, const ASTContext &Context) {
   E = E->IgnoreParens();
 
   if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
     if (CE->getCastKind() == CK_ToVoid) {
       return true;
     }
 
     // static_cast<void> on a dependent type will not show up as CK_ToVoid.
     if (CE->getCastKind() == CK_Dependent && E->getType()->isVoidType() &&
         CE->getSubExpr()->getType()->isDependentType()) {
       return true;
     }
   }
 
+  if (const auto *CE = dyn_cast<CallExpr>(E))
+    return CE->getCallReturnType(Context)->isVoidType();
   return false;
 }
 
 // Look for instances where it is likely the comma operator is confused with
 // another operator.  There is an explicit list of acceptable expressions for
 // the left hand side of the comma operator, otherwise emit a warning.
 void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
   // No warnings in macros
   if (Loc.isMacroID())
void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
   // of the comma operator as the LHS.
   while (const BinaryOperator *BO = dyn_cast<BinaryOperator>(LHS)) {
     if (BO->getOpcode() != BO_Comma)
       break;
     LHS = BO->getRHS();
   }
 
   // Only allow some expressions on LHS to not warn.
-  if (IgnoreCommaOperand(LHS))
+  if (IgnoreCommaOperand(LHS, Context))
     return;
 
   Diag(Loc, diag::warn_comma_operator);
   Diag(LHS->getBeginLoc(), diag::note_cast_to_void)
       << LHS->getSourceRange()
       << FixItHint::CreateInsertion(LHS->getBeginLoc(),
                                     LangOpts.CPlusPlus ? "static_cast<void>("
                                                        : "(void)(")

clang/test/SemaCXX/warn-comma-operator.cpp

void test_nested_fixits(void) {
   // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
   // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
   // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast<void>("
   // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")"
   // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast<void>("
   // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
 }
 
+
+void void_func();
+int int_func() { return 0; }
+
+void void_function_comma(){
+  void_func(), int_func(); // expected no -Wcomma because of the returning type `void` 
+  // Reported by https://github.com/llvm/llvm-project/issues/57151
+  // Descriptions about -Wcomma: https://reviews.llvm.org/D3976
+}
+
+typedef void Void;
+Void typedef_func();
+
+void whatever() {
+  // We don't get confused about type aliases.
+  typedef_func(), int_func();
+  // Even function pointers don't confuse us.
+  void (*fp)() = void_func;
+  fp(), int_func();
+}
+
 #ifdef __cplusplus
 class S2 {
 public:
   void advance();
 
   S2 operator++();
   S2 operator++(int);
   S2 operator--();

 template <typename T>
 void test_dependent_cast() {
   (void)42, 0;
   static_cast<void>(42), 0;
 
   (void)T{}, 0;
   static_cast<void>(T{}), 0;
 }
+
+namespace {
+
+// issue #57151
+
+struct S {
+  void mem() {}
+};
+
+void whatever() {
+  struct S s;
+  // Member function calls also work as expected.
+  s.mem(), int_func();
+  // As do lambda calls.
+  []() { return; }(), int_func();
+}
+
+} // namespace
+
 #endif  // ifdef __cplusplus