[clang-tidy] performance-unnecessary-copy-initialization: Fix false negative.

[courbet]

Summary

isConstRefReturningMethodCall should be considering
CXXOperatorCallExpr in addition to CXXMemberCallExpr. Clang considers
these to be distinct (CXXOperatorCallExpr derives from CallExpr, not
CXXMemberCallExpr), but we don't care in the context of this
check.

This is important because of
std::vector<Expensive>::operator[](size_t) const.

[flx]

Thank you for catching and fixing this!

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

96

Does this work if if the object argument is a pointer or a type alias? Could you add a test to confirm?

[courbet]

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

96

It does not work with a pointer. I plan to make it work in a separate cl, but first I need to fix isOnlyUsedAsConst to make more general (specifically, handle unary *).

[courbet]

rebase

[courbet]

Canonicalize more types and add more container tests.

[flx]

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

228–232

Would you mind adding this test also to the file testing the exclusion of containers:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp

This would cover whether the type matching of pointer types works for excluded container types.

[courbet]

add container exclusion tests for operators.

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

 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(hasCanonicalType(matchers::isReferenceToConst())))
-                 .bind(MethodDeclId)),
-      on(declRefExpr(to(varDecl().bind(ObjectArgId)))),
-      thisPointerType(namedDecl(
-          unless(matchers::matchesAnyListedName(ExcludedContainerTypes)))));
+  const auto MethodDecl =
+      cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
+          .bind(MethodDeclId);
+  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverType =
+      hasCanonicalType(recordType(hasDeclaration(namedDecl(
+          unless(matchers::matchesAnyListedName(ExcludedContainerTypes))))));
+
+  return expr(anyOf(
+      cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
+                        thisPointerType(ReceiverType)),
+      cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
+                          hasArgument(0, hasType(ReceiverType)))));
 }
 
 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(hasCanonicalType(
                                           matchers::isReferenceToConst())))

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

 // RUN: %check_clang_tidy -std=c++17 %s performance-unnecessary-copy-initialization %t
 
 template <typename T>
 struct Iterator {
   void operator++();
-  const T &operator*() const;
+  T &operator*() const;
   bool operator!=(const Iterator &) const;
   typedef const T &const_reference;
 };
 
+template <typename T>
+struct ConstIterator {
+  void operator++();
+  const T &operator*() const;
+  bool operator!=(const ConstIterator &) const;
+  typedef const T &const_reference;
+};
+
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
   using ConstRef = const ExpensiveToCopyType &;
   ConstRef referenceWithAlias() const;
   const ExpensiveToCopyType *pointer() const;
-  Iterator<ExpensiveToCopyType> begin() const;
-  Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
   template <typename A>
   const A &templatedAccessor() const;
   operator int() const; // Implicit conversion to int.
 };
 
+template <typename T>
+struct Container {
+  bool empty() const;
+  const T& operator[](int) const;
+  const T& operator[](int);
+  Iterator<T> begin();
+  Iterator<T> end();
+  ConstIterator<T> begin() const;
+  ConstIterator<T> end() const;
+  void nonConstMethod();
+  bool constMethod() const;
+};
+
+using ExpensiveToCopyContainerAlias = Container<ExpensiveToCopyType>;
+
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 
 struct WeirdCopyCtorType {
   WeirdCopyCtorType();
   WeirdCopyCtorType(const WeirdCopyCtorType &w, bool oh_yes = true);
 
void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) {
   VarAssigned.constMethod();
 
   const ExpensiveToCopyType VarCopyConstructed(Obj->reference());
   // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj->reference());
   VarCopyConstructed.constMethod();
 }
 
+void PositiveOperatorCallConstReferenceParam(const Container<ExpensiveToCopyType> &C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType> C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlias C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
+  const auto AutoAssigned = (*C)[42];
+  // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // TODO-FIXES: const auto& AutoAssigned = (*C)[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed((*C)[42]);
+  // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // TODO-FIXES: const auto& AutoCopyConstructed((*C)[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C->operator[](42);
+  // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // TODO-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C->operator[](42));
+  // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // TODO-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
+  VarCopyConstructed.constMethod();
+}
+
 void PositiveLocalConstValue() {
   const ExpensiveToCopyType Obj;
   const auto UnnecessaryCopy = Obj.reference();
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = Obj.reference();
   UnnecessaryCopy.constMethod();
 }
 
void WarningOnlyMultiDeclStmt() {
   ExpensiveToCopyType orig;
   ExpensiveToCopyType copy = orig, copy2;
   // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy' of the variable 'orig' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]
   // CHECK-FIXES: ExpensiveToCopyType copy = orig, copy2;
   copy.constMethod();
 }
 
 class Element {};
-class Container {
-public:
-  class Iterator {
-  public:
-    void operator++();
-    Element operator*();
-    bool operator!=(const Iterator &);
-    WeirdCopyCtorType c;
-  };
-  const Iterator &begin() const;
-  const Iterator &end() const;
-};
 
 void implicitVarFalsePositive() {
-  for (const Element &E : Container()) {
+  for (const Element &E : Container<Element>()) {
   }
 }
 
 // This should not trigger the check as the argument could introduce an alias.
 void negativeInitializedFromFreeFunctionWithArg() {
   ExpensiveToCopyType Orig;
   const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig);
 }
void positiveUnusedReferenceIsRemoved() {
 
   auto UnusedAndUnnecessary = ExpensiveTypeReference();
   // Comments on a new line should not be deleted.
   // CHECK-MESSAGES: [[@LINE-2]]:8: warning: the variable 'UnusedAndUnnecessary' is copy-constructed
   // CHECK-FIXES-NOT: auto UnusedAndUnnecessary = ExpensiveTypeReference();
   // CHECK-FIXES: // Comments on a new line should not be deleted.
 }
 
-void negativeloopedOverObjectIsModified() {
-  ExpensiveToCopyType Orig;
+void positiveLoopedOverObjectIsConst() {
+  const Container<ExpensiveToCopyType> Orig;
+  for (const auto &Element : Orig) {
+    const auto Copy = Element;
+    // CHECK-MESSAGES: [[@LINE-1]]:16: warning: local copy 'Copy'
+    // CHECK-FIXES: const auto& Copy = Element;
+    Orig.constMethod();
+    Copy.constMethod();
+  }
+
+  auto Lambda = []() {
+    const Container<ExpensiveToCopyType> Orig;
+    for (const auto &Element : Orig) {
+      const auto Copy = Element;
+      // CHECK-MESSAGES: [[@LINE-1]]:18: warning: local copy 'Copy'
+      // CHECK-FIXES: const auto& Copy = Element;
+      Orig.constMethod();
+      Copy.constMethod();
+    }
+  };
+}
+
+void negativeLoopedOverObjectIsModified() {
+  Container<ExpensiveToCopyType> Orig;
   for (const auto &Element : Orig) {
     const auto Copy = Element;
     Orig.nonConstMethod();
     Copy.constMethod();
   }
 
   auto Lambda = []() {
-    ExpensiveToCopyType Orig;
+    Container<ExpensiveToCopyType> Orig;
     for (const auto &Element : Orig) {
       const auto Copy = Element;
       Orig.nonConstMethod();
       Copy.constMethod();
     }
   };
 }