Fix clang-tidy patterns to adapt to newly added ExprWithCleanups nodes.

[timshen]

Summary

This is a fix for the new ExprWithCleanups introduced by clang's temporary variable lifetime marks change.

[alexfh]

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

[alexfh]

In D21243#455999, @alexfh wrote:

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

[timshen]

In D21243#455999, @alexfh wrote:

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests?

This just fixes the tests broken by D20498. I didn't look at other patterns.

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

A MaterializeTemporaryExpr now will always generate an ExprWithCleanups at full-expr entry. That is to say, there must be an ExprWithCleanups as an ancestor node of a MaterializeTemporaryExpr.

For example, the previous pattern:

fooStmt(has(
  barExpr(
    hasDescendant(
      materializeTemporaryExpr()))))

should be changed to:

fooStmt(has(
  exprWithCleanups(
    barExpr(
      hasDescendant(
        materializeTemporaryExpr())))

There are patterns around that are not explicitly checking for a MaterializeTemporaryExpr. If in the checked AST there is a MaterializeTemporaryExpr, an ignoringExprWithCleanups should be added to each of these patterns.

Does this help?

[timshen]

Updated the patch to use ignorngImplicit/Expr::IgnoreImplicit matcher.

I think there is a systematic way to do this change:
Look at every ignoringImpCasts, ignoringParenCasts, ignoringParenImpCasts, ignoringParens, and
Expr::IgnoreImpCasts, Expr::IgnoreParenCasts, Expr::IgnoreParenImpCasts, Expr::IgnoreParens to
see if they can be changed to ignoringImplicit or Expr::IgnoreImplicit (which also ignores
ExprWithCleanups).

I'd say for most of the cases ExprWithCleanups should be ignored, but I'm not sure if I'm the
right person to look at these patterns.

[alexfh]

Yes, you're probably not the right person to fix our test coverage. Thanks for fixing the stuff that broke and I hope we'll figure out what else needs to be fixed in a finite time after the change is committed ;)

LG

clang-tidy/llvm/TwineLocalCheck.cpp

void TwineLocalCheck::check(const MatchFinder::MatchResult &Result) {
   auto Diag = diag(VD->getLocation(),
                    "twine variables are prone to use-after-free bugs");
 
   // If this VarDecl has an initializer try to fix it.
   if (VD->hasInit()) {
     // Peel away implicit constructors and casts so we can see the actual type
     // of the initializer.
     const Expr *C = VD->getInit();
+
+    if (auto Expr = dyn_cast<ExprWithCleanups>(C))
+      C = Expr->getSubExpr();
+
     while (isa<CXXConstructExpr>(C))
       C = cast<CXXConstructExpr>(C)->getArg(0)->IgnoreParenImpCasts();
 
     SourceRange TypeRange =
         VD->getTypeSourceInfo()->getTypeLoc().getSourceRange();
 
     // A real Twine, turn it into a std::string.
     if (VD->getType()->getCanonicalTypeUnqualified() ==

clang-tidy/misc/DanglingHandleCheck.cpp

 void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
   // Return a local.
   Finder->addMatcher(
       returnStmt(
           // The AST contains two constructor calls:
           //   1. Value to Handle conversion.
           //   2. Handle copy construction.
           // We have to match both.
-          has(ignoringParenImpCasts(handleFrom(
+          has(ignoringExprWithCleanups(ignoringParenImpCasts(handleFrom(
               IsAHandle,
               handleFrom(IsAHandle, declRefExpr(to(varDecl(
                                         // Is function scope ...
                                         hasAutomaticStorageDuration(),
                                         // ... and it is a local array or Value.
                                         anyOf(hasType(arrayType()),
                                               hasType(recordDecl(
-                                                  unless(IsAHandle))))))))))),
+                                                  unless(IsAHandle)))))))))))),
           // Temporary fix for false positives inside lambdas.
           unless(hasAncestor(lambdaExpr())))
           .bind("bad_stmt"),
       this);
 
   // Return a temporary.
   Finder->addMatcher(
       returnStmt(

clang-tidy/modernize/LoopConvertCheck.cpp

StatementMatcher IteratorBoundMatcher =
                      declRefExpr(to(varDecl().bind(ConditionEndVarName)))),
                  ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
                  materializeTemporaryExpr(ignoringParenImpCasts(
                      expr(EndCallMatcher).bind(EndCallName)))));
 
   StatementMatcher IteratorComparisonMatcher = expr(
       ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ConditionVarName)))));
 
