[clang-tidy] performance-unnecessary-copy-initialization: Create option to exclude container types from triggering the check.

[flx]

Summary

Add string list option of type names analagous to AllowedTypes which lets
users specify a list of ExcludedContainerTypes.

Types matching this list will not trigger the check when an expensive variable
is copy initialized from a const accessor method they provide, i.e.:

ExcludedContainerTypes = 'ExcludedType'

void foo() {
  ExcludedType<ExpensiveToCopy> Container;
  const ExpensiveToCopy NecessaryCopy = Container.get();
}

Even though an expensive to copy variable is copy initialized the check does not
trigger because the container type is excluded.

This is useful for container types that don't own their data, such as view types
where modification of the returned references in other places cannot be reliably
tracked, or const incorrect types.

[ymandel]

Looks good. I was only a little confused by the first sentence of the option description.

clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

53

should this "not" be here?

[flx]

Add test file for the new option.

[flx]

I also added a file testing the new option.

clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

53

Yeah, that makes it more clear. Done.

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

private:
   void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
                                   const DeclStmt &Stmt, bool IssueFix,
                                   const VarDecl *ObjectArg,
                                   ASTContext &Context);
   void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
                               const Stmt &BlockStmt, const DeclStmt &Stmt,
                               bool IssueFix, ASTContext &Context);
   const std::vector<std::string> AllowedTypes;
+  const std::vector<std::string> ExcludedContainerTypes;
 };
 
 } // 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

auto End =
             : BeforeFirstTokenAfterComment;
     Diagnostic << FixItHint::CreateRemoval(
         SourceRange(Stmt.getBeginLoc(), End));
   } else {
     Diagnostic << FixItHint::CreateRemoval(Stmt.getSourceRange());
   }
 }
 
-AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
+AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
+                       std::vector<std::string>, ExcludedContainerTypes) {
   // 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()))
                  .bind(MethodDeclId)),
-      on(declRefExpr(to(varDecl().bind(ObjectArgId)))));
+      on(declRefExpr(to(
+          varDecl(
+              unless(hasType(qualType(hasCanonicalType(hasDeclaration(namedDecl(
+                  matchers::matchesAnyListedName(ExcludedContainerTypes))))))))
+              .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()))
                              .bind(FunctionDeclId)),
                   argumentCountIs(0), unless(callee(cxxMethodDecl())))
       .bind(InitFunctionCallId);
 }
 
-AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) {
+AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
+                       std::vector<std::string>, ExcludedContainerTypes) {
   auto OldVarDeclRef =
       declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
   return expr(
-      anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(),
+      anyOf(isConstRefReturningFunctionCall(),
+            isConstRefReturningMethodCall(ExcludedContainerTypes),
             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,
-                                            ASTContext &Context) {
+static bool isInitializingVariableImmutable(
+    const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context,
+    const std::vector<std::string> &ExcludedContainerTypes) {
   if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
     return false;
 
   QualType T = InitializingVar.getType().getCanonicalType();
   // 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;
 
   // The reference or pointer is not declared and hence not initialized anywhere
   // in the function. We assume its pointee is not modified then.
   if (!InitializingVar.isLocalVarDecl() || !InitializingVar.hasInit()) {
     return true;
   }
 
-  auto Matches = match(initializerReturnsReferenceToConst(),
-                       *InitializingVar.getInit(), Context);
+  auto Matches =
+      match(initializerReturnsReferenceToConst(ExcludedContainerTypes),
   // 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, Context,
+                                           ExcludedContainerTypes);
   // 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, Context,
+                                           ExcludedContainerTypes);
 
   return false;
 }
 
 bool isVariableUnused(const VarDecl &Var, const Stmt &BlockStmt,
                       ASTContext &Context) {
   return allDeclRefExprs(Var, BlockStmt, Context).empty();
 }

 }
 
 } // namespace
 
 UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AllowedTypes(
-          utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
+          utils::options::parseStringList(Options.get("AllowedTypes", ""))),
+      ExcludedContainerTypes(utils::options::parseStringList(
+          Options.get("ExcludedContainerTypes", ""))) {}
 
 void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
   auto LocalVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
                    declStmt(
                        unless(has(decompositionDecl())),
                        has(varDecl(hasLocalStorage(),
return compoundStmt(
                                            hasArgument(0, CopyCtorArg))
                                            .bind("ctorCall"))))
                                .bind("newVarDecl")))
                        .bind("declStmt")))
         .bind("blockStmt");
   };
 
   Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
