[clang-tools-extra] [clang-tidy] fix match for binaryOperator in ExprMutationAnalyzer for misc-const-correctness (PR #70559)

[Julian Schmidt via cfe-commits]

https://github.com/5chmidti created https://github.com/llvm/llvm-project/pull/70559

The `ExprMutationAnalyzer`s matcher of `binaryOperator`s
that contained the variable expr, were previously narrowing the
variable to be type dependent, when the `binaryOperator` should
have been narrowed as dependent.
The variable we are trying to find mutations for does
not need to be the dependent type, the other operand of
the `binaryOperator` could be dependent.

Fixes #57297


>From b29eb35fe8597ceefc4c615817174181a16f3c4c Mon Sep 17 00:00:00 2001
From: Julian Schmidt <44101708+5chmidti at users.noreply.github.com>
Date: Sat, 28 Oct 2023 18:08:51 +0200
Subject: [PATCH] [clang-tidy] fix match for binaryOperator in
 ExprMutationAnalyzer for misc-const-correctness

The `ExprMutationAnalyzer`s matcher of `binaryOperator`s
that contained the variable expr, were previously narrowing the
variable to be type dependent, when the `binaryOperator` should
have been narrowed as dependent.
The variable we are trying to find mutations for does
not need to be the dependent type, the other operand of
the `binaryOperator` could be dependent.

Fixes #57297
---
 clang-tools-extra/docs/ReleaseNotes.rst               |  5 +++++
 .../checkers/misc/const-correctness-templates.cpp     | 11 +++++++++++
 clang/docs/ReleaseNotes.rst                           |  3 +++
 clang/lib/Analysis/ExprMutationAnalyzer.cpp           |  6 +++---
 clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp | 11 +++++++++++
 5 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5ce38ab48bf295f..bb75c9a3ce08125 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -308,6 +308,11 @@ Changes in existing checks
   <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when
   using pointer to member function.
 
+- Improved :doc:`misc-const-correctness
+  <clang-tidy/checks/misc/const-correctness>` check to not warn on uses in
+  type-dependent binary operators, when the variable that is being
+  looked at, is not the dependent operand.
+
 - Improved :doc:`misc-include-cleaner
   <clang-tidy/checks/misc/include-cleaner>` check by adding option
   `DeduplicateFindings` to output one finding per symbol occurrence, avoid
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
index 7b5ccabdd6ef611..415bb06020b9dc3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
@@ -20,3 +20,14 @@ void instantiate_template_cases() {
   type_dependent_variables<int>();
   type_dependent_variables<float>();
 }
+
+namespace gh57297{
+struct Stream {};
+// Explicitly not declaring a (templated) stream operator
+// so the `<<` is a `binaryOperator` with a dependent type.
+template <typename T> void f() {
+  T t;
+  Stream stream;
+  stream << t;
+}
+} // namespace gh57297
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2595737e8b3b143..fb9afc0646ac8cb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -683,6 +683,9 @@ Bug Fixes to AST Handling
 - Fixed a bug where RecursiveASTVisitor fails to visit the
   initializer of a bitfield.
   `Issue 64916 <https://github.com/llvm/llvm-project/issues/64916>`_
+- Fixed a bug where ``ExprMutationAnalyzer`` did not find a potential mutation
+  for uses in type-dependent binary operators, when the variable that is being
+  looked at, is not the dependent operand.
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index f2e1beb025423cf..624a643cc60e4ba 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -296,9 +296,9 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       // resolved and modelled as `binaryOperator` on a dependent type.
       // Such instances are considered a modification, because they can modify
       // in different instantiations of the template.
-      binaryOperator(hasEitherOperand(
-          allOf(ignoringImpCasts(canResolveToExpr(equalsNode(Exp))),
-                isTypeDependent()))),
+      binaryOperator(
+          hasEitherOperand(ignoringImpCasts(canResolveToExpr(equalsNode(Exp)))),
+          isTypeDependent()),
       // Within class templates and member functions the member expression might
       // not be resolved. In that case, the `callExpr` is considered to be a
       // modification.
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index c0a398394093c48..cfa3c535ce35292 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -343,6 +343,17 @@ TEST(ExprMutationAnalyzerTest, UnresolvedOperator) {
   EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
+TEST(ExprMutationAnalyzerTest, DependentOperatorWithNonDependentOperand) {
+  // gh57297
+  // Explicitly not declaring a (templated) stream operator
+  // so the `<<` is a `binaryOperator` with a dependent type.
+  const auto AST = buildASTFromCode("struct Stream {}; template <typename T> "
+                                    "void f() { T t; Stream x; x << t;}");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x << t"));
+}
+
 // Section: expression as call argument
 
 TEST(ExprMutationAnalyzerTest, ByValueArgument) {