[clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.

[flx]

Summary

This change extends isOnlyUsedAsConst() to avoid false positives in cases where
a const method returns a mutable pointer or refernce and the pointed to object
is modified.

To achieve this we look at each const method or operator call and check if it
returns a mutable pointer/reference. If that's the case the call expression
itself is checked for constant use.

We also capture assignments of expressions to non-const references/pointers and
then check that the declared alias variables are used in a const-only fasion as
well.

[ymandel]

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

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

30

please expand this comment. It's not obvious (to me) what it means for a variable to be used in a "const compatible fashion".

[flx]

In D103087#2793673, @ymandel wrote:

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

That's a fair point. Is there prior art in terms of dataflow analysis in ClangTidy or LLVM I could take a look at?

In terms of measuring performance, do you have suggestions how to measure this? I can add a counter that counts the recursion depth that is reached to see how often this happens in practice.

Another idea is to not count const methods from returning mutable pointer or reference types as const access. Standard types std::vector and absl::StatusOr would not be affected by this restriction, their const accessors return const references as well.

I'll hold off on this change until I see more false positives.

[ymandel]

In D103087#2813901, @flx wrote:

In D103087#2793673, @ymandel wrote:

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

That's a fair point. Is there prior art in terms of dataflow analysis in ClangTidy or LLVM I could take a look at?

Added Dmitri to speak to prior art.

In terms of measuring performance, do you have suggestions how to measure this? I can add a counter that counts the recursion depth that is reached to see how often this happens in practice.

I would simply run clang-tidy over a reasonable size set of files and see the timing w/ and w/o this change. But, I think that clang-tidy may have some built-in perf monitoring as well (specifically, a way to display the cost of each check, or the top most costly ones).

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

 /// do not modify it.
 ///
 /// Returns ``true`` if only const methods or operators are called on the
 /// variable or the variable is a const reference or value argument to a
 /// ``callExpr()``.
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context);
 
+// Returns true if expression is only used in a const compatible fashion.
+bool isOnlyUsedAsConst(const Expr &E, const Stmt &Stmt, ASTContext &Context);
+
 /// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
 llvm::SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
 
 /// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl``.
 llvm::SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
 

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

 // Extracts all Nodes keyed by ID from Matches and inserts them into Nodes.
 template <typename Node>
 void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
                         SmallPtrSet<const Node *, 16> &Nodes) {
   for (const auto &Match : Matches)
     Nodes.insert(Match.getNodeAs<Node>(ID));
 }
 
-} // namespace
+bool hasMutableReturnType(const CallExpr &CallExpr, ASTContext &Context) {
+  auto Matches =
+      match(callExpr(callee(
+                functionDecl(returns(anyOf(matchers::isPointerToNonConst(),
+                                           matchers::isReferenceToNonConst())))
+                    .bind("functionDecl"))),
+            CallExpr, Context);
+  return !Matches.empty();
+}
 
-// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
-// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
-SmallPtrSet<const DeclRefExpr *, 16>
-constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
-                           ASTContext &Context) {
-  auto DeclRefToVar =
-      declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+// Finds all occurrences of an expression matcher `M` where the expression is
+// used in a const fashion.
+template <typename Matcher, typename Type>
+void constReferencedExprs(
+    const Matcher &M, const Stmt &Stmt, ASTContext &Context,
+    SmallPtrSet<const Type *, 16> &ConstReferencesToExpr) {
+  auto BoundExpr = M.bind("referencedExpr");
   auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
-  // Match method call expressions where the variable is referenced as the this
-  // implicit object argument and opertor call expression for member operators
-  // where the variable is the 0-th argument.
+  // Match method call expressions where the expression is referenced as the
+  // `this` implicit object argument and operator call expressions for member
+  // operators where the variable is the 0-th argument.
   auto Matches = match(
-      findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
-                         cxxOperatorCallExpr(ConstMethodCallee,
-                                             hasArgument(0, DeclRefToVar))))),
+      findAll(expr(anyOf(
+          cxxMemberCallExpr(ConstMethodCallee, on(BoundExpr)).bind("callExpr"),
+          cxxOperatorCallExpr(ConstMethodCallee, hasArgument(0, BoundExpr))
+              .bind("callExpr")))),
       Stmt, Context);
