[clang] [Clang] Only ignore special methods for unused private fields in BuildFieldReferenceExpr (PR #116965)

[Mészáros Gergely via cfe-commits]

https://github.com/Maetveis updated https://github.com/llvm/llvm-project/pull/116965

>From 90febd4a613fc2e76548da6912b32d50bc88a0ac Mon Sep 17 00:00:00 2001
From: Gergely Meszaros <gergely.meszaros at intel.com>
Date: Wed, 20 Nov 2024 12:55:59 +0000
Subject: [PATCH 1/3] [Clang] Only ignore special methods for unused private
 fields in BuildFieldReferenceExpr

The original code assumed that only special methods might be defined
as defaulted. Since C++20 comparison operators might be defaulted too,
and we *do* want to consider those as using the fields of the class.

Fixes: #116961
---
 clang/lib/Sema/SemaExprMember.cpp                | 12 ++++++++++--
 clang/test/SemaCXX/warn-unused-private-field.cpp | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index 434768b99d631e..85d5dfcb3db6de 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -1874,8 +1874,16 @@ Sema::BuildFieldReferenceExpr(Expr *BaseExpr, bool IsArrow,
           Context.getAttributedType(attr::NoDeref, MemberType, MemberType);
   }
 
-  auto *CurMethod = dyn_cast<CXXMethodDecl>(CurContext);
-  if (!(CurMethod && CurMethod->isDefaulted()))
+  auto isDefaultedSpecialMember = [this](const DeclContext *Ctx) {
+    auto *Method = dyn_cast<CXXMethodDecl>(CurContext);
+    if (!Method || !Method->isDefaulted())
+      return false;
+
+    return getDefaultedFunctionKind(Method).isSpecialMember();
+  };
+
+  // Implicit special members should not mark fields as used.
+  if (!isDefaultedSpecialMember(CurContext))
     UnusedPrivateFields.remove(Field);
 
   ExprResult Base = PerformObjectMemberConversion(BaseExpr, SS.getScopeRep(),
diff --git a/clang/test/SemaCXX/warn-unused-private-field.cpp b/clang/test/SemaCXX/warn-unused-private-field.cpp
index 1128eacc309d9f..9913dbaafb7a57 100644
--- a/clang/test/SemaCXX/warn-unused-private-field.cpp
+++ b/clang/test/SemaCXX/warn-unused-private-field.cpp
@@ -20,6 +20,20 @@ class SpaceShipDefaultCompare {
   int operator<=>(const SpaceShipDefaultCompare &) const = default;
 };
 
+class EqDefaultCompareOutOfClass {
+  int used;
+  bool operator==(const EqDefaultCompareOutOfClass &) const;
+};
+
+bool EqDefaultCompareOutOfClass::operator==(const EqDefaultCompareOutOfClass &) const = default;
+
+class FriendEqDefaultCompareOutOfClass {
+  int used;
+  friend bool operator==(const FriendEqDefaultCompareOutOfClass &, const FriendEqDefaultCompareOutOfClass &);
+};
+
+bool operator==(const FriendEqDefaultCompareOutOfClass &, const FriendEqDefaultCompareOutOfClass &) = default;
+
 #endif
 
 class NotFullyDefined {

>From acffecafadf20037211d73acee7ce9d715123cba Mon Sep 17 00:00:00 2001
From: Gergely <gergely.meszaros at intel.com>
Date: Mon, 25 Nov 2024 17:35:03 +0000
Subject: [PATCH 2/3] Add Release Notes and comments to test

---
 clang/docs/ReleaseNotes.rst                      | 2 ++
 clang/test/SemaCXX/warn-unused-private-field.cpp | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b9986434d09d24..367c1ae2803e0a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -585,6 +585,8 @@ Improvements to Clang's diagnostics
 - For an rvalue reference bound to a temporary struct with an integer member, Clang will detect constant integer overflow
   in the initializer for the integer member (#GH46755).
 
+- Fixed a false negative ``-Wunused-private-field`` diagnostic when a defaulted comparison operator is defined out of class (#GH116961).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/test/SemaCXX/warn-unused-private-field.cpp b/clang/test/SemaCXX/warn-unused-private-field.cpp
index 9913dbaafb7a57..047204e68617a1 100644
--- a/clang/test/SemaCXX/warn-unused-private-field.cpp
+++ b/clang/test/SemaCXX/warn-unused-private-field.cpp
@@ -21,14 +21,14 @@ class SpaceShipDefaultCompare {
 };
 
 class EqDefaultCompareOutOfClass {
-  int used;
+  int used; // no warning
   bool operator==(const EqDefaultCompareOutOfClass &) const;
 };
 
 bool EqDefaultCompareOutOfClass::operator==(const EqDefaultCompareOutOfClass &) const = default;
 
 class FriendEqDefaultCompareOutOfClass {
-  int used;
+  int used; // no warning
   friend bool operator==(const FriendEqDefaultCompareOutOfClass &, const FriendEqDefaultCompareOutOfClass &);
 };
 

>From 43cd9cd14d0def19a0613b7143d912ab4aa56e86 Mon Sep 17 00:00:00 2001
From: Gergely Meszaros <gergely.meszaros at intel.com>
Date: Tue, 26 Nov 2024 10:16:35 +0000
Subject: [PATCH 3/3] Expand on test comment why the new test case is required

---
 clang/test/SemaCXX/warn-unused-private-field.cpp | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/clang/test/SemaCXX/warn-unused-private-field.cpp b/clang/test/SemaCXX/warn-unused-private-field.cpp
index 047204e68617a1..bf104b1a76a656 100644
--- a/clang/test/SemaCXX/warn-unused-private-field.cpp
+++ b/clang/test/SemaCXX/warn-unused-private-field.cpp
@@ -21,14 +21,20 @@ class SpaceShipDefaultCompare {
 };
 
 class EqDefaultCompareOutOfClass {
-  int used; // no warning
+  int used; // no warning, the compiler generated AST for the comparison operator
+            // references the fields of the class, and this should be considered
+            // a use.
+            // This test case is needed because clang does not emit the body
+            // of the defaulted operator when it is defined in-class until it
+            // finds a call to it. `-Wunused-private-field` is suppressed in
+            // a different way in that case.
   bool operator==(const EqDefaultCompareOutOfClass &) const;
 };
 
 bool EqDefaultCompareOutOfClass::operator==(const EqDefaultCompareOutOfClass &) const = default;
 
 class FriendEqDefaultCompareOutOfClass {
-  int used; // no warning
+  int used; // no warning, same reasoning just tested via a friend declaration.
   friend bool operator==(const FriendEqDefaultCompareOutOfClass &, const FriendEqDefaultCompareOutOfClass &);
 };