-                                              isConstRefReturningMethodCall())),
+                                              isConstRefReturningMethodCall(
+                                                  ExcludedContainerTypes))),
                      this);
 
   Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
                          to(varDecl(hasLocalStorage()).bind(OldVarDeclId)))),
                      this);
 }
 
 void UnnecessaryCopyInitialization::check(

 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
     const VarDecl &Var, const Stmt &BlockStmt, const DeclStmt &Stmt,
     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, Context,
+                                       ExcludedContainerTypes))
     return;
   if (isVariableUnused(Var, BlockStmt, Context)) {
     auto Diagnostic =
         diag(Var.getLocation(),
              "the %select{|const qualified }0variable %1 is copy-constructed "
              "from a const reference but is never used; consider "
              "removing the statement")
         << IsConstQualified << &Var;
if (IssueFix)
       recordFixes(Var, Context, Diagnostic);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
     const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
-      !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
+      !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+                                       ExcludedContainerTypes))
     return;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
     auto Diagnostic = diag(NewVar.getLocation(),
                            "local copy %0 of the variable %1 is never modified "
                            "and never used; "
                            "consider removing the statement")
                       << &NewVar << &OldVar;
if (IssueFix)
       recordFixes(NewVar, Context, Diagnostic);
   }
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AllowedTypes",
                 utils::options::serializeStringList(AllowedTypes));
+  Options.store(Opts, "ExcludedContainerTypes",
+                utils::options::serializeStringList(ExcludedContainerTypes));
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

 .. option:: AllowedTypes
 
    A semicolon-separated list of names of types allowed to be initialized by
    copying. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
    every type with suffix `Ref`, `ref`, `Reference` and `reference`. The default
    is empty. If a name in the list contains the sequence `::` it is matched
    against the qualified typename (i.e. `namespace::Type`, otherwise it is
    matched against only the type name (i.e. `Type`).
+
+.. option:: ExcludedContainerTypes
+
+   A semicolon-separated list of names of types whose methods are allowed to
+   return the const reference the variable is copied from. When an expensive to
+   copy variable is copy initialized by the return value from a type on this
+   list the check does not trigger. This can be used to exclude types known to
+   be const incorrect or where the lifetime or immutability of returned
+   references is not tied to mutations of the container. An example are view
+   types that don't own the underlying data. Like for `AllowedTypes` above,
+   regular expressions are accepted and the inclusion of `::` determines whether
+   the qualified typename is matched or not.

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

+// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t -- -config="{CheckOptions: [{key: performance-unnecessary-copy-initialization.ExcludedContainerTypes, value: 'ns::ViewType$;::ConstInCorrectType$'}]}" --
+
+namespace ns {
+template <typename T>
+struct ViewType {
+  ViewType(const T &);
+  const T &view() const;
+};
+} // namespace ns
+
+template <typename T>
+struct ViewType {
+  ViewType(const T &);
+  const T &view() const;
+};
+
+struct ExpensiveToCopy {
+  ~ExpensiveToCopy();
+  void constMethod() const;
+};
+
+struct ConstInCorrectType {
+  const ExpensiveToCopy &secretlyMutates() const;
+};
+
+using NSVTE = ns::ViewType<ExpensiveToCopy>;
+typedef ns::ViewType<ExpensiveToCopy> FewType;
+
+void positiveViewType() {
+  ExpensiveToCopy E;
+  ViewType<ExpensiveToCopy> V(E);
+  const auto O = V.view();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'O' is copy-constructed
+  // CHECK-FIXES: const auto& O = V.view();
+  O.constMethod();
+}
+
+void excludedViewTypeInNamespace() {
+  ExpensiveToCopy E;
+  ns::ViewType<ExpensiveToCopy> V(E);
+  const auto O = V.view();
+  O.constMethod();
+}
+
+void excludedViewTypeAliased() {
+  ExpensiveToCopy E;
+  NSVTE V(E);
+  const auto O = V.view();
+  O.constMethod();
+
+  FewType F(E);
+  const auto P = F.view();
+  P.constMethod();
+}
+
+void excludedConstIncorrectType() {
+  ConstInCorrectType C;
+  const auto E = C.secretlyMutates();
+  E.constMethod();
+}