[clang-tidy] ExprMutationAnalyzer: construct from references. Fixes PR38888

[lebedev.ri]

Summary

I have hit this the rough way, while trying to use this in D51870.

There is no particular point in storing the pointers, and moreover
the pointers are assumed to be non-null, and that assumption is not
enforced. If they are null, it won't be able to do anything good
with them anyway.

Initially i thought about simply adding asserts() that they are
not null, but taking/storing references looks like even cleaner solution?

Fixes PR38888

[lebedev.ri]

clang-tidy/utils/ExprMutationAnalyzer.cpp

209–214

Hm, looks like i failed to rebase the patch :)
Will fix momentarily.

[lebedev.ri]

Fixed diff.

[shuaiwang]

LGTM :)

[JonasToth]

I am in favor of the change.

I feel that the findMutation... functions that take raw pointers should get the assertions though, at least the ones in the public interface. Having them for the private ones won't hurt either.

[lebedev.ri]

In D51884#1229460, @shuaiwang wrote:

LGTM :)

Thank you for the review!

In D51884#1229463, @JonasToth wrote:

I feel that the findMutation... functions that take raw pointers should get the assertions though, at least the ones in the public interface. Having them for the private ones won't hurt either.

I could do that.
But should that come with tests?
Or do we want to also take references there?

[JonasToth]

I think you dont need tests there. Its just life insurance :)

I'd stick with pointers because working with the AST always results in
pointers. It is more convenient to stay in pointerland there.

I feel that the findMutation... functions that take raw pointers should get the assertions though, at least the ones in the public interface. Having them for the private ones won't hurt either.

I could do that.
But should that come with tests?
Or do we want to also take references there?

clang-tidy/performance/ForRangeCopyCheck.cpp

bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
   // We omit the case where the loop variable is not used in the loop body. E.g.
   //
   // for (auto _ : benchmark_state) {
   // }
   //
   // Because the fix (changing to `const auto &`) will introduce an unused
   // compiler warning which can't be suppressed.
   // Since this case is very rare, it is safe to ignore it.
-  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+  if (!utils::ExprMutationAnalyzer(*ForRange.getBody(), Context)
            .isMutated(&LoopVar) &&
       !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
                                              Context)
            .empty()) {
     diag(LoopVar.getLocation(),
          "loop variable is copied but only used as const reference; consider "
          "making it a const reference")
         << utils::fixit::changeVarDeclToConst(LoopVar)
         << utils::fixit::changeVarDeclToReference(LoopVar, Context);
     return true;
   }
   return false;
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

 
 void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
 
   // Do not trigger on non-const value parameters when they are mutated either
   // within the function body or within init expression(s) when the function is
   // a ctor.
-  if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context)
+  if (utils::ExprMutationAnalyzer(*Function->getBody(), *Result.Context)
           .isMutated(Param))
     return;
   // CXXCtorInitializer might also mutate Param but they're not part of function
   // body, so check them separately here.
   if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
     for (const auto *Init : Ctor->inits()) {
-      if (utils::ExprMutationAnalyzer(Init->getInit(), Result.Context)
+      if (utils::ExprMutationAnalyzer(*Init->getInit(), *Result.Context)
               .isMutated(Param))
         return;
     }
   }
 
   const bool IsConstQualified =
       Param->getType().getCanonicalType().isConstQualified();
 

clang-tidy/utils/ExprMutationAnalyzer.h

 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// Analyzes whether any mutative operations are applied to an expression within
 /// a given statement.
 class ExprMutationAnalyzer {
 public:
-  ExprMutationAnalyzer(const Stmt *Stm, ASTContext *Context)
+  ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context)
       : Stm(Stm), Context(Context) {}
 
   bool isMutated(const Decl *Dec) { return findDeclMutation(Dec) != nullptr; }
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
   const Stmt *findMutation(const Expr *Exp);
 
 private:
   bool isUnevaluated(const Expr *Exp);
 
   const Stmt *findExprMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
   const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
   const Stmt *findDeclMutation(const Decl *Dec);
 
   const Stmt *findDirectMutation(const Expr *Exp);
   const Stmt *findMemberMutation(const Expr *Exp);
   const Stmt *findArrayElementMutation(const Expr *Exp);
   const Stmt *findCastMutation(const Expr *Exp);
   const Stmt *findRangeLoopMutation(const Expr *Exp);
   const Stmt *findReferenceMutation(const Expr *Exp);
 
-  const Stmt *const Stm;
-  ASTContext *const Context;
+  const Stmt &Stm;
+  ASTContext &Context;
   llvm::DenseMap<const Expr *, const Stmt *> Results;
 };
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H

clang-tidy/utils/ExprMutationAnalyzer.cpp

return selectFirst<Expr>(
                                   cxxTypeidExpr(
                                       unless(isPotentiallyEvaluated())),
                                   // The controlling expression of
                                   // `GenericSelectionExpr` is unevaluated.
                                   genericSelectionExpr(hasControllingExpr(
                                       hasDescendant(equalsNode(Exp)))),
                                   cxxNoexceptExpr())))))
                          .bind("expr")),
