[clang-tidy] performance-unnecessary-copy-initialization: Prevent false positives when dependent variable is modified.

[flx]

Summary

Extend the check to not only look at the variable the unnecessarily copied
variable is initialized from, but also ensure that any variable the old variable
references is not modified.

Extend DeclRefExprUtils to also count references and pointers to const assigned
from the DeclRef we check for const usages.

[flx]

Fix formatting and comments.

[flx]

Fixed clang tidy warnings.

[flx]

Could someone take a first pass at this change? It would be great progress on this as this is the last currently known case that generates false positives.

[aaron.ballman]

I have some quick drive-by comments but still have to think about the test cases a bit more.

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

74

typo: the the

77

typo: it its

83

You should mark the function as being static.

86

Elide braces around single-line if statements. For the cases involving comments, I'd recommend hoisting the comments above the if when returning a constant.

90

No need for the llvm::, is there? This should be replaced with !isa<ReferenceType, PointerType>(T) since you only care about the Boolean results.

[flx]

Address review comments.

[flx]

Thanks for the review, Aaron! I appreciate it. Please let me know if you have any questions about the test cases.

[aaron.ballman]

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

92

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

492

Making sure I understand properly, if you were calling Orig.constMethod() then the diagnostic should be triggered, correct? Assuming so, having those test cases would be useful.

[flx]

Added positive variants of new tests.

[flx]

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

492

Your understanding is correct. I validated the positive cases with the existing tests from lines 104 and 112, but those didn't have any further references to the original variable. I agree it's easier to understand when the negative and positive cases are grouped together.

[aaron.ballman]

Thanks for verifying! Aside from the minor nit with isa<>, this LGTM

[flx]

Shortened isa<> expression.

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

 #include "../utils/OptionsUtils.h"
 #include "clang/Basic/Diagnostic.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 namespace {
 
+using namespace ::clang::ast_matchers;
+using llvm::StringRef;
+using utils::decl_ref_expr::isOnlyUsedAsConst;
+
+static constexpr StringRef kObjectArg = "objectArg";
+static constexpr StringRef kInitFunctionCall = "initFunctionCall";
+static constexpr StringRef kOldVarDecl = "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))
       Diagnostic << *Fix;
   }
 }
 
-} // namespace
+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()))),
+      on(declRefExpr(to(varDecl().bind(kObjectArg)))));
+}
 
-using namespace ::clang::ast_matchers;
-using utils::decl_ref_expr::isOnlyUsedAsConst;
+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()))),
+                  argumentCountIs(0), unless(callee(cxxMethodDecl())))
+      .bind(kInitFunctionCall);
+}
+
+AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) {
+  auto OldVarDeclRef =
+      declRefExpr(to(varDecl(hasLocalStorage()).bind(kOldVarDecl)));
+  return declStmt(has(varDecl(hasInitializer(
+      anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(),
+            ignoringImpCasts(OldVarDeclRef),
+            ignoringImpCasts(unaryOperator(
+                hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef))))))));
+}
+
+// This checks that the variable itself is only used as const, and also makes
+// sure that it does not reference another variable that could be modified in
+// the BlockStmt. It does this by checking the following:
+// 1. If the variable is neither a reference nor a pointer then the the
+// isOnlyUsedAsConst() check is sufficient.
+// 2. If the (reference or pointer) variable is not initialized in a DeclStmt in
+// the BlockStmt. In this case it its pointee is likely not modified (unless it
+// is passed as an alias into the method as well).
+// 3. If the reference is initialized from a reference to const. This is
+// the same set of criteria we apply when identifying the unnecessary copied
+// variable in this check to begin with. In this case we check whether the
+// object arg or variable that is referenced is immutable as well.
+bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
+                                     const Stmt &BlockStmt,
+                                     ASTContext &Context) {
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) {
+    return false;
+  }
+  QualType type = InitializingVar.getType();
+  if (!llvm::dyn_cast<ReferenceType>(type) &&
+      !llvm::dyn_cast<PointerType>(type)) {
+    // The variable is a value type and we know it is only used as const. Safe
+    // to reference it and avoid the copy.
+    return true;
+  }
+  auto Matches =
+      match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar))))
+                        .bind("declStmt")),
+            BlockStmt, Context);
+  if (Matches.empty()) {
+    // The reference or pointer is not initialized in the BlockStmt. We assume
+    // its pointee is not modified then.
+    return true;
+  }
+  const auto *Initialization = selectFirst<DeclStmt>("declStmt", Matches);
+  Matches =
+      match(isInitializedFromReferenceToConst(), *Initialization, Context);
+  if (selectFirst<CallExpr>(kInitFunctionCall, Matches) != nullptr) {
+    // The reference is initialized from a free function without arguments
+    // returning a const reference. This is a global immutable object.
+    return true;
+  }
+  if (const auto *OrigVar = selectFirst<VarDecl>(kObjectArg, Matches)) {
+    // Check that the object argument is immutable as well.
+    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+  }
+  if (const auto *OrigVar = selectFirst<VarDecl>(kOldVarDecl, Matches)) {
+    // Check that the old variable we reference is immutable as well.
+    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+  }
+  return false;
+}
+
+} // namespace
 
 UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AllowedTypes(
           utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
 
 void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
-  auto ConstReference = referenceType(pointee(qualType(isConstQualified())));
-
-  // 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.
-  auto ConstRefReturningMethodCall =
-      cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))),
-                        on(declRefExpr(to(varDecl().bind("objectArg")))));
-  auto ConstRefReturningFunctionCall =
-      callExpr(callee(functionDecl(returns(ConstReference))),
-               unless(callee(cxxMethodDecl())))
-          .bind("initFunctionCall");
-
   auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
                    declStmt(
                        has(varDecl(hasLocalStorage(),
                                    hasType(qualType(
                                        hasCanonicalType(allOf(
                                            matchers::isExpensiveToCopy(),
return compoundStmt(
                                                isCopyConstructor())),
                                            hasArgument(0, CopyCtorArg))
                                            .bind("ctorCall"))))
                                .bind("newVarDecl")))
                        .bind("declStmt")))
         .bind("blockStmt");
   };
 
