[clang-tidy] doesNotMutateObject: Handle calls to member functions …

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

@@ -75,16 +75,15 @@ void recordRemoval(const DeclStmt &Stmt, ASTContext &Context,
   }
 }
 
-AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
+AST_MATCHER_FUNCTION_P(StatementMatcher, isRefReturningMethodCall,
                        std::vector<StringRef>, 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.
+  // const, this will be checked in `check()` part. This returned reference is
+  // highly likely to outlive the local const reference of the variable being
+  // declared. The assumption is that the reference being returned either points
+  // to a global static variable or to a member of the called object.
   const auto MethodDecl =
-      cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
+      cxxMethodDecl(returns(hasCanonicalType(referenceType())))
           .bind(MethodDeclId);
   const auto ReceiverExpr =
       ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
@@ -121,7 +120,7 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
       declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
   return expr(
       anyOf(isConstRefReturningFunctionCall(),
-            isConstRefReturningMethodCall(ExcludedContainerTypes),
+            isRefReturningMethodCall(ExcludedContainerTypes),
             ignoringImpCasts(OldVarDeclRef),
             ignoringImpCasts(unaryOperator(hasOperatorName("&"),
                                            hasUnaryOperand(OldVarDeclRef)))));
@@ -259,9 +258,9 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
         .bind("blockStmt");
   };
 
-  Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
-                                              isConstRefReturningMethodCall(
-                                                  ExcludedContainerTypes))),
+  Finder->addMatcher(LocalVarCopiedFrom(anyOf(
+                         isConstRefReturningFunctionCall(),
+                         isRefReturningMethodCall(ExcludedContainerTypes))),
                      this);
 
   Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(

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

@@ -36,6 +36,111 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
     Nodes.insert(Match.getNodeAs<Node>(ID));
 }
 
+// If `D` has a const-qualified overload with otherwise identical
+// ref-qualifiers, returns that overload.
+const CXXMethodDecl *findConstOverload(const CXXMethodDecl &D) {
+  assert(!D.isConst());
+
+  DeclContext::lookup_result lookup_result =
+      D.getParent()->lookup(D.getNameInfo().getName());
+  if (lookup_result.isSingleResult()) {
+    // No overload.
+    return nullptr;
+  }
+  for (const Decl *overload : lookup_result) {
+    const CXXMethodDecl *candidate = dyn_cast<CXXMethodDecl>(overload);
+    if (candidate && !candidate->isDeleted() && candidate->isConst() &&
+        candidate->getRefQualifier() == D.getRefQualifier()) {
+      return candidate;
+    }
+  }
+  return nullptr;
+}
+
+// Returns true if both types refer to the same to the same type,
+// ignoring the const-qualifier.
+bool isSameTypeIgnoringConst(QualType A, QualType B) {
+  A = A.getCanonicalType();
+  B = B.getCanonicalType();
+  A.addConst();
+  B.addConst();
+  return A == B;
+}
+
+// Returns true if both types are pointers or reference to the same type,
+// ignoring the const-qualifier.
+bool pointsToSameTypeIgnoringConst(QualType A, QualType B) {
+  assert(A->isPointerType() || A->isReferenceType());
+  assert(B->isPointerType() || B->isReferenceType());
+  return isSameTypeIgnoringConst(A->getPointeeType(), B->getPointeeType());
+}
+
+// Return true if non-const member function `M` likely does not mutate `*this`.
+//
+// Note that if the member call selects a method/operator `f` that
+// is not const-qualified, then we also consider that the object is
+// not mutated if:
+//  - (A) there is a const-qualified overload `cf` of `f` that has
+//  the
+//    same ref-qualifiers;
+//  - (B) * `f` returns a value, or
+//        * if `f` returns a `T&`, `cf` returns a `const T&` (up to
+//          possible aliases such as `reference` and
+//          `const_reference`), or
+//        * if `f` returns a `T*`, `cf` returns a `const T*` (up to
+//          possible aliases).
+//  - (C) the result of the call is not mutated.
+//
+// The assumption that `cf` has the same semantics as `f`.
+// For example:
+//   - In `std::vector<T> v; const T t = v[...];`, we consider that
+//     expression `v[...]` does not mutate `v` as
+//    `T& std::vector<T>::operator[]` has a const overload
+//     `const T& std::vector<T>::operator[] const`, and the
+//     result expression of type `T&` is only used as a `const T&`;
+//   - In `std::map<K, V> m; V v = m.at(...);`, we consider
+//     `m.at(...)` to be an immutable access for the same reason.
+// However:
+//   - In `std::map<K, V> m; const V v = m[...];`, We consider that
+//     `m[...]` mutates `m` as `V& std::map<K, V>::operator[]` does
+//     not have a const overload.
+//   - In `std::vector<T> v; T& t = v[...];`, we consider that
+//     expression `v[...]` mutates `v` as the result is kept as a
+//     mutable reference.
+//
+// This function checks (A) ad (B), but the caller should make sure that the
+// object is not mutated through the return value.
+bool isLikelyShallowConst(const CXXMethodDecl &M) {
+  assert(!M.isConst());
+  // The method can mutate our variable.
+
+  // (A)
+  const CXXMethodDecl *ConstOverload = findConstOverload(M);
+  if (ConstOverload == nullptr) {
+    return false;
+  }
+
+  // (B)
+  const QualType CallTy = M.getReturnType().getCanonicalType();
+  const QualType OverloadTy = ConstOverload->getReturnType().getCanonicalType();
+  if (CallTy->isReferenceType()) {
+    if (!(OverloadTy->isReferenceType() &&
+          pointsToSameTypeIgnoringConst(CallTy, OverloadTy))) {
+      return false;
+    }
+  } else if (CallTy->isPointerType()) {
+    if (!(OverloadTy->isPointerType() &&
+          pointsToSameTypeIgnoringConst(CallTy, OverloadTy))) {
+      return false;
+    }
+  } else {
+    if (!isSameTypeIgnoringConst(CallTy, OverloadTy)) {
+      return false;
+    }
+  }
+  return true;
+}
+
 // A matcher that matches DeclRefExprs that are used in ways such that the
 // underlying declaration is not modified.
 // If the declaration is of pointer type, `Indirections` specifies the level
