From fc1b24d7360f59c8b89f1b6290fe849a6207dc44 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet@google.com>
Date: Thu, 28 Oct 2021 16:48:53 +0200
Subject: [PATCH] [clang-tidy]performance-unnecessary-copy-initialization: fix
 false negative

We're missing all cases where the return value is a type alias.

Unfortunately, this includes things we care about, such as
`std::vector<T>::operator[]` (return value is `const_reference`,
not `const T&`).

Match the canonical type instead.

Differential Revision: https://reviews.llvm.org/D112722
---
 .../performance/UnnecessaryCopyInitialization.cpp     |  6 ++++--
 .../performance-unnecessary-copy-initialization.cpp   | 11 +++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 6f9495fd1e66c2..2cdd7827ee42a2 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -84,7 +84,8 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
   // returned either points to a global static variable or to a member of the
   // called object.
   return cxxMemberCallExpr(
-      callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))
+      callee(cxxMethodDecl(
+                 returns(hasCanonicalType(matchers::isReferenceToConst())))
                  .bind(MethodDeclId)),
       on(declRefExpr(to(
           varDecl(
@@ -97,7 +98,8 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
   // Only allow initialization of a const reference from a free function if it
   // has no arguments. Otherwise it could return an alias to one of its
   // arguments and the arguments need to be checked for const use as well.
-  return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))
+  return callExpr(callee(functionDecl(returns(hasCanonicalType(
+                                          matchers::isReferenceToConst())))
                              .bind(FunctionDeclId)),
                   argumentCountIs(0), unless(callee(cxxMethodDecl())))
       .bind(InitFunctionCallId);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
index 68a010c4d953c9..4c469c966860b0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -12,6 +12,8 @@ struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
+  using ConstRef = const ExpensiveToCopyType &;
+  ConstRef referenceWithAlias() const;
   const ExpensiveToCopyType *pointer() const;
   Iterator<ExpensiveToCopyType> begin() const;
   Iterator<ExpensiveToCopyType> end() const;
@@ -206,6 +208,15 @@ void positiveNonConstVarInCodeBlock(const ExpensiveToCopyType &Obj) {
   }
 }
 
+void positiveNonConstVarInCodeBlockWithAlias(const ExpensiveToCopyType &Obj) {
+  {
+    const ExpensiveToCopyType Assigned = Obj.referenceWithAlias();
+    // CHECK-MESSAGES: [[@LINE-1]]:31: warning: the const qualified variable
+    // CHECK-FIXES: const ExpensiveToCopyType& Assigned = Obj.referenceWithAlias();
+    useAsConstReference(Assigned);
+  }
+}
+
 void negativeNonConstVarWithNonConstUse(const ExpensiveToCopyType &Obj) {
   {
     auto NonConstInvoked = Obj.reference();