-  Finder->addMatcher(localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall,
-                                              ConstRefReturningMethodCall)),
+  Finder->addMatcher(localVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
+                                              isConstRefReturningMethodCall())),
                      this);
 
   Finder->addMatcher(localVarCopiedFrom(declRefExpr(
-                         to(varDecl(hasLocalStorage()).bind("oldVarDecl")))),
+                         to(varDecl(hasLocalStorage()).bind(kOldVarDecl)))),
                      this);
 }
 
 void UnnecessaryCopyInitialization::check(
     const MatchFinder::MatchResult &Result) {
   const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
-  const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>("oldVarDecl");
-  const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
+  const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(kOldVarDecl);
+  const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(kObjectArg);
   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);
 
   // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
   // since we cannot place them correctly.
   bool IssueFix =
       Result.Nodes.getNodeAs<DeclStmt>("declStmt")->isSingleDecl() &&
       !NewVar->getLocation().isMacroID();
 
   // 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 (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 {
     handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
                            *Result.Context);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
     const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
     const VarDecl *ObjectArg, ASTContext &Context) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
     return;
   if (ObjectArg != nullptr &&
-      !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
+      !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context))
     return;
 
   auto Diagnostic =
       diag(Var.getLocation(),
            IsConstQualified ? "the const qualified variable %0 is "
                               "copy-constructed from a const reference; "
                               "consider making it a const reference"
                             : "the variable %0 is copy-constructed from a "
                               "const reference but is only used as const "
                               "reference; consider making it a const reference")
       << &Var;
   if (IssueFix)
     recordFixes(Var, Context, Diagnostic);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
     bool IssueFix, ASTContext &Context) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
-      !isOnlyUsedAsConst(OldVar, BlockStmt, Context))
+      !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
     return;
 
   auto Diagnostic = diag(NewVar.getLocation(),
                          "local copy %0 of the variable %1 is never modified; "
                          "consider avoiding the copy")
                     << &NewVar << &OldVar;
   if (IssueFix)
     recordFixes(NewVar, Context, Diagnostic);

clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp

constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
   auto Matches = match(
       findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
                          cxxOperatorCallExpr(ConstMethodCallee,
                                              hasArgument(0, DeclRefToVar))))),
       Stmt, Context);
   SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
-      qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+      qualType(anyOf(matchers::isReferenceToConst(),
                      unless(anyOf(referenceType(), pointerType(),
                                   substTemplateTypeParmType()))));
   auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
       ConstReferenceOrValue,
       substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
       DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =
       match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  // References and pointers to const assignments.
+  Matches =
+      match(findAll(declStmt(
+                has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
+                            hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
+            Stmt, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  Matches =
+      match(findAll(declStmt(has(varDecl(
+                hasType(qualType(matchers::isPointerToConst())),
+                hasInitializer(ignoringImpCasts(unaryOperator(
+                    hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
+            Stmt, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
   return DeclRefs;
 }
 
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context) {
   // Collect all DeclRefExprs to the loop variable and all CallExprs and
   // CXXConstructExprs where the loop variable is used as argument to a const
   // reference parameter.

clang-tools-extra/clang-tidy/utils/Matchers.h

 }
 
 // Returns QualType matcher for references to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
   using namespace ast_matchers;
   return referenceType(pointee(qualType(isConstQualified())));
 }
 
+// Returns QualType matcher for pointers to const.
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
+  using namespace ast_matchers;
+  return pointerType(pointee(qualType(isConstQualified())));
+}
+
 AST_MATCHER_P(NamedDecl, matchesAnyListedName, std::vector<std::string>,
               NameList) {
   return llvm::any_of(NameList, [&Node](const std::string &Name) {
       return llvm::Regex(Name).match(Node.getName());
     });
 }
 
 } // namespace matchers
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H

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

 }
 
 void negativeInvokedOnStdFunction(
     std::function<void(ExpensiveToCopyType &)> Update,
     const ExpensiveToCopyType Orig) {
   auto Copy = Orig.reference();
   Update(Copy);
 }
+
+void negativeCopiedFromReferenceToModifiedVar() {
+  ExpensiveToCopyType Orig;
+  const auto &Ref = Orig;
+  const auto NecessaryCopy = Ref;
+  Orig.nonConstMethod();
+}
+
+void negativeCopiedFromGetterOfReferenceToModifiedVar() {
+  ExpensiveToCopyType Orig;
+  const auto &Ref = Orig.reference();
+  const auto NecessaryCopy = Ref.reference();
+  Orig.nonConstMethod();
+}