[clang-tidy] performance-unnecessary-copy-initialization: Search whole function body for variable initializations.

[flx]

Summary

This fixes false positive cases where a reference is initialized outside of a
block statement and then its initializing variable is modified. Another case is
when the looped over container is modified.

[ymandel]

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

97

Maybe a bit clearer:
// Search the whole function body, not just the current block statement, for decl statements of the initialization variable.

98–103

Consider inspecting the DeclContexts instead, which should be much more efficient than searching the entire block. Pass the FunctionDecl as an argument instead of Body, since it is a DeclContext. e.g. const DeclContext &Fun

Then, either

1. Call Fun.containsDecl(InitializingVar), or
2. Search through the contexts yourself; something like:

DeclContext* DC = InitializingVar->getDeclContext(); 
while (DC != nullptr && DC != &Fun)
  DC = DC->getLexicalParent();
if (DC == nullptr)
  // The reference or pointer is not initialized anywhere witin the function. We
  // assume its pointee is not modified then.
  return true;

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

530

typo: Lambda

[flx]

Addressed first round of comments.

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

98–103

Are #1 and #2 equivalent? From the implementation and comment I cannot tell whether #1 would cover cases where the variable is not declared directly in the function, but in child block:

void Fun() {
 {
   var i;
   {
     i.usedHere();
   }  
 } 
}

I'm also reading this as an optimization to more quickly determine whether we can stop here. We still need to find the matches for the next steps, but I think I could then limit matching to the DeclContext I found here. Is this correct? For this I would actually need the DeclContext result from #2.

[ymandel]

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

98–103

A. I think you're right that #2 is more suited to what you need. I wasn't sure, so included both. Agreed that the comments are ambiguous.
B. yes, this is just an optimization. it may be premature for that matter; just that match can be expensive and this seemed a more direct expression of the algorithm.

[flx]

Use more efficient method to check for local variable declaration.

[flx]

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

98–103

I was not able to pass the (possibly more narrow) DeclContext to the match function as scope since match does not support DeclContexts.

I implemented isDeclaredInFunction() which iterates through the decl contexts as you suggested. I'm not sure though whether we should start with VarDecl::getDeclContext() or VarDecl::getLexicalDeclContext()?

While looking at VarDecl::getLexicalDeclContext() I discovered is VarDecl has the following method:

/// Returns true for local variable declarations other than parameters.                                                    
/// Note that this includes static variables inside of functions. It also                                                  
/// includes variables inside blocks.                                                                                      
///                                                                                                                        
///   void foo() { int x; static int y; extern int z; }                                                                    
bool isLocalVarDecl() const;

I think this is exactly what we'd want here. What do you think?

[ymandel]

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

98–103

You mean instead of isDeclaredInFunction? If so -- yes, that seems right. But, if so, are you still planning to bind "declStmt" with the match, or will you replace that with something more direct?

[flx]

Directly examine initializer.

[flx]

Remove now unnecessary FunctionDecl.

[flx]

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

98–103

I was able to examine the`VarDecl.getInit()` expression directly. This completely avoids a search inside the function and he FunctionDecl is also no longer needed. PTAL.

[ymandel]

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

61

This name seems a little off now. maybe rename?

98–103

Nice!

[flx]

Renamed initializer matcher.

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

bool isLanguageVersionSupported(const LangOptions &LangOpts) const override{
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
-                                  bool IssueFix, const VarDecl *ObjectArg,
+                                  const Stmt &Body, bool IssueFix,
+                                  const VarDecl *ObjectArg,
                                   ASTContext &Context);
   void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
-                              const Stmt &BlockStmt, bool IssueFix,
-                              ASTContext &Context);
+                              const Stmt &BlockStmt, const Stmt &Body,
+                              bool IssueFix, ASTContext &Context);
   const std::vector<std::string> AllowedTypes;
 };
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H

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

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(InitFunctionCallId);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) {
   auto OldVarDeclRef =
       declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
   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
 // isOnlyUsedAsConst() check is sufficient.
 // 2. If the (reference or pointer) variable is not initialized in a DeclStmt in
 // the BlockStmt. In this case 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.
 static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
                                             const Stmt &BlockStmt,
+                                            const Stmt &Body,
                                             ASTContext &Context) {
   if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
     return false;
 
   QualType T = InitializingVar.getType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa<ReferenceType, PointerType>(T))
     return true;
 
+  // Search the whole function body, not just the current block statement, for
+  // decl statements of the initialization variable.
   auto Matches =
       match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar))))
                         .bind("declStmt")),