-  SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  for (const auto &Match : Matches) {
+    const auto *E = Match.template getNodeAs<Type>("referencedExpr");
+    const auto *Call = Match.template getNodeAs<CallExpr>("callExpr");
+    // Examine whether the call expression returns a mutable return type, if so
+    // make sure that the call expression is used as const as well.
+    if (!hasMutableReturnType(*Call, Context) ||
+        isOnlyUsedAsConst(*Call, Stmt, Context)) {
+      ConstReferencesToExpr.insert(E);
+    }
+  }
+
+  // The expression is used as an argument to a function that takes a value or
+  // const reference parameter.
   auto ConstReferenceOrValue =
       qualType(anyOf(matchers::isReferenceToConst(),
                      unless(anyOf(referenceType(), pointerType(),
                                   substTemplateTypeParmType()))));
   auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
       ConstReferenceOrValue,
       substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-      DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
+      BoundExpr, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+
   // References and pointers to const assignments.
-  Matches =
-      match(findAll(declStmt(
-                has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
-                            hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
+  Matches = match(findAll(declStmt(has(
+                      varDecl(hasType(qualType(matchers::isReferenceToConst())),
+                              hasInitializer(ignoringImpCasts(BoundExpr)))))),
                   Stmt, Context);
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
   Matches =
       match(findAll(declStmt(has(varDecl(
                 hasType(qualType(matchers::isPointerToConst())),
                 hasInitializer(ignoringImpCasts(unaryOperator(
-                    hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
-            Stmt, Context);
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  return DeclRefs;
+                    hasOperatorName("&"), hasUnaryOperand(BoundExpr)))))))),
+            Stmt, Context);
+  extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+
+  // Non-const references and pointers assignments.
+  Matches = match(
+      findAll(declStmt(has(varDecl(hasType(hasCanonicalType(qualType(anyOf(
+                                       matchers::isReferenceToNonConst(),
+                                       matchers::isPointerToNonConst())))),
+                                   hasInitializer(ignoringImpCasts(BoundExpr)))
+                               .bind("var")))),
+  for (const auto &Match : Matches) {
+    const auto *Var = Match.template getNodeAs<VarDecl>("var");
+    // Check that each non-const reference variable is also used as const.
+    if (isOnlyUsedAsConst(*Var, Stmt, Context)) {
+      const auto *E = Match.template getNodeAs<Type>("referencedExpr");
+      ConstReferencesToExpr.insert(E);
+    }
+  }
+  // Parent is a compound statement. This catches cases where a CallExpr that
+  // returns a mutable pointer or reference is matched, but is not used.
+  Matches =
+      match(findAll(expr(BoundExpr, hasParent(compoundStmt()))), Stmt, Context);
+  extractNodesByIdTo(Matches, "referencedExpr", ConstReferencesToExpr);
+}
+
+} // namespace
+
+SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
+                           ASTContext &Context) {
+  auto DeclRefToVar = declRefExpr(to(varDecl(equalsNode(&VarDecl))));
+  SmallPtrSet<const DeclRefExpr *, 16> ConstDeclRefExprs;
+  constReferencedExprs(DeclRefToVar, Stmt, Context, ConstDeclRefExprs);
+  return ConstDeclRefExprs;
 }
 
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context) {
   // Collect all DeclRefExprs to the loop variable and all CallExprs and
   // CXXConstructExprs where the loop variable is used as argument to a const
   // reference parameter.
   // If the difference is empty it is safe for the loop variable to be a const
   // reference.
-  auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
-  auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
-  return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
+  auto AllConstReferencesToExpr = allDeclRefExprs(Var, Stmt, Context);
+  auto ConstReferenceConstReferencesToExpr =
+      constReferenceDeclRefExprs(Var, Stmt, Context);
+  return isSetDifferenceEmpty(AllConstReferencesToExpr,
+                              ConstReferenceConstReferencesToExpr);
+}
+
+bool isOnlyUsedAsConst(const Expr &E, const Stmt &Stmt, ASTContext &Context) {
+  auto ExprMatcher = expr(equalsNode(&E));
+  auto Matches = match(findAll(ExprMatcher), Stmt, Context);
+  SmallPtrSet<const Expr *, 16> ConstReferencedExprs;
+  constReferencedExprs(ExprMatcher, Stmt, Context, ConstReferencedExprs);
+  return ConstReferencedExprs.size() == 1;
 }
 
 SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
   auto Matches = match(
       findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
       Stmt, Context);
