[clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[flx]

Summary

This fixes false positive cases where a non-const reference is passed to a
std::function but interpreted as a const reference.

Fix the definition of the fake std::function added in the test to match std::function and make the bug reproducible.

[flx]

Remove code comment.

[flx]

Revert -std=c++17 change.

[flx]

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

[aaron.ballman]

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

[flx]

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

[aaron.ballman]

In D90042#2356265, @flx wrote:

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

[flx]

Add instantiated template method.

[flx]

Comment out replaced parameter.

[flx]

In D90042#2357078, @aaron.ballman wrote:

In D90042#2356265, @flx wrote:

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

[aaron.ballman]

In D90042#2360042, @flx wrote:

In D90042#2357078, @aaron.ballman wrote:

In D90042#2356265, @flx wrote:

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

I can't, but to be honest, I'm not certain I understand how that false positive could happen in the first place. That's why I was hoping to see the original case -- one thing you could try is with the original code, pass -E to preprocess to a file, and then try reducing the test case from that output (either by hand or by using a tool like creduce), or did you already give that a shot?

[flx]

In D90042#2368219, @aaron.ballman wrote:

In D90042#2360042, @flx wrote:

In D90042#2357078, @aaron.ballman wrote:

In D90042#2356265, @flx wrote:

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

I can't, but to be honest, I'm not certain I understand how that false positive could happen in the first place. That's why I was hoping to see the original case -- one thing you could try is with the original code, pass -E to preprocess to a file, and then try reducing the test case from that output (either by hand or by using a tool like creduce), or did you already give that a shot?

Thanks for the suggestion, I had never hear of creduce! After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Here's also the test script I used for posterity:

#!/bin/bash                                                                                                                  
                                                                                                                             
trap 'exit 1' ERR                                                                                                            
out=$(tempfile)                                                                                                              
clang-tidy  --checks=-*,performance-unnecessary-copy-initialization --extra-arg=-std=c++17 full.cc > $out                                     
grep "warning: local copy.*Copy" $out                                                                                        
grep "f(Copy)" full.cc                                                                                                       
grep "std::function" full.cc

I'll update the test case with this next.

[aaron.ballman]

In D90042#2390203, @flx wrote:

Thanks for the suggestion, I had never hear of creduce!

Glad to have introduced you to it -- it's a great tool!

After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Aha, I may have spotted it. The call operators have subtly different signatures and the signature we have in our test file is wrong. Note the && in our test file compared to what the standard defines: http://eel.is/c++draft/func.wrap.func#inv which is what's causing the difference here: https://godbolt.org/z/hxfM7P

[flx]

Fixed definition of fake std::function which now makes the bug fixed by this change reproducible.

[flx]

Remove unnecessary test code that is not needed.

[flx]

In D90042#2391246, @aaron.ballman wrote:

In D90042#2390203, @flx wrote:

Thanks for the suggestion, I had never hear of creduce!

Glad to have introduced you to it -- it's a great tool!

After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Aha, I may have spotted it. The call operators have subtly different signatures and the signature we have in our test file is wrong. Note the && in our test file compared to what the standard defines: http://eel.is/c++draft/func.wrap.func#inv which is what's causing the difference here: https://godbolt.org/z/hxfM7P

Ah! That is subtle and surprising, but makes sense. I confirmed that the new test cases fail now before the fix is put in place. Thanks for your help!

[flx]

Updated change description.

[aaron.ballman]

LGTM, thank you for the fix!

[flx]

In D90042#2397885, @aaron.ballman wrote:

LGTM, thank you for the fix!

Thanks for helping track down the difference in our definition of std::function :)

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

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()))),
-                     unless(anyOf(referenceType(), pointerType()))));
+                     unless(anyOf(referenceType(), pointerType(),
+                                  substTemplateTypeParmType()))));
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+      ConstReferenceOrValue,
+      substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-      DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+      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);
   return DeclRefs;
 }
 

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

void negativeInitialzedFromFreeFunctionWithNonDefaultArg() {
   const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig);
 }
 
 namespace std {
 inline namespace __1 {
 
 template <class>
 class function;
-template <class R, class... Args>
-class function<R(Args...)> {
+template <class R, class... ArgTypes>
+class function<R(ArgTypes...)> {
 public:
   function();
-  function(const function &other);
-  R operator()(Args &&...args) const;
+  function(const function &Other);
+  R operator()(ArgTypes... Args) const;
+};
+
+template <typename>
+struct remove_reference;
+
+template <typename _Tp>
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template <typename _Tp>
+constexpr _Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
 void negativeStdFunction() {
   std::function<int()> Orig;
   std::function<int()> Copy = Orig;
   int i = Orig();
 }

 
 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
+
+void positiveInvokedOnStdFunction(
+    std::function<void(const ExpensiveToCopyType &)> Update,
+    const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+    std::function<void(ExpensiveToCopyType &)> Update,
+    const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}