[clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

[flx]

Summary

Since its call operator is const but can modify the state of its underlying
functor we cannot tell whether the copy is necessary or not.

This avoids false positives.

[lebedev.ri]

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

41

Just put it here?

[flx]

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

41

I tried this first, but this list is substring matched against only on the non-qualified type name, so std::function would not match anything and if we added "function" here it would match many other types that contain the word function.

[gribozavr2]

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

409

Could you add a nested inline namespace to better imitate what declarations look like in libc++?

[lebedev.ri]

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

41

I would personally say it's a bug, especially because i personally don't like hardcoded "choices" that are impossible to change.

[flx]

Add more complete fake version of std::function.

[flx]

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

41

Are you referring to how the matching of "AllowedTypes" works or the fact that std::function causes false positives?

The latter is currently a bug and by excluding it now its severity is downgraded from bug to missing feature. Is your concern that you would like to have std::function be matched even if it is a false positive?

Regarding "AllowedTypes" matching I'm also surprised to find it matches substrings without namespaces as the name doesn't communicate this and makes it an imprecise net to cast. Changing it could break existing users that have type substrings configured though.

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

409

I'm not sure I follow. I looked through the other tests that declare a std function and copied the declaration from modernize-avoid-bind.cpp.

[njames93]

How does this type alias and typedef, In theory that should also be handled.

using Functor = std::function<...>;
Functor Copy = Orig; // No warning.

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

35–40

It's better to use node.isInStdNamespace() instead of checking the qualified name as its designed for that very purpose. Is should probably take a CXXRecordDecl instead of a NamedDecl aswell.

[gribozavr2]

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

409

libc++ declarations look like this:

namespace std {
inline namespace __1 {
template<...> struct function...
} // __1
} // std

The inline namespace in the middle often trips up declaration matching in checkers. And yes, many other tests don't imitate this pattern, and are often broken with libc++. Those tests should be improved.

[njames93]

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

409

@flx Thats the reason why its advisable to use Decl::isInStdNamespace as it will handle inline namespaces.

[njames93]

Come to think of it, this is a pretty illogical way to solve this problem, just append ::std::function to the AllowedTypes vector in registerMatchers and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simpler fix.
The has(Any)Name matcher has logic for skipping inline namespaces.

[aaron.ballman]

In D89332#2338319, @njames93 wrote:

Come to think of it, this is a pretty illogical way to solve this problem, just append ::std::function to the AllowedTypes vector in registerMatchers and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simpler fix.
The has(Any)Name matcher has logic for skipping inline namespaces.

This also seems like a reasonable alternative, to me.

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

35

There's no need for this matcher -- hasName("::std::function") is the correct way to test for this.

[flx]

Use hasName matcher on the declaration of the canonical type.

[flx]

Thank you all for the input!

In D89332#2336566, @njames93 wrote:

How does this type alias and typedef, In theory that should also be handled.

using Functor = std::function<...>;
Functor Copy = Orig; // No warning.

Good point. I added test cases that cover this and motivated the use of the hasName() matcher on the canonical type.

In D89332#2338319, @njames93 wrote:

Come to think of it, this is a pretty illogical way to solve this problem, just append ::std::function to the AllowedTypes vector in registerMatchers and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simpler fix.
The has(Any)Name matcher has logic for skipping inline namespaces.

AllowedTypes is currently matched using a regular expression on only the not fully qualified name of the NamedDecl:

https://github.com/llvm/llvm-project/blob/343410d1cc154db99b7858e0a9c3ffd86ad94e45/clang-tools-extra/clang-tidy/utils/Matchers.h#L49

This is why this approach didn't work.

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

35–40

hasName() on the canonical type worked and handled type aliases as well.

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

409

Thanks for the extra explanation and sample code. This is done now and works.

[flx]

Fix compile errors.

[aaron.ballman]

LGTM with an extra testing request (that should hopefully just work for you).

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

445

Can you add one more test case to be sure we really only care about The std::function and not some pathological code like:

namespace clever {
namespace std {
template <class R, class... Args>
class function {}; // clever::std::function is not ::std::function
}
}
}

[flx]

Add fake std function to ensure it is still matched.

[flx]

Added a fake std function that still triggers the check.

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

if (llvm::Optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
             Var, Context, DeclSpec::TQ::TQ_const))
       Diagnostic << *Fix;
   }
 }
 
 } // namespace
 
 using namespace ::clang::ast_matchers;
 using utils::decl_ref_expr::isOnlyUsedAsConst;
 
 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(
-                                           matchers::isExpensiveToCopy()),
+                                       hasCanonicalType(allOf(
+                                           matchers::isExpensiveToCopy(),
+                                           unless(hasDeclaration(namedDecl(
+                                               hasName("::std::function")))))),
                                        unless(hasDeclaration(namedDecl(
                                            matchers::matchesAnyListedName(
                                                AllowedTypes)))))),
                                    unless(isImplicit()),
                                    hasInitializer(traverse(
                                        ast_type_traits::TK_AsIs,
                                        cxxConstructExpr(
                                            hasDeclaration(cxxConstructorDecl(

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

 void negativeInitializedFromFreeFunctionWithDefaultArg() {
   const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg();
 }
 
 void negativeInitialzedFromFreeFunctionWithNonDefaultArg() {
   ExpensiveToCopyType Orig;
   const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig);
 }
+
+namespace std {
+inline namespace __1 {
+
+template <class>
+class function;
+template <class R, class... Args>
+class function<R(Args...)> {
+public:
+  function();
+  function(const function &other);
+  R operator()(Args &&...args) const;
+};
+
+} // namespace __1
+} // namespace std
+
+void negativeStdFunction() {
+  std::function<int()> Orig;
+  std::function<int()> Copy = Orig;
+  int i = Orig();
+}
+
+using Functor = std::function<int()>;
+
+void negativeAliasedStdFunction() {
+  Functor Orig;
+  Functor Copy = Orig;
+  int i = Orig();
+}
+
+typedef std::function<int()> TypedefFunc;
+
+void negativeTypedefedStdFunction() {
+  TypedefFunc Orig;
+  TypedefFunc Copy = Orig;
+  int i = Orig();
+}
+
+namespace fake {
+namespace std {
+template <class R, class... Args>
+struct function {
+  // Custom copy constructor makes it expensive to copy;
+  function(const function &);
+};
+} // namespace std
+
+void positiveFakeStdFunction(std::function<void(int)> F) {
+  auto Copy = F;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'Copy' of the variable 'F' is never modified;
+  // CHECK-FIXES: const auto& Copy = F;
+}
+
+} // namespace fake