[clang-tidy] avoid false positive when overload for bugprone-return-const-ref-from-parameter

[HerrCai0907]

Fixes: #90274

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes

Fixes: #90274

---

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

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp (+71-5)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp (+34)

diff --git a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
index cacba38b4a5aa..ecdcd3c2d2571 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
@@ -7,7 +7,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "ReturnConstRefFromParameterCheck.h"
-#include "../utils/Matchers.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
@@ -18,20 +17,87 @@ namespace clang::tidy::bugprone {
 void ReturnConstRefFromParameterCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       returnStmt(
-          hasReturnValue(declRefExpr(to(parmVarDecl(hasType(hasCanonicalType(
-              qualType(matchers::isReferenceToConst()).bind("type"))))))),
-          hasAncestor(functionDecl(hasReturnTypeLoc(
-              loc(qualType(hasCanonicalType(equalsBoundNode("type"))))))))
+          hasReturnValue(declRefExpr(
+              to(parmVarDecl(hasType(hasCanonicalType(
+                                 qualType(lValueReferenceType(pointee(
+                                              qualType(isConstQualified()))))
+                                     .bind("type"))))
+                     .bind("param")))),
+          hasAncestor(
+              functionDecl(hasReturnTypeLoc(loc(qualType(
+                               hasCanonicalType(equalsBoundNode("type"))))))
+                  .bind("func")))
           .bind("ret"),
       this);
 }
 
+static bool isSameTypeIgnoringConst(QualType A, QualType B) {
+  A = A.getCanonicalType();
+  B = B.getCanonicalType();
+  A.addConst();
+  B.addConst();
+  return A == B;
+}
+
+static bool isSameTypeIgnoringConstRef(QualType A, QualType B) {
+  return isSameTypeIgnoringConst(A.getCanonicalType().getNonReferenceType(),
+                                 B.getCanonicalType().getNonReferenceType());
+}
+
+static bool hasSameParameterTypes(const FunctionDecl &FD, const FunctionDecl &O,
+                                  const ParmVarDecl &PD) {
+  if (FD.getNumParams() != O.getNumParams())
+    return false;
+  for (unsigned I = 0, E = FD.getNumParams(); I < E; ++I) {
+    const ParmVarDecl *DPD = FD.getParamDecl(I);
+    const QualType OPT = O.getParamDecl(I)->getType();
+    if (DPD == &PD) {
+      if (!llvm::isa<RValueReferenceType>(OPT) ||
+          !isSameTypeIgnoringConstRef(DPD->getType(), OPT))
+        return false;
+    } else {
+      if (!isSameTypeIgnoringConst(DPD->getType(), OPT))
+        return false;
+    }
+  }
+  return true;
+}
+
+static const Decl *findRVRefOverload(const FunctionDecl &FD,
+                                     const ParmVarDecl &PD) {
+  // Actually it would be better to do lookup in caller site.
+  // But in most of cases, overloads of LVRef and RVRef will appear together.
+  // FIXME:
+  // 1. overload in anonymous namespace
+  // 2. forward reference
+  DeclContext::lookup_result LookupResult =
+      FD.getParent()->lookup(FD.getNameInfo().getName());
+  if (LookupResult.isSingleResult()) {
+    return nullptr;
+  }
+  for (const Decl *Overload : LookupResult) {
+    if (Overload == &FD)
+      continue;
+    Overload->dumpColor();
+    if (const auto *O = dyn_cast<FunctionDecl>(Overload))
+      if (hasSameParameterTypes(FD, *O, PD))
+        return O;
+  }
+  return nullptr;
+}
+
 void ReturnConstRefFromParameterCheck::check(
     const MatchFinder::MatchResult &Result) {
+  const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  const auto *PD = Result.Nodes.getNodeAs<ParmVarDecl>("param");
   const auto *R = Result.Nodes.getNodeAs<ReturnStmt>("ret");
   const SourceRange Range = R->getRetValue()->getSourceRange();
   if (Range.isInvalid())
     return;
+
+  if (findRVRefOverload(*FD, *PD) != nullptr)
+    return;
+
   diag(Range.getBegin(),
        "returning a constant reference parameter may cause use-after-free "
        "when the parameter is constructed from a temporary")
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
index ca41bdf74a107..d13c127da7c2a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
@@ -143,3 +143,37 @@ void instantiate(const int &param, const float &paramf, int &mut_param, float &m
 }
 
 } // namespace valid
+
+namespace overload {
+
+int const &overload_base(int const &a) { return a; }
+int const &overload_base(int &&a);
+
+int const &overload_ret_type(int const &a) { return a; }
+void overload_ret_type(int &&a);
+
+int const &overload_params1(int p1, int const &a) { return a; }
+int const & overload_params1(int p1, int &&a);
+
+int const &overload_params2(int p1, int const &a, int p2) { return a; }
+int const &overload_params2(int p1, int &&a, int p2);
+
+int const &overload_params3(T p1, int const &a, int p2) { return a; }
+int const &overload_params3(int p1, int &&a, T p2);
+
+int const &overload_params_const(int p1, int const &a, int const p2) { return a; }
+int const &overload_params_const(int const p1, int &&a, int p2);
+
+int const &overload_params_difference1(int p1, int const &a, int p2) { return a; }
+// CHECK-MESSAGES: :[[@LINE-1]]:79: warning: returning a constant reference parameter
+int const &overload_params_difference1(long p1, int &&a, int p2);
+
+int const &overload_params_difference2(int p1, int const &a, int p2) { return a; }
+// CHECK-MESSAGES: :[[@LINE-1]]:79: warning: returning a constant reference parameter
+int const &overload_params_difference2(int p1, int &&a, long p2);
+
+int const &overload_params_difference3(int p1, int const &a, int p2) { return a; }
+// CHECK-MESSAGES: :[[@LINE-1]]:79: warning: returning a constant reference parameter
+int const &overload_params_difference3(int p1, long &&a, int p2);
+
+} // namespace overload

[PiotrZSL]

Few nits.

[PiotrZSL]

Suggested change

	A = A.getCanonicalType();
	B = B.getCanonicalType();
	A.addConst();
	B.addConst();
	return A == B;
	return A.getCanonicalType().withConst() == B.getCanonicalType().withConst();

[PiotrZSL]

debug ?