-                 *Stm, *Context)) != nullptr;
+                 Stm, Context)) != nullptr;
 }
 
 const Stmt *
 ExprMutationAnalyzer::findExprMutation(ArrayRef<BoundNodes> Matches) {
   for (const auto &Nodes : Matches) {
     if (const Stmt *S = findMutation(Nodes.getNodeAs<Expr>("expr")))
       return S;
   }
   return nullptr;
 }
 
 const Stmt *
 ExprMutationAnalyzer::findDeclMutation(ArrayRef<BoundNodes> Matches) {
   for (const auto &DeclNodes : Matches) {
     if (const Stmt *S = findDeclMutation(DeclNodes.getNodeAs<Decl>("decl")))
       return S;
   }
   return nullptr;
 }
 
 const Stmt *ExprMutationAnalyzer::findDeclMutation(const Decl *Dec) {
   const auto Refs = match(
-      findAll(declRefExpr(to(equalsNode(Dec))).bind("expr")), *Stm, *Context);
+      findAll(declRefExpr(to(equalsNode(Dec))).bind("expr")), Stm, Context);
   for (const auto &RefNodes : Refs) {
     const auto *E = RefNodes.getNodeAs<Expr>("expr");
     if (findMutation(E))
       return E;
   }
   return nullptr;
 }
 
const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp)));
 
   const auto Matches =
       match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis,
                                AsAmpersandOperand, AsPointerFromArrayDecay,
                                AsNonConstRefArg, AsLambdaRefCaptureInit,
                                AsNonConstRefReturn))
                         .bind("stmt")),
-            *Stm, *Context);
+            Stm, Context);
   return selectFirst<Stmt>("stmt", Matches);
 }
 
 const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) {
   // Check whether any member of 'Exp' is mutated.
   const auto MemberExprs =
       match(findAll(expr(anyOf(memberExpr(hasObjectExpression(equalsNode(Exp))),
                                cxxDependentScopeMemberExpr(
                                    hasObjectExpression(equalsNode(Exp)))))
                         .bind("expr")),
-            *Stm, *Context);
+            Stm, Context);
   return findExprMutation(MemberExprs);
 }
 
 const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) {
   // Check whether any element of an array is mutated.
   const auto SubscriptExprs = match(
       findAll(arraySubscriptExpr(hasBase(ignoringImpCasts(equalsNode(Exp))))
                   .bind("expr")),
-      *Stm, *Context);
+      Stm, Context);
   return findExprMutation(SubscriptExprs);
 }
 
 const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
   // If 'Exp' is casted to any non-const reference type, check the castExpr.
   const auto Casts =
       match(findAll(castExpr(hasSourceExpression(equalsNode(Exp)),
                              anyOf(explicitCastExpr(hasDestinationType(
                                        nonConstReferenceType())),
                                    implicitCastExpr(hasImplicitDestinationType(
                                        nonConstReferenceType()))))
                         .bind("expr")),
-            *Stm, *Context);
+            Stm, Context);
   return findExprMutation(Casts);
 }
 
 const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
   // If range for looping over 'Exp' with a non-const reference loop variable,
   // check all declRefExpr of the loop variable.
   const auto LoopVars =
       match(findAll(cxxForRangeStmt(
                 hasLoopVariable(
                     varDecl(hasType(nonConstReferenceType())).bind("decl")),
                 hasRangeInit(equalsNode(Exp)))),
-            *Stm, *Context);
+            Stm, Context);
   return findDeclMutation(LoopVars);
 }
 
 const Stmt *ExprMutationAnalyzer::findReferenceMutation(const Expr *Exp) {
   // If 'Exp' is bound to a non-const reference, check all declRefExpr to that.
   const auto Refs = match(
       stmt(forEachDescendant(
           varDecl(
               hasType(nonConstReferenceType()),
               hasInitializer(anyOf(equalsNode(Exp),
                                    conditionalOperator(anyOf(
                                        hasTrueExpression(equalsNode(Exp)),
                                        hasFalseExpression(equalsNode(Exp)))))),
               hasParent(declStmt().bind("stmt")),
               // Don't follow the reference in range statement, we've handled
               // that separately.
               unless(hasParent(declStmt(hasParent(
                   cxxForRangeStmt(hasRangeStmt(equalsBoundNode("stmt"))))))))
               .bind("decl"))),
-      *Stm, *Context);
+      Stm, Context);
   return findDeclMutation(Refs);
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

 
 StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
   return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
 }
 
 bool isMutated(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
   const auto *const S = selectFirst<Stmt>("stmt", Results);
   const auto *const E = selectFirst<Expr>("expr", Results);
-  return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+  return utils::ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
 }
 
 SmallVector<std::string, 1>
 mutatedBy(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
   const auto *const S = selectFirst<Stmt>("stmt", Results);
   SmallVector<std::string, 1> Chain;
-  utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+  utils::ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
   for (const auto *E = selectFirst<Expr>("expr", Results); E != nullptr;) {
     const Stmt *By = Analyzer.findMutation(E);
     std::string buffer;
     llvm::raw_string_ostream stream(buffer);
     By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
     Chain.push_back(StringRef(stream.str()).trim().str());
     E = dyn_cast<DeclRefExpr>(By);
   }