-  SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  return DeclRefs;
+  SmallPtrSet<const DeclRefExpr *, 16> ConstReferencesToExpr;
+  extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr);
+  return ConstReferencesToExpr;
 }
 
 SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context) {
   auto Matches = match(
       decl(forEachDescendant(
           declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))),
       Decl, Context);
-  SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
-  extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  return DeclRefs;
+  SmallPtrSet<const DeclRefExpr *, 16> ConstReferencesToExpr;
+  extractNodesByIdTo(Matches, "declRef", ConstReferencesToExpr);
+  return ConstReferencesToExpr;
 }
 
 bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
                                ASTContext &Context) {
   auto UsedAsConstRefArg = forEachArgumentWithParam(
       declRefExpr(equalsNode(&DeclRef)),
       parmVarDecl(hasType(matchers::isReferenceToConst())));
   auto Matches = match(

clang-tools-extra/clang-tidy/utils/Matchers.h

 }
 
 // Returns QualType matcher for references to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
   using namespace ast_matchers;
   return referenceType(pointee(qualType(isConstQualified())));
 }
 
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToNonConst) {
+  using namespace ast_matchers;
+  return allOf(referenceType(),
+               referenceType(pointee(qualType(unless(isConstQualified())))));
+}
+
 // Returns QualType matcher for pointers to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
   using namespace ast_matchers;
   return pointerType(pointee(qualType(isConstQualified())));
 }
 
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToNonConst) {
+  using namespace ast_matchers;
+  return allOf(pointerType(),
+               pointerType(pointee(qualType(unless(isConstQualified())))));
+}
+
 // A matcher implementation that matches a list of type name regular expressions
 // against a NamedDecl. If a regular expression contains the substring "::"
 // matching will occur against the qualified name, otherwise only the typename.
 class MatchesAnyListedNameMatcher
     : public ast_matchers::internal::MatcherInterface<NamedDecl> {
 public:
   explicit MatchesAnyListedNameMatcher(llvm::ArrayRef<std::string> NameList) {
     std::transform(

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

 // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
   void nonConstMethod();
   bool constMethod() const;
+  ExpensiveToCopyType &operator*() const;
+  ExpensiveToCopyType *operator->() const;
+  ExpensiveToCopyType *get() const;
 };
 
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 
 struct WeirdCopyCtorType {
   WeirdCopyCtorType();

 void positiveCopiedFromGetterOfReferenceToConstVar() {
   ExpensiveToCopyType Orig;
   const auto &Ref = Orig.reference();
   auto UnnecessaryCopy = Ref.reference();
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'UnnecessaryCopy' is
   // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
   Orig.constMethod();
 }
+
+void negativePointerIsModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  NecessaryCopy->nonConstMethod();
+}
+
+void negativeReferenceIsModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  (*NecessaryCopy).nonConstMethod();
+}
+
+void negativePointerIsModifiedThroughConstOperatorChain() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  (*NecessaryCopy)->nonConstMethod();
+}
+
+void positivePointerIsNotModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  UnnecessaryCopy->constMethod();
+}
+
+void positiveReferenceIsNotModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  (*UnnecessaryCopy).constMethod();
+}
+
+void negativeReferenceIsAssignedToVarAndModified() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  auto &Ref = *NecessaryCopy;
+  Ref.nonConstMethod();
+}
+
+void positiveReferenceIsAssignedToVarAndNotModified() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  auto &Ref = *UnnecessaryCopy;
+  Ref.constMethod();
+}
+
+void negativePointerIsAssignedToVarAndModified() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  auto *Pointer = NecessaryCopy.get();
+  Pointer->nonConstMethod();
+}
+
+void positivePointerIsAssignedToVarAndNotModified() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  auto *Pointer = UnnecessaryCopy.get();
+  Pointer->constMethod();
+}
+
+void positiveConstMethodReturningPointer() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  // Test that a const method that returns a mutable pointer/reference that is
+  // unused counts as a const use.
+  UnnecessaryCopy.get();
+}