[clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from free functions without arguments

[flx]

Summary

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

struct S {
  S(const S&);
  void modify();
};

const S& alias(const S& a) {
  return a;
}

void f(S a) {
  const S c = alias(a);
  a.modify();
}

Taking a const reference of the return value of alias would not be safe since a is modified.

A more involved alternative approach would be to inspect each argument passed to these functions for its constness, but this can get complex quickly.

[flx]

The motivating false positive was the free functions std::min. To not limit the usefulness of the check I'll only disable arguments for free functions for now. Updated change coming.

[aaron.ballman]

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

126

I'm not certain I agree with this change, but I'm also not certain I disagree with it. There are more ways to alias than just default arguments (global variables, etc) and the presence of a default argument is not really an indication about its return value in this case. e.g.,

const ExpensiveToCopyType &freeFunctionWithDefaultArg(int i = 12);

(I don't see a reason why = 12 should impact whether use of this function diagnoses or not.)

Perhaps if this was tightened up a bit to care about the type of the default argument it might make sense, but even that gets tricky when you consider base classes, template types, etc. and doesn't cover all of the aliasing cases anyway. Is there an important use case for this behavior?

[flx]

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

126

This was mostly following the logic already applied to the copy constructor where the existing code also allowed default arguments. The idea is to not disallow functions with default arguments that are not overridden.

I'll switch to not allowing any arguments. This will just exclude a few more cases, but better to be safe, until we might be able to verify the constness of all function arguments as well.

[flx]

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

126

This is done now.

[aaron.ballman]

LGTM with some minor nits.

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

122

There's no real need for this change now, so I'd roll back to the previous form (then I don't have to wonder what the 1 means at the call site).

145

Some spurious whitespace changes that can also be rolled back.

[flx]

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

[aaron.ballman]

In D87455#2271582, @flx wrote:

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

I've commit on your behalf in 98e07b5596c8692c43770bc4e21a2b19467e35f7, thank you for the patch!

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

   }
 }
 
+template <typename Call>
+bool hasNonDefaultArgs(const Call &CallExpr, unsigned int StartIndex = 0) {
+  for (unsigned int I = StartIndex; I < CallExpr.getNumArgs(); ++I)
+    if (!CallExpr.getArg(I)->isDefaultArgument())
+      return true;
+  return false;
+}
+
 } // namespace
 
 using namespace ::clang::ast_matchers;

                         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(

   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);
 

   // A constructor that looks like T(const T& t, bool arg = false) counts as a
   // copy only when it is called with default arguments for the arguments after
   // the first.
-  for (unsigned int i = 1; i < CtorCall->getNumArgs(); ++i)
-    if (!CtorCall->getArg(i)->isDefaultArgument())
-      return;
+  if (hasNonDefaultArgs(*CtorCall, 1))
+    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 {

   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
     return;
+
   if (ObjectArg != nullptr &&
       !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
     return;
-
   auto Diagnostic =
       diag(Var.getLocation(),
            IsConstQualified ? "the const qualified variable %0 is "

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

 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 &);

   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);
+}