-            BlockStmt, Context);
-  // The reference or pointer is not initialized in the BlockStmt. We assume
-  // its pointee is not modified then.
+            Body, Context);
+  // The reference or pointer is not initialized anywhere witin the function. We
+  // assume its pointee is not modified then.
   if (Matches.empty())
     return true;
 
   const auto *Initialization = selectFirst<DeclStmt>("declStmt", Matches);
   Matches =
       match(isInitializedFromReferenceToConst(), *Initialization, Context);
   // The reference is initialized from a free function without arguments
   // returning a const reference. This is a global immutable object.
   if (selectFirst<CallExpr>(InitFunctionCallId, Matches) != nullptr)
     return true;
   // Check that the object argument is immutable as well.
   if (const auto *OrigVar = selectFirst<VarDecl>(ObjectArgId, Matches))
-    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Body, Context);
   // Check that the old variable we reference is immutable as well.
   if (const auto *OrigVar = selectFirst<VarDecl>(OldVarDeclId, Matches))
-    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+    return isInitializingVariableImmutable(*OrigVar, BlockStmt, Body, 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 LocalVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
                    declStmt(
+                       hasAncestor(functionDecl().bind("containingFunc")),
                        has(varDecl(hasLocalStorage(),
                                    hasType(qualType(
                                        hasCanonicalType(allOf(
                                            matchers::isExpensiveToCopy(),
                                            unless(hasDeclaration(namedDecl(
                                                hasName("::std::function")))))),
                                        unless(hasDeclaration(namedDecl(
                                            matchers::matchesAnyListedName(

 
 void UnnecessaryCopyInitialization::check(
     const MatchFinder::MatchResult &Result) {
   const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
   const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(OldVarDeclId);
   const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(ObjectArgId);
   const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
+  const auto *Body =
+      Result.Nodes.getNodeAs<FunctionDecl>("containingFunc")->getBody();
 
   TraversalKindScope RAII(*Result.Context, 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) {
-    handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+    handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Body, IssueFix, ObjectArg,
                                *Result.Context);
   } else {
-    handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
+    handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Body, IssueFix,
                            *Result.Context);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
-    const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
+    const VarDecl &Var, const Stmt &BlockStmt, const Stmt &Body, bool IssueFix,
     const VarDecl *ObjectArg, ASTContext &Context) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
     return;
   if (ObjectArg != nullptr &&
-      !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context))
+      !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Body, Context))
     return;
 
   auto Diagnostic =
       diag(Var.getLocation(),
            "the %select{|const qualified }0variable %1 is copy-constructed "
            "from a const reference%select{ but is only used as const "
            "reference|}0; consider making it a const reference")
       << IsConstQualified << &Var;
   if (IssueFix)
     recordFixes(Var, Context, Diagnostic);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
-    bool IssueFix, ASTContext &Context) {
+    const Stmt &Body, bool IssueFix, ASTContext &Context) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
-      !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
+      !isInitializingVariableImmutable(OldVar, BlockStmt, Body, 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/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

 // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
 
+template <typename T>
+struct Iterator {
+  void operator++();
+  const T &operator*() const;
+  bool operator!=(const Iterator &) const;
+  typedef const T &const_reference;
+};
+
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
+  Iterator<ExpensiveToCopyType> begin() const;
+  Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
 };
 
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 

 void positiveCopiedFromGetterOfReferenceToConstVar() {
   ExpensiveToCopyType Orig;
   const auto &Ref = Orig.reference();
   auto UnnecessaryCopy = Ref.reference();
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'UnnecessaryCopy' is
   // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
   Orig.constMethod();
 }
+
+void negativeloopedOverObjectIsModified() {
+  ExpensiveToCopyType Orig;
+  for (const auto &Element : Orig) {
+    const auto Copy = Element;
+    Orig.nonConstMethod();
+    Copy.constMethod();
+  }
+
+  auto Lambda = []() {
+    ExpensiveToCopyType Orig;
+    for (const auto &Element : Orig) {
+      const auto Copy = Element;
+      Orig.nonConstMethod();
+      Copy.constMethod();
+    }
+  };
+}
+
+void negativeReferenceIsInitializedOutsideOfBlock() {
+  ExpensiveToCopyType Orig;
+  const auto &E2 = Orig;
+  if (1 != 2 * 3) {
+    const auto C2 = E2;
+    Orig.nonConstMethod();
+    C2.constMethod();
+  }
+
+  auto Lambda = []() {
+    ExpensiveToCopyType Orig;
+    const auto &E2 = Orig;
+    if (1 != 2 * 3) {
+      const auto C2 = E2;
+      Orig.nonConstMethod();
+      C2.constMethod();
+    }
+  };
+}