[clang] [clang-tools-extra] [clang-tidy] fix fp when modifying variant by ``operator[]`` with template in parameters (PR #128407)

[Congcong Cai via cfe-commits]

https://github.com/HerrCai0907 created https://github.com/llvm/llvm-project/pull/128407

`ArraySubscriptExpr` can switch base and idx. For dependent array subscript access, we should check both base and idx conservatively.

>From ff265c9f01d68b8657d217ba4ea62b77a5775bb5 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Sun, 23 Feb 2025 15:24:52 +0800
Subject: [PATCH 1/2] wip

---
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   | 18 ++++++++++++++---
 .../Analysis/ExprMutationAnalyzerTest.cpp     | 20 +++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 8944343484e58..823d7543f085f 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -80,6 +80,17 @@ static bool canExprResolveTo(const Expr *Source, const Expr *Target) {
 
 namespace {
 
+// `ArraySubscriptExpr` can switch base and idx, e.g. `a[4]` is the same as
+// `4[a]`. When type is dependent, we conservatively assume both sides are base.
+AST_MATCHER_P(ArraySubscriptExpr, hasBaseConservative,
+              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (Node.isTypeDependent()) {
+    return InnerMatcher.matches(*Node.getLHS(), Finder, Builder) ||
+           InnerMatcher.matches(*Node.getRHS(), Finder, Builder);
+  }
+  return InnerMatcher.matches(*Node.getBase(), Finder, Builder);
+}
+
 AST_MATCHER(Type, isDependentType) { return Node.isDependentType(); }
 
 AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) {
@@ -513,8 +524,8 @@ ExprMutationAnalyzer::Analyzer::findArrayElementMutation(const Expr *Exp) {
   // Check whether any element of an array is mutated.
   const auto SubscriptExprs = match(
       findAll(arraySubscriptExpr(
-                  anyOf(hasBase(canResolveToExpr(Exp)),
-                        hasBase(implicitCastExpr(allOf(
+                  anyOf(hasBaseConservative(canResolveToExpr(Exp)),
+                        hasBaseConservative(implicitCastExpr(allOf(
                             hasCastKind(CK_ArrayToPointerDecay),
                             hasSourceExpression(canResolveToExpr(Exp)))))))
                   .bind(NodeID<Expr>::value)),
@@ -716,7 +727,8 @@ ExprMutationAnalyzer::Analyzer::findPointeeValueMutation(const Expr *Exp) {
                    unaryOperator(hasOperatorName("*"),
                                  hasUnaryOperand(canResolveToExprPointee(Exp))),
                    // deref by []
-                   arraySubscriptExpr(hasBase(canResolveToExprPointee(Exp)))))
+                   arraySubscriptExpr(
+                       hasBaseConservative(canResolveToExprPointee(Exp)))))
               .bind(NodeID<Expr>::value))),
       Stm, Context);
   return findExprMutation(Matches);
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index cc277d56b37a2..d237aaa9616f8 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -870,6 +870,26 @@ TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
   EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
 }
 
+TEST(ExprMutationAnalyzerTest, T1) {
+  const auto AST = buildASTFromCodeWithArgs(
+      R"(
+      
+template <class T, class V> class unordered_map {
+public:
+  V &operator[](T t);
+};
+
+template <typename T> void func() {
+  unordered_map<int, int> x;
+  x[T{}] = 3;
+}
+      )",
+      {"-fno-delayed-template-parsing"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
 // section: special case: all created references are non-mutating themself
 //          and therefore all become 'const'/the value is not modified!
 

>From 6f464d8420426731ff9c2b1165c28224ac872dd5 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Sun, 23 Feb 2025 15:29:00 +0800
Subject: [PATCH 2/2] add test

---
 clang-tools-extra/docs/ReleaseNotes.rst         |  4 ++++
 .../checkers/misc/const-correctness-values.cpp  |  8 ++++++++
 .../Analysis/ExprMutationAnalyzerTest.cpp       | 17 +++++------------
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b8fe22242417..dc367b6b476f5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
   additional C++ member functions to match.
 
+- Improved :doc:`misc-const-correctness
+  <clang-tidy/checks/const-correctness>` check by fixing false positives when
+  modify variant by ``operator[]`` with template in parameters.
+
 - Improved :doc:`misc-redundant-expression
   <clang-tidy/checks/misc/redundant-expression>` check by providing additional
   examples and fixing some macro related false positives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
index 0d1ff0db58371..e598cf9e64373 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -998,3 +998,11 @@ void member_pointer_const(Value &x, PointerToConstMemberFunction m) {
   // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'member_pointer_tmp' of type 'Value &' can be declared 'const'
   (member_pointer_tmp.*m)();
 }
+
+namespace gh127776_false_positive {
+template <class T> struct vector { T &operator[](int t); };
+template <typename T> void f() {
+  vector<int> x;
+  x[T{}] = 3;
+}
+} // namespace gh127776_false_positive
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index d237aaa9616f8..720999207083d 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -872,18 +872,11 @@ TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
 
 TEST(ExprMutationAnalyzerTest, T1) {
   const auto AST = buildASTFromCodeWithArgs(
-      R"(
-      
-template <class T, class V> class unordered_map {
-public:
-  V &operator[](T t);
-};
-
-template <typename T> void func() {
-  unordered_map<int, int> x;
-  x[T{}] = 3;
-}
-      )",
+      "template <class T> struct vector { T &operator[](int t); };"
+      "template <typename T> void func() {"
+      "  vector<int> x;"
+      "  x[T{}] = 3;"
+      "}",
       {"-fno-delayed-template-parsing"});
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());