-  StatementMatcher OverloadedNEQMatcher =
+  StatementMatcher OverloadedNEQMatcher = ignoringExprWithCleanups(
       cxxOperatorCallExpr(hasOverloadedOperatorName("!="), argumentCountIs(2),
                           hasArgument(0, IteratorComparisonMatcher),
-                          hasArgument(1, IteratorBoundMatcher));
+                          hasArgument(1, IteratorBoundMatcher)));
 
   // This matcher tests that a declaration is a CXXRecordDecl that has an
   // overloaded operator*(). If the operator*() returns by value instead of by
   // reference then the return type is tagged with DerefByValueResultName.
   internal::Matcher<VarDecl> TestDerefReturnsByValue =
       hasType(cxxRecordDecl(hasMethod(allOf(
           hasOverloadedOperatorName("*"),
           anyOf(

clang-tidy/modernize/LoopConvertUtils.cpp

 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector<int>::iterator it;
 ///   vector<int>::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
 const Expr *digThroughConstructors(const Expr *E) {
   if (!E)
     return nullptr;
+  if (auto Cleanups = dyn_cast<ExprWithCleanups>(E))
+    E = Cleanups->getSubExpr();
   E = E->IgnoreParenImpCasts();
   if (const auto *ConstructExpr = dyn_cast<CXXConstructExpr>(E)) {
     // The initial constructor must take exactly one parameter, but base class
     // and deferred constructors can take more.
     if (ConstructExpr->getNumArgs() != 1 ||
         ConstructExpr->getConstructionKind() != CXXConstructExpr::CK_Complete)
       return nullptr;
     E = ConstructExpr->getArg(0);

clang-tidy/modernize/UseAutoCheck.cpp

 ///
 /// varDecl(hasWrittenNonListInitializer()) maches \c I and \c A but not \c B
 /// or \c C.
 AST_MATCHER(VarDecl, hasWrittenNonListInitializer) {
   const Expr *Init = Node.getAnyInitializer();
   if (!Init)
     return false;
 
+  if (const auto *E = dyn_cast<ExprWithCleanups>(Init))
+    Init = E->getSubExpr();
+
   // The following test is based on DeclPrinter::VisitVarDecl() to find if an
   // initializer is implicit or not.
   if (const auto *Construct = dyn_cast<CXXConstructExpr>(Init)) {
     return !Construct->isListInitialization() && Construct->getNumArgs() > 0 &&
            !Construct->getArg(0)->isDefaultArgument();
   }
   return Node.getInitStyle() != VarDecl::ListInit;
 }

clang-tidy/readability/RedundantStringInitCheck.cpp

const auto EmptyStringCtorExprWithTemporaries =
           cxxConstructExpr(StringConstructorExpr,
               hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)))));
 
   // Match a variable declaration with an empty string literal as initializer.
   // Examples:
   //     string foo = "";
   //     string bar("");
   Finder->addMatcher(
-      namedDecl(varDecl(hasType(cxxRecordDecl(hasName("basic_string"))),
-                        hasInitializer(
-                            expr(anyOf(EmptyStringCtorExpr,
-                                       EmptyStringCtorExprWithTemporaries))
+      namedDecl(
+          varDecl(hasType(cxxRecordDecl(hasName("basic_string"))),
+                  hasInitializer(expr(ignoringExprWithCleanups(anyOf(
+                                          EmptyStringCtorExpr,
+                                          EmptyStringCtorExprWithTemporaries)))
                                      .bind("expr"))),
-                unless(parmVarDecl()))
           .bind("decl"),
       this);
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CtorExpr = Result.Nodes.getNodeAs<Expr>("expr");
   const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl");
   diag(CtorExpr->getExprLoc(), "redundant string initialization")
       << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
                                       Decl->getName());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang