[clang-tidy] Fix false positives about references in misc-const-correctness

[flovent]

It's not legal to cast const pointer type to it's non-const reference type implicitly, and will cause compile error.

And for explicit cast, it's legal but the pointer is mutable through this reference.

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: None (flovent)

Changes

It's not legal to cast const pointer type to it's non-const reference type implicitly, and will cause compile error.

And for explicit cast, it's legal but the pointer is mutable through this reference.

---

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

4 Files Affected:

• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-pointers.cpp (+15)
• (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+13-12)
• (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (+23)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9257dc6b98ba2..a6e4ed2edaebc 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -309,6 +309,10 @@ Changes in existing checks
 
   - ``for`` loops are supported.
 
+- Improved :doc:`misc-const-correctness
+  <clang-tidy/checks/misc/misc-const-correctness>` check to avoid false
+  positives when pointers is tranferred to non-const references.
+
 - Improved :doc:`misc-header-include-cycle
   <clang-tidy/checks/misc/header-include-cycle>` check performance.
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-pointers.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-pointers.cpp
index 2ef47266b02b0..bcd946e7404c1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-pointers.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-pointers.cpp
@@ -48,3 +48,18 @@ void ignore_const_alias() {
   p_local0 = &a[1];
 }
 
+void takeNonConstRef(int *&r);
+
+void ignoreNonConstRefOps() {
+  // init with non-const ref
+  int* p0 {nullptr};
+  int*& r1 = p0;
+  
+  // non-const ref param
+  int* p1 {nullptr};
+  takeNonConstRef(p1);
+
+  // cast
+  int* p2 {nullptr};
+  int*& r2 = (int*&)p2;
+}
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 3fcd3481c2d6b..1e376da1be83d 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -755,22 +755,23 @@ ExprMutationAnalyzer::Analyzer::findPointeeMemberMutation(const Expr *Exp) {
 
 const Stmt *
 ExprMutationAnalyzer::Analyzer::findPointeeToNonConst(const Expr *Exp) {
-  const auto NonConstPointerOrDependentType =
-      type(anyOf(nonConstPointerType(), isDependentType()));
+  const auto NonConstPointerOrNonConstRefOrDependentType = type(
+      anyOf(nonConstPointerType(), nonConstReferenceType(), isDependentType()));
 
   // assign
   const auto InitToNonConst =
-      varDecl(hasType(NonConstPointerOrDependentType),
+      varDecl(hasType(NonConstPointerOrNonConstRefOrDependentType),
               hasInitializer(expr(canResolveToExprPointee(Exp)).bind("stmt")));
-  const auto AssignToNonConst =
-      binaryOperation(hasOperatorName("="),
-                      hasLHS(expr(hasType(NonConstPointerOrDependentType))),
-                      hasRHS(canResolveToExprPointee(Exp)));
+  const auto AssignToNonConst = binaryOperation(
+      hasOperatorName("="),
+      hasLHS(expr(hasType(NonConstPointerOrNonConstRefOrDependentType))),
+      hasRHS(canResolveToExprPointee(Exp)));
   // arguments like
   const auto ArgOfInstantiationDependent = allOf(
       hasAnyArgument(canResolveToExprPointee(Exp)), isInstantiationDependent());
-  const auto ArgOfNonConstParameter = forEachArgumentWithParamType(
-      canResolveToExprPointee(Exp), NonConstPointerOrDependentType);
+  const auto ArgOfNonConstParameter =
+      forEachArgumentWithParamType(canResolveToExprPointee(Exp),
+                                   NonConstPointerOrNonConstRefOrDependentType);
   const auto CallLikeMatcher =
       anyOf(ArgOfNonConstParameter, ArgOfInstantiationDependent);
   const auto PassAsNonConstArg =
@@ -779,9 +780,9 @@ ExprMutationAnalyzer::Analyzer::findPointeeToNonConst(const Expr *Exp) {
                  parenListExpr(has(canResolveToExprPointee(Exp))),
                  initListExpr(hasAnyInit(canResolveToExprPointee(Exp)))));
   // cast
-  const auto CastToNonConst =
-      explicitCastExpr(hasSourceExpression(canResolveToExprPointee(Exp)),
-                       hasDestinationType(NonConstPointerOrDependentType));
+  const auto CastToNonConst = explicitCastExpr(
+      hasSourceExpression(canResolveToExprPointee(Exp)),
+      hasDestinationType(NonConstPointerOrNonConstRefOrDependentType));
 
   // capture
   // FIXME: false positive if the pointee does not change in lambda
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index 4e97174c17d95..95f8ae266ae10 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1749,6 +1749,13 @@ TEST(ExprMutationAnalyzerTest, PointeeMutatedByInitToNonConst) {
         match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
     EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
   }
+  {
+    const std::string Code = "void f() { int* x = nullptr; int*& b = x; }";
+    auto AST = buildASTFromCodeWithArgs(Code, {});
+    auto Results =
+        match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+    EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+  }
 }
 
 TEST(ExprMutationAnalyzerTest, PointeeMutatedByAssignToNonConst) {
@@ -1786,6 +1793,14 @@ TEST(ExprMutationAnalyzerTest, PointeeMutatedByPassAsArgument) {
         match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
     EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
   }
+  {
+    const std::string Code =
+        "void b(int *&); void f() { int* x = nullptr; b(x); }";
+    auto AST = buildASTFromCodeWithArgs(Code, {});
+    auto Results =
+        match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+    EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+  }
 }
 
 TEST(ExprMutationAnalyzerTest, PointeeMutatedByPassAsArgumentInConstruct) {
@@ -1884,6 +1899,14 @@ TEST(ExprMutationAnalyzerTest, PointeeMutatedByExplicitCastToNonConst) {
         match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
     EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
   }
+  {
+    const std::string Code =
+        "void f() { int* x = nullptr; static_cast<int*&>(x); }";
+    auto AST = buildASTFromCodeWithArgs(Code, {"-Wno-everything"});
+    auto Results =
+        match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+    EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+  }
 }
 
 TEST(ExprMutationAnalyzerTest, PointeeMutatedByConstCastToNonConst) {

[localspook]

Just a nit about the release notes:

[localspook]

Suggested change

	positives when pointers is tranferred to non-const references.
	positives when pointers to non-const are bound to references.

Since the false positive happens whether the reference itself is const or not.

[flovent]

For the second function in link:

void const_ref() {
  int* p1 {nullptr};
  int* const& r1 = p1;
}

It's not fixed by this patch actually, because it's a reference points to const (int* const), and it's not legal to write a const reference version:

   int*  p1 {nullptr};
   const int* & r1 = p1; // error: binding reference of type 'const int *' to value of type 'int *' not permitted due to incompatible qualifiers

And canResolveToExprPointee don't handle NoOp right now, for the AST :

VarDecl <col:4, col:22> col:17 r1 'int *const &' cinit
  `-ImplicitCastExpr <col:22> 'int *const' lvalue <NoOp>
    `-DeclRefExpr <col:22> 'int *' lvalue Var 0x38dc2e98 'p1' 'int *'

So it might need more work to erase this FP.

[HerrCai0907]

LGTM