[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

212–216

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 ReceiverTypeDecl =
+      namedDecl(unless(matchers::matchesAnyListedName(ExcludedContainerTypes)));
+
+  return expr(anyOf(
+      cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
+                        thisPointerType(ReceiverTypeDecl)),
+      cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
+                          hasArgument(0, hasType(recordType(hasDeclaration(
+                                             ReceiverTypeDecl)))))));
 }
 
 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

struct ExpensiveToCopyType {
   Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
   template <typename A>
   const A &templatedAccessor() const;
   operator int() const; // Implicit conversion to int.
 };
 
+struct ExpensiveToCopyContainer {
+  const ExpensiveToCopyType &operator[](int) const;
+  const ExpensiveToCopyType &operator[](int);
+};
+
 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 ExpensiveToCopyContainer &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 ExpensiveToCopyContainer 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 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 PositiveLocalConstRef() {
   const ExpensiveToCopyType Obj;
   const ExpensiveToCopyType &ConstReference = Obj.reference();
   const auto UnnecessaryCopy = ConstReference.reference();
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = ConstReference.reference();
   UnnecessaryCopy.constMethod();
 }
 
 void PositiveLocalConstPointer() {
   const ExpensiveToCopyType Obj;
   const ExpensiveToCopyType *const ConstPointer = &Obj;
   const auto UnnecessaryCopy = ConstPointer->reference();
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = ConstPointer->reference();
   UnnecessaryCopy.constMethod();
 }
 
 void NegativeFunctionCallTrivialType() {
   const auto AutoAssigned = TrivialTypeReference();
   const auto AutoCopyConstructed(TrivialTypeReference());
   const TrivialToCopyType VarAssigned = TrivialTypeReference();
   const TrivialToCopyType VarCopyConstructed(TrivialTypeReference());
 }
 
 void NegativeStaticLocalVar(const ExpensiveToCopyType &Obj) {
   static const auto StaticVar = Obj.reference();