Restrict UnnecessaryCopyInitialization check to variables initialized from free functions without arguments

This restriction avoids cases where an alias is returned to an argument and
which could lead to to a false positive change.

Author: Felix Berger

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

@@ -54,7 +54,8 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
                         on(declRefExpr(to(varDecl().bind("objectArg")))));
   auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
-               unless(callee(cxxMethodDecl())));
+               unless(callee(cxxMethodDecl())))
+          .bind("initFunctionCall");
 
   auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
@@ -96,6 +97,8 @@ void UnnecessaryCopyInitialization::check(
   const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
   const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
+  const auto *InitFunctionCall =
+      Result.Nodes.getNodeAs<CallExpr>("initFunctionCall");
 
   TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs);
 
@@ -113,6 +116,11 @@ void UnnecessaryCopyInitialization::check(
       return;
 
   if (OldVar == nullptr) {
+    // 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.
+    if (InitFunctionCall != nullptr && InitFunctionCall->getNumArgs() > 0)
+      return;
     handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
                                *Result.Context);
   } else {

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

@@ -23,6 +23,9 @@ struct WeirdCopyCtorType {
 ExpensiveToCopyType global_expensive_to_copy_type;
 
 const ExpensiveToCopyType &ExpensiveTypeReference();
+const ExpensiveToCopyType &freeFunctionWithArg(const ExpensiveToCopyType &);
+const ExpensiveToCopyType &freeFunctionWithDefaultArg(
+    const ExpensiveToCopyType *arg = nullptr);
 const TrivialToCopyType &TrivialTypeReference();
 
 void mutate(ExpensiveToCopyType &);
@@ -387,3 +390,18 @@ void implicitVarFalsePositive() {
   for (const Element &E : Container()) {
   }
 }
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromFreeFunctionWithArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig);
+}
+
+void negativeInitializedFromFreeFunctionWithDefaultArg() {
+  const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg();
+}
+
+void negativeInitialzedFromFreeFunctionWithNonDefaultArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig);
+}