[clang-tidy] performance-unnecessary-copy-initialization: Disable check when variable and initializer have different replaced template param types.

[flx]

Summary

This can happen when a template with two parameter types is instantiated with a
single type. The fix would only be valid for this instantiation but fail for
others that rely on an implicit type conversion.

The test cases illustrate when the check should trigger and when not.

[ymandel]

Nice!

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

178–183

nit: maybe just return the condition (instead of using if(...) return true)? that is
return VarTmpType->... != ...

271

typo: conversion

[flx]

Addressed review feedback.

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

178–183

Done. Was left over from when there were debug statements there ;)

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

 
 using namespace ::clang::ast_matchers;
 using llvm::StringRef;
 using utils::decl_ref_expr::allDeclRefExprs;
 using utils::decl_ref_expr::isOnlyUsedAsConst;
 
 static constexpr StringRef ObjectArgId = "objectArg";
 static constexpr StringRef InitFunctionCallId = "initFunctionCall";
+static constexpr StringRef MethodDeclId = "methodDecl";
+static constexpr StringRef FunctionDeclId = "functionDecl";
 static constexpr StringRef OldVarDeclId = "oldVarDecl";
 
 void recordFixes(const VarDecl &Var, ASTContext &Context,
                  DiagnosticBuilder &Diagnostic) {
   Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context);
   if (!Var.getType().isLocalConstQualified()) {
     if (llvm::Optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
             Var, Context, DeclSpec::TQ::TQ_const))

 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
   // Match method call expressions where the `this` argument is only used as
   // const, this will be checked in `check()` part. This returned const
   // reference is highly likely to outlive the local const reference of the
   // variable being declared. The assumption is that the const reference being
   // 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(matchers::isReferenceToConst()))
+                 .bind(MethodDeclId)),
       on(declRefExpr(to(varDecl().bind(ObjectArgId)))));
 }
 
 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(matchers::isReferenceToConst()))
+                             .bind(FunctionDeclId)),
                   argumentCountIs(0), unless(callee(cxxMethodDecl())))
       .bind(InitFunctionCallId);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) {
   auto OldVarDeclRef =
       declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
   return expr(
static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
   return false;
 }
 
 bool isVariableUnused(const VarDecl &Var, const Stmt &BlockStmt,
                       ASTContext &Context) {
   return allDeclRefExprs(Var, BlockStmt, Context).empty();
 }
 
+const SubstTemplateTypeParmType *getSubstitutedType(const QualType &Type,
+                                                    ASTContext &Context) {
+  auto Matches = match(
+      qualType(anyOf(substTemplateTypeParmType().bind("subst"),
+                     hasDescendant(substTemplateTypeParmType().bind("subst")))),
+      Type, Context);
+  return selectFirst<SubstTemplateTypeParmType>("subst", Matches);
+}
+
+bool differentReplacedTemplateParams(const QualType &VarType,
+                                     const QualType &InitializerType,
+                                     ASTContext &Context) {
+  if (const SubstTemplateTypeParmType *VarTmplType =
+          getSubstitutedType(VarType, Context)) {
+    if (const SubstTemplateTypeParmType *InitializerTmplType =
+            getSubstitutedType(InitializerType, Context)) {
+      return VarTmplType->getReplacedParameter()
+                 ->desugar()
+                 .getCanonicalType() !=
+             InitializerTmplType->getReplacedParameter()
+                 ->desugar()
+                 .getCanonicalType();
+    }
+  }
+  return false;
+}
+
+QualType constructorArgumentType(const VarDecl *OldVar,
+                                 const BoundNodes &Nodes) {
+  if (OldVar) {
+    return OldVar->getType();
+  }
+  if (const auto *FuncDecl = Nodes.getNodeAs<FunctionDecl>(FunctionDeclId)) {
+    return FuncDecl->getReturnType();
+  }
+  const auto *MethodDecl = Nodes.getNodeAs<CXXMethodDecl>(MethodDeclId);
+  return MethodDecl->getReturnType();
+}
+
 } // namespace
 
 UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AllowedTypes(
           utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
 
void UnnecessaryCopyInitialization::check(
 
   // 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;
 
+  // Don't apply the check if the variable and its initializer have different
+  // replaced template parameter types. In this case the check triggers for a
+  // template instantiation where the substituted types are the same, but
+  // instantiations where the types differ and rely on implicit conversion would
+  // no longer compile if we switched to a reference.
+  if (differentReplacedTemplateParams(
+          NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
+          *Result.Context))
+    return;
+
   if (OldVar == nullptr) {
     handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
                                *Result.Context);
   } else {
     handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
                            *Result.Context);
   }
 }

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

struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
   const ExpensiveToCopyType *pointer() const;
   Iterator<ExpensiveToCopyType> begin() const;
   Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
+  template <typename A>
+  const A &templatedAccessor() const;
+  operator int() const; // Implicit conversion to int.
 };
 
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 
 struct WeirdCopyCtorType {
   WeirdCopyCtorType();
struct Pair {
     ExpensiveToCopyType second;
   };
 
   Pair P;
   const auto [C, D] = P;
   C.constMethod();
   D.constMethod();
 }
+
+template <typename A>
+const A &templatedReference();
+
+template <typename A, typename B>
+void negativeTemplateTypes() {
+  A Orig;
+  // Different replaced template type params do not trigger the check. In some
+  // template instantiation this might not be a copy but an implicit
+  // conversion, so converting this to a reference might not work.
+  B AmbiguousCopy = Orig;
+  // CHECK-NOT-FIXES: B AmbiguousCopy = Orig;
+
+  B NecessaryCopy = templatedReference<A>();
+  // CHECK-NOT-FIXES: B NecessaryCopy = templatedReference<A>();
+
+  B NecessaryCopy2 = Orig.template templatedAccessor<A>();
+
+  // Non-dependent types in template still trigger the check.
+  const auto UnnecessaryCopy = ExpensiveTypeReference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy' is copy-constructed
+  // CHECK-FIXES: const auto& UnnecessaryCopy = ExpensiveTypeReference();
+  UnnecessaryCopy.constMethod();
+}
+
+void instantiateNegativeTemplateTypes() {
+  negativeTemplateTypes<ExpensiveToCopyType, ExpensiveToCopyType>();
+  // This template instantiation would not compile if the `AmbiguousCopy` above was made a reference.
+  negativeTemplateTypes<ExpensiveToCopyType, int>();
+}
+
+template <typename A>
+void positiveSingleTemplateType() {
+  A Orig;
+  A SingleTmplParmTypeCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: local copy 'SingleTmplParmTypeCopy' of the variable 'Orig' is never modified
+  // CHECK-FIXES: const A& SingleTmplParmTypeCopy = Orig;
+  SingleTmplParmTypeCopy.constMethod();
+
+  A UnnecessaryCopy2 = templatedReference<A>();
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy2' is copy-constructed from a const reference
+  // CHECK-FIXES: const A& UnnecessaryCopy2 = templatedReference<A>();
+  UnnecessaryCopy2.constMethod();
+
+  A UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy3' is copy-constructed from a const reference
+  // CHECK-FIXES: const A& UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
+  UnnecessaryCopy3.constMethod();
+}
+
+void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType<ExpensiveToCopyType>(); }