[clang-tidy] improve robustness of the member initializer detection in modernize-use-default-member-init

[HerrCai0907]

Fixed #156295
Fixed #122480
Fixed #160394
The original ast matcher based member initializer detection is bug prone. In this PR we introduce a new recusively detection which can be easier extended to detect whether we can move the initializer to member

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Congcong Cai (HerrCai0907)

Changes

Fixed #156295, #122480
The original ast matcher based member initializer detection is bug prone. In this PR we introduce a new recusively detection which can be easier extended to detect whether we can move the initializer to member

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp (+39-24)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp (+16)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
index d920af7fc477b..e06154578938b 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -8,17 +8,52 @@
 
 #include "UseDefaultMemberInitCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::modernize {
 
+static bool isExprAllowedInMemberInit(const Expr *E) {
+  if (E == nullptr)
+    return false;
+  if (isa<IntegerLiteral, FloatingLiteral, CXXBoolLiteralExpr,
+          CXXNullPtrLiteralExpr, CharacterLiteral, StringLiteral>(E))
+    return true;
+  if (isa<ImplicitValueInitExpr>(E))
+    return true;
+  if (const auto *PE = dyn_cast<ParenExpr>(E))
+    return isExprAllowedInMemberInit(PE->getSubExpr());
+  if (const auto *UO = dyn_cast<UnaryOperator>(E); UO && UO->isArithmeticOp())
+    return isExprAllowedInMemberInit(UO->getSubExpr());
+  if (const auto *BO = dyn_cast<BinaryOperator>(E))
+    return isExprAllowedInMemberInit(BO->getLHS()) &&
+           isExprAllowedInMemberInit(BO->getRHS());
+  if (const auto *CE = dyn_cast<CastExpr>(E))
+    return isExprAllowedInMemberInit(CE->getSubExpr());
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+    if (const ValueDecl *D = DRE->getDecl()) {
+      if (isa<EnumConstantDecl>(D))
+        return true;
+      if (const auto *VD = dyn_cast<VarDecl>(D))
+        return VD->isConstexpr() || VD->getStorageClass() == SC_Static;
+    }
+    return false;
+  }
+  return false;
+}
+
 namespace {
+
 AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) {
   return Node.getNumInits() == N;
 }
+
+AST_MATCHER(Expr, allowedInitExpr) { return isExprAllowedInMemberInit(&Node); }
+
 } // namespace
 
 static StringRef getValueOfValueInit(const QualType InitType) {
@@ -206,30 +241,10 @@ void UseDefaultMemberInitCheck::storeOptions(
 }
 
 void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) {
-  auto NumericLiteral = anyOf(integerLiteral(), floatLiteral());
-  auto UnaryNumericLiteral = unaryOperator(hasAnyOperatorName("+", "-"),
-                                           hasUnaryOperand(NumericLiteral));
-
-  auto ConstExprRef = varDecl(anyOf(isConstexpr(), isStaticStorageClass()));
-  auto ImmutableRef =
-      declRefExpr(to(decl(anyOf(enumConstantDecl(), ConstExprRef))));
-
-  auto BinaryNumericExpr = binaryOperator(
-      hasOperands(anyOf(NumericLiteral, ImmutableRef, binaryOperator()),
-                  anyOf(NumericLiteral, ImmutableRef, binaryOperator())));
-
-  auto InitBase =
-      anyOf(stringLiteral(), characterLiteral(), NumericLiteral,
-            UnaryNumericLiteral, cxxBoolLiteral(), cxxNullPtrLiteralExpr(),
-            implicitValueInitExpr(), ImmutableRef, BinaryNumericExpr);
-
-  auto ExplicitCastExpr = castExpr(hasSourceExpression(InitBase));
-  auto InitMatcher = anyOf(InitBase, ExplicitCastExpr);
-
-  auto Init =
-      anyOf(initListExpr(anyOf(allOf(initCountIs(1), hasInit(0, InitMatcher)),
-                               initCountIs(0), hasType(arrayType()))),
-            InitBase, ExplicitCastExpr);
+  auto Init = anyOf(
+      initListExpr(anyOf(allOf(initCountIs(1), hasInit(0, allowedInitExpr())),
+                         initCountIs(0), hasType(arrayType()))),
+      allowedInitExpr());
 
   Finder->addMatcher(
       cxxConstructorDecl(forEachConstructorInitializer(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a4652a7a54858..3e332a62abdc8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -282,6 +282,10 @@ Changes in existing checks
   uses of non-standard ``enable_if`` with a signature different from
   ``std::enable_if`` (such as ``boost::enable_if``).
 
+- Improved :doc:`modernize-use-default-member-init
+  <clang-tidy/checks/modernize/use-default-member-init>` check to
+  improve the robustness of the member initializer detection.
+
 - Improved :doc:`modernize-use-designated-initializers
   <clang-tidy/checks/modernize/use-designated-initializers>` check to
   suggest using designated initializers for aliased aggregate types.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
index 015216c4a9d59..4d8de6a97925b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
@@ -596,3 +596,19 @@ class DefaultMemberInitWithArithmetic {
 };
 
 } //namespace PR122480
+
+namespace GH156295 {
+
+class NotFix {
+  NotFix(int v) : x(0 + 0 + (0 * 0 * (((((((v)))) - 20))) + 10)) {}
+  int x;
+};
+
+class ShouldFix {
+  ShouldFix(int v) : x(0 + 0 + (0 * 0 * (((((((1)))) - 20))) + 10)) {}
+  int x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'x' [modernize-use-default-member-init]
+  // CHECK-FIXES: int x{0 + 0 + (0 * 0 * (((((((1)))) - 20))) + 10)};
+};
+
+} // namespace GH156295

[zwuis]

Fixed #156295, #122480

Linked issue	Syntax	Example
Multiple issues	Use full syntax for each issue	Resolves #10, resolves #123, resolves octo-org/octo-repo#100

https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[RiverDave]

Thanks for doing this! I've had thought of working on this on a while but have been caught with school.
Overall good

[vbvictor]

Could you add also test from #160394?

[vbvictor]

LGTM, I'd vote to backport it to 21st release because we are already having multiple issues.