[clang-tidy] fix crash due to assumed callee in min-max-use-initializer-list

[5chmidti]

Previously, the call to findArgs for a CallExpr inside of a min or
max call would call findArgs before checking if the argument is a
call to min or max, which is what findArgs is expecting.
The fix moves the name checking before the call to findArgs, such that
only a min or max function call is used as an argument.

Fixes #91982
Fixes #92249

[llvmbot]

@llvm/pr-subscribers-clang-tidy

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

Author: Julian Schmidt (5chmidti)

Changes

Previously, the call to findArgs for a CallExpr inside of a min or
max call would call findArgs before checking if the argument is a
call to min or max, which is what findArgs is expecting.
The fix moves the name checking before the call to findArgs, such that
only a min or max function call is used as an argument.

Fixes #91982

---

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

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp (+5-5)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp (+21)

diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
index 45f7700463d57..418699ffbc4d1 100644
--- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
@@ -129,17 +129,17 @@ generateReplacements(const MatchFinder::MatchResult &Match,
       continue;
     }
 
+    // if the nested call is not the same as the top call
+    if (InnerCall->getDirectCallee()->getQualifiedNameAsString() !=
+        TopCall->getDirectCallee()->getQualifiedNameAsString())
+      continue;
+
     const FindArgsResult InnerResult = findArgs(InnerCall);
 
     // if the nested call doesn't have arguments skip it
     if (!InnerResult.First || !InnerResult.Last)
       continue;
 
-    // if the nested call is not the same as the top call
-    if (InnerCall->getDirectCallee()->getQualifiedNameAsString() !=
-        TopCall->getDirectCallee()->getQualifiedNameAsString())
-      continue;
-
     // if the nested call doesn't have the same compare function
     if ((Result.Compare || InnerResult.Compare) &&
         !utils::areStatementsIdentical(Result.Compare, InnerResult.Compare,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
index 51ab9bda975f1..1f2dad2b933ca 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
@@ -300,6 +300,27 @@ B maxTT2 = std::max(B(), std::max(B(), B()));
 B maxTT3 = std::max(B(), std::max(B(), B()), [](const B &lhs, const B &rhs) { return lhs.a[0] < rhs.a[0]; });
 // CHECK-FIXES: B maxTT3 = std::max(B(), std::max(B(), B()), [](const B &lhs, const B &rhs) { return lhs.a[0] < rhs.a[0]; });
 
+struct GH91982 {
+  int fun0Args();
+  int fun1Arg(int a);
+  int fun2Args(int a, int b);
+  int fun3Args(int a, int b, int c);
+  int fun4Args(int a, int b, int c, int d);
+
+  int foo() {
+    return std::max(
+        fun0Args(),
+        std::max(fun1Arg(0),
+                 std::max(fun2Args(0, 1),
+                          std::max(fun3Args(0, 1, 2), fun4Args(0, 1, 2, 3)))));
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: do not use nested 'std::max' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
+// CHECK-FIXES: return std::max(
+// CHECK-FIXES-NEXT: {fun0Args(),
+// CHECK-FIXES-NEXT: fun1Arg(0),
+// CHECK-FIXES-NEXT: fun2Args(0, 1),
+// CHECK-FIXES-NEXT: fun3Args(0, 1, 2), fun4Args(0, 1, 2, 3)});
+  }
+};
 
 } // namespace
 

[5chmidti]

CC @sopyb I was already looking into it ^^

[5chmidti]

I have a few small changes to findArgs that I have not committed, which would change the if's to check the exact number of args that are expected, instead of < 3 and else (+change from iterators to getArg(n)). Should I add them as well?

[PiotrZSL]

LGTM

[sopyb]

Glad someone was around to make a fix 🙂