@@ -54,16 +159,15 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
 //  matches (A).
 //
 AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
-  // We walk up the parents of the DeclRefExpr recursively until we end up on a
-  // parent that cannot modify the underlying object. There are a few kinds of
-  // expressions:
-  //  - Those that cannot be used to mutate the underlying object. We can stop
+  // We walk up the parents of the DeclRefExpr recursively. There are a few
+  // kinds of expressions:
+  //  - Those that cannot be used to mutate the underlying variable. We can stop
   //    recursion there.
-  //  - Those that can be used to mutate the underlying object in analyzable
+  //  - Those that can be used to mutate the underlying variable in analyzable
   //    ways (such as taking the address or accessing a subobject). We have to
   //    examine the parents.
   //  - Those that we don't know how to analyze. In that case we stop there and
-  //    we assume that they can mutate the underlying expression.
+  //    we assume that they can modify the expression.
 
   struct StackEntry {
     StackEntry(const Expr *E, int Indirections)
@@ -90,7 +194,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
       assert(Ty->isPointerType());
       Ty = Ty->getPointeeType().getCanonicalType();
     }
-    if (Ty.isConstQualified())
+    if (Ty->isVoidType() || Ty.isConstQualified())
       continue;
 
     // Otherwise we have to look at the parents to see how the expression is
@@ -159,11 +263,56 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
             // The method call cannot mutate our variable.
             continue;
           }
+          if (isLikelyShallowConst(*Method)) {
+            // We still have to check that the object is not modified through
+            // the method's return value (C).
+            const auto MemberParents = Ctx.getParents(*Member);
+            assert(MemberParents.size() == 1);
+            const CallExpr *Call = MemberParents[0].get<CallExpr>();
+            // If `o` is an object of class type and `f` is a member function,
+            // then `o.f` has to be used as part of a call expression.
+            assert(Call != nullptr && "member function has to be called");
+            Stack.emplace_back(
+                Call,
+                Method->getReturnType().getCanonicalType()->isPointerType()
+                    ? 1
+                    : 0);
+            continue;
+          }
           return false;
         }
         Stack.emplace_back(Member, 0);
         continue;
       }
+      if (const auto *const OpCall = dyn_cast<CXXOperatorCallExpr>(P)) {
+        // Operator calls have function call syntax. The `*this` parameter
+        // is the first parameter.
+        if (OpCall->getNumArgs() == 0 || OpCall->getArg(0) != Entry.E) {
+          return false;
+        }
+        const auto *const Method =
+            dyn_cast<CXXMethodDecl>(OpCall->getDirectCallee());
+
+        if (Method == nullptr) {
+          // This is not a member operator. Typically, a friend operator. These
+          // are handled like function calls.
+          return false;
+        }
+
+        if (Method->isConst() || Method->isStatic()) {
+          continue;
+        }
+        if (isLikelyShallowConst(*Method)) {
+          // We still have to check that the object is not modified through
+          // the operator's return value (C).
+          Stack.emplace_back(
+              OpCall,
+              Method->getReturnType().getCanonicalType()->isPointerType() ? 1
+                                                                          : 0);
+          continue;
+        }
+        return false;
+      }
 
       if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
         switch (Op->getOpcode()) {

clang-tools-extra/docs/ReleaseNotes.rst

@@ -362,8 +362,10 @@ Changes in existing checks
 - Improved :doc:`performance-unnecessary-copy-initialization
   <clang-tidy/checks/performance/unnecessary-copy-initialization>` check by
   detecting more cases of constant access. In particular, pointers can be
-  analyzed, se the check now handles the common patterns
+  analyzed, so the check now handles the common patterns
   `const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`.
+  Calls to mutable function where there exists a `const` overload are also
+  handled.
 
 - Improved :doc:`readability-avoid-return-with-void-value
   <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding

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

@@ -32,6 +32,9 @@ struct ExpensiveToCopyType {
 
 template <typename T>
 struct Container {
+  using reference = T&;
+  using const_reference = const T&;
+
   bool empty() const;
   const T& operator[](int) const;
   const T& operator[](int);
@@ -42,8 +45,8 @@ struct Container {
   void nonConstMethod();
   bool constMethod() const;
 
-  const T& at(int) const;
-  T& at(int);
+  reference at(int) const;
+  const_reference at(int);
 
 };
 
@@ -207,6 +210,28 @@ void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType> C)
   VarCopyConstructed.constMethod();
 }
 
+void PositiveOperatorValueParam(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.at(42);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C.at(42);
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C.at(42));
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C.at(42));
+  VarCopyConstructed.constMethod();
+}
+
 void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlias C) {
   const auto AutoAssigned = C[42];
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'