Fix traversal over CXXConstructExpr in Syntactic mode

[steveire]

Summary

Skip over elidable nodes, and ensure that intermediate
CXXFunctionalCastExpr nodes are also skipped if they are semantic.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp

80 ↗

(On Diff #272309)

While I believe the change is necessary here, it's not obvious to me what "hints" tell me this behavior is correct for the given matchers. None of the matchers look like they're going to care about implicit nodes, so how am I to know that AsIs is correct or not as a reviewer? As it stands, I sort of feel like I have to take it on faith that this change is correct and it looks a little suspicious because the code using the matcher is creating a fix-it at what now may be the location of an implicit node.

clang/lib/AST/Expr.cpp

2829–2830

Looks like the change introduced new curly braces for a single-line if.

clang/lib/AST/ParentMapContext.cpp

157

const auto *

[ymandel]

Thanks for this fix!

clang/lib/AST/Expr.cpp

2829–2830

Why is it necessary to check isElidable? I think the logic here is subtle (since the AST doesn't explicitly tag implicit expressions), so please add an explanatory comment.

clang/lib/AST/ParentMapContext.cpp

163

Same here. Please comment on the logic.

[steveire]

Rebased

[steveire]

clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp

80 ↗

(On Diff #272309)

I don't know if I was wrong about it being required before, or if it was required before, but the change to this file is not required now.

clang/lib/AST/Expr.cpp

2829–2830

The ignoringElidableConstructorCall does it this way. I'm afraid I don't know more than that.

[ymandel]

clang/lib/AST/Expr.cpp

2822

nit: just return SE?

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1236 ↗

(On Diff #300550)

is this necessary/desirable given that we reverted the default behavior? if so, should we update other tests (at least, others involving this matcher) as well?

[steveire]

Update

[aaron.ballman]

LGTM!

clang/lib/AST/Expr.cpp

auto IgnoreNoopCastsSingleStep = [&Ctx](Expr *E) {
     return E;
   };
   return IgnoreExprNodes(this, IgnoreParensSingleStep,
                          IgnoreNoopCastsSingleStep);
 }
 
 Expr *Expr::IgnoreUnlessSpelledInSource() {
   auto IgnoreImplicitConstructorSingleStep = [](Expr *E) {
+    if (auto *Cast = dyn_cast<CXXFunctionalCastExpr>(E)) {
+      auto *SE = Cast->getSubExpr();
+      if (SE->getSourceRange() == E->getSourceRange())
+        return SE;
+    }
+
     if (auto *C = dyn_cast<CXXConstructExpr>(E)) {
       auto NumArgs = C->getNumArgs();
       if (NumArgs == 1 ||
           (NumArgs > 1 && isa<CXXDefaultArgExpr>(C->getArg(1)))) {
         Expr *A = C->getArg(0);
-        if (A->getSourceRange() == E->getSourceRange() ||
-            !isa<CXXTemporaryObjectExpr>(C))
+        if (A->getSourceRange() == E->getSourceRange() || C->isElidable())
           return A;
       }
     }
     return E;
   };
   auto IgnoreImplicitMemberCallSingleStep = [](Expr *E) {
     if (auto *C = dyn_cast<CXXMemberCallExpr>(E)) {
       Expr *ExprNode = C->getImplicitObjectArgument();

clang/lib/AST/ParentMapContext.cpp

auto ShouldSkip = [](const Expr *E, const Expr *Child) {
       if (isa<ParenExpr>(E))
         return true;
 
       if (isa<ExprWithCleanups>(E))
         return true;
 
       auto SR = Child->getSourceRange();
 
+      if (const auto *C = dyn_cast<CXXFunctionalCastExpr>(E)) {
+        if (C->getSourceRange() == SR)
+          return true;
+      }
+
       if (const auto *C = dyn_cast<CXXConstructExpr>(E)) {
-        if (C->getSourceRange() == SR || !isa<CXXTemporaryObjectExpr>(C))
+        if (C->getSourceRange() == SR || C->isElidable())
           return true;
       }
 
       if (const auto *C = dyn_cast<CXXMemberCallExpr>(E)) {
         if (C->getSourceRange() == SR)
           return true;
       }
 

clang/unittests/AST/ASTTraverserTest.cpp

 
 template <unsigned alignment>
 void template_test() {
   static_assert(alignment, "");
 }
 void actual_template_test() {
   template_test<4>();
 }
+
+struct OneParamCtor {
+  explicit OneParamCtor(int);
+};
+struct TwoParamCtor {
+  explicit TwoParamCtor(int, int);
+};
+
+void varDeclCtors() {
+  {
+  auto var1 = OneParamCtor(5);
+  auto var2 = TwoParamCtor(6, 7);
+  }
+  {
+  OneParamCtor var3(5);
+  TwoParamCtor var4(6, 7);
+  }
+  int i = 0;
+  {
+  auto var5 = OneParamCtor(i);
+  auto var6 = TwoParamCtor(i, 7);
+  }
+  {
+  OneParamCtor var7(i);
+  TwoParamCtor var8(i, 7);
+  }
+}
+
 )cpp");
 
   {
     auto FN =
         ast_matchers::match(functionDecl(hasName("stringConstruct")).bind("fn"),
                             AST->getASTContext());
     EXPECT_EQ(FN.size(), 1u);
 
)cpp");
     EXPECT_EQ(dumpASTString(TK_IgnoreUnlessSpelledInSource,
                             FN[1].getNodeAs<Decl>("staticAssert")),
               R"cpp(
 StaticAssertDecl
 |-IntegerLiteral
 `-StringLiteral
 )cpp");
   }
+
+  auto varChecker = [&AST](StringRef varName, StringRef SemanticDump,
+                           StringRef SyntacticDump) {
+    auto FN = ast_matchers::match(
+        functionDecl(
+            hasName("varDeclCtors"),
+            forEachDescendant(varDecl(hasName(varName)).bind("varDeclCtor"))),
+        AST->getASTContext());
+    EXPECT_EQ(FN.size(), 1u);
+
+    EXPECT_EQ(dumpASTString(TK_AsIs, FN[0].getNodeAs<Decl>("varDeclCtor")),
+              SemanticDump);
+
+    EXPECT_EQ(dumpASTString(TK_IgnoreUnlessSpelledInSource,
+                            FN[0].getNodeAs<Decl>("varDeclCtor")),
+              SyntacticDump);
+  };
+
+  varChecker("var1",
+             R"cpp(
+VarDecl 'var1'
+`-ExprWithCleanups
+  `-CXXConstructExpr
+    `-MaterializeTemporaryExpr
+      `-CXXFunctionalCastExpr
+        `-CXXConstructExpr
+          `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var1'
+`-CXXConstructExpr
+  `-IntegerLiteral
+)cpp");
+
+  varChecker("var2",
+             R"cpp(
+VarDecl 'var2'
+`-ExprWithCleanups
+  `-CXXConstructExpr
+    `-MaterializeTemporaryExpr
+      `-CXXTemporaryObjectExpr
+        |-IntegerLiteral
+        `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var2'
+`-CXXTemporaryObjectExpr
+  |-IntegerLiteral
+  `-IntegerLiteral
+)cpp");
+
+  varChecker("var3",
+             R"cpp(
+VarDecl 'var3'
+`-CXXConstructExpr
+  `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var3'
+`-CXXConstructExpr
+  `-IntegerLiteral
+)cpp");
+
+  varChecker("var4",
+             R"cpp(
+VarDecl 'var4'
+`-CXXConstructExpr
+  |-IntegerLiteral
+  `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var4'
+`-CXXConstructExpr
+  |-IntegerLiteral
+  `-IntegerLiteral
+)cpp");
+
+  varChecker("var5",
+             R"cpp(
+VarDecl 'var5'
+`-ExprWithCleanups
+  `-CXXConstructExpr
+    `-MaterializeTemporaryExpr
+      `-CXXFunctionalCastExpr
+        `-CXXConstructExpr
+          `-ImplicitCastExpr
+            `-DeclRefExpr 'i'
+)cpp",
+             R"cpp(
+VarDecl 'var5'
+`-CXXConstructExpr
+  `-DeclRefExpr 'i'
+)cpp");
+
+  varChecker("var6",
+             R"cpp(
+VarDecl 'var6'
+`-ExprWithCleanups
+  `-CXXConstructExpr
+    `-MaterializeTemporaryExpr
+      `-CXXTemporaryObjectExpr
+        |-ImplicitCastExpr
+        | `-DeclRefExpr 'i'
+        `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var6'
+`-CXXTemporaryObjectExpr
+  |-DeclRefExpr 'i'
+  `-IntegerLiteral
+)cpp");
+
+  varChecker("var7",
+             R"cpp(
+VarDecl 'var7'
+`-CXXConstructExpr
+  `-ImplicitCastExpr
+    `-DeclRefExpr 'i'
+)cpp",
+             R"cpp(
+VarDecl 'var7'
+`-CXXConstructExpr
+  `-DeclRefExpr 'i'
+)cpp");
+
+  varChecker("var8",
+             R"cpp(
+VarDecl 'var8'
+`-CXXConstructExpr
+  |-ImplicitCastExpr
+  | `-DeclRefExpr 'i'
+  `-IntegerLiteral
+)cpp",
+             R"cpp(
+VarDecl 'var8'
+`-CXXConstructExpr
+  |-DeclRefExpr 'i'
+  `-IntegerLiteral
+)cpp");
 }
 
 TEST(Traverse, IgnoreUnlessSpelledInSourceStructs) {
   auto AST = buildASTFromCode(R"cpp(
 
 struct MyStruct {
   MyStruct();
   MyStruct(int i) {
)cpp";
   EXPECT_EQ(
       dumpASTString(TK_IgnoreUnlessSpelledInSource, getFunctionNode("func2")),
       Expected);
 
   Expected = R"cpp(
 FunctionDecl 'func3'
 `-CompoundStmt
   `-ReturnStmt
-    `-CXXFunctionalCastExpr
+    `-CXXConstructExpr
       `-IntegerLiteral
 )cpp";
   EXPECT_EQ(
       dumpASTString(TK_IgnoreUnlessSpelledInSource, getFunctionNode("func3")),
       Expected);
 
   Expected = R"cpp(
 FunctionDecl 'func4'

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

)cpp";
   EXPECT_TRUE(matches(
       Code,
       traverse(TK_AsIs,
                staticAssertDecl(has(implicitCastExpr(has(
                    substNonTypeTemplateParmExpr(has(integerLiteral())))))))));
 
   EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource,
                                      staticAssertDecl(has(integerLiteral())))));
+
+  Code = R"cpp(
+
+struct OneParamCtor {
+  explicit OneParamCtor(int);
+};
+struct TwoParamCtor {
+  explicit TwoParamCtor(int, int);
+};
+
+void varDeclCtors() {
+  {
+  auto var1 = OneParamCtor(5);
+  auto var2 = TwoParamCtor(6, 7);
+  }
+  {
+  OneParamCtor var3(5);
+  TwoParamCtor var4(6, 7);
+  }
+  int i = 0;
+  {
+  auto var5 = OneParamCtor(i);
+  auto var6 = TwoParamCtor(i, 7);
+  }
+  {
+  OneParamCtor var7(i);
+  TwoParamCtor var8(i, 7);
+  }
+}
+
+)cpp";
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_AsIs, varDecl(hasName("var1"), hasInitializer(hasDescendant(
+                                                     cxxConstructExpr()))))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_AsIs, varDecl(hasName("var2"), hasInitializer(hasDescendant(
+                                                     cxxConstructExpr()))))));
+  EXPECT_TRUE(matches(
+      Code, traverse(TK_AsIs, varDecl(hasName("var3"),
+                                      hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code, traverse(TK_AsIs, varDecl(hasName("var4"),
+                                      hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_AsIs, varDecl(hasName("var5"), hasInitializer(hasDescendant(
+                                                     cxxConstructExpr()))))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_AsIs, varDecl(hasName("var6"), hasInitializer(hasDescendant(
+                                                     cxxConstructExpr()))))));
+  EXPECT_TRUE(matches(
+      Code, traverse(TK_AsIs, varDecl(hasName("var7"),
+                                      hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code, traverse(TK_AsIs, varDecl(hasName("var8"),
+                                      hasInitializer(cxxConstructExpr())))));
+
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var1"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var2"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var3"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var4"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var5"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var6"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var7"), hasInitializer(cxxConstructExpr())))));
+  EXPECT_TRUE(matches(
+      Code,
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               varDecl(hasName("var8"), hasInitializer(cxxConstructExpr())))));
 }
 
 template <typename MatcherT>
 bool matcherTemplateWithBinding(StringRef Code, const MatcherT &M) {
   return matchAndVerifyResultTrue(
       Code, M.bind("matchedStmt"),
       std::make_unique<VerifyIdIsBoundTo<ReturnStmt>>("matchedStmt", 1));
 }
EXPECT_TRUE(matches(
       Code,
       traverse(
           TK_IgnoreUnlessSpelledInSource,
           integerLiteral(equals(42),
                          hasParent(cxxTemporaryObjectExpr(hasParent(returnStmt(
                              forFunction(functionDecl(hasName("func2"))))))))),
       langCxx20OrLater()));
 
-  EXPECT_TRUE(matches(
-      Code,
-      traverse(
-          TK_IgnoreUnlessSpelledInSource,
+  EXPECT_TRUE(
+      matches(Code,
+              traverse(TK_IgnoreUnlessSpelledInSource,
                        returnStmt(forFunction(functionDecl(hasName("func3"))),
-                     hasReturnValue(cxxFunctionalCastExpr(
-                         hasSourceExpression(integerLiteral(equals(42))))))),
+                                  hasReturnValue(cxxConstructExpr(hasArgument(
+                                      0, integerLiteral(equals(42))))))),
               langCxx20OrLater()));
 
   EXPECT_TRUE(matches(
       Code,
       traverse(
           TK_IgnoreUnlessSpelledInSource,
           integerLiteral(equals(42),
-                         hasParent(cxxFunctionalCastExpr(hasParent(returnStmt(
+                         hasParent(cxxConstructExpr(hasParent(returnStmt(
                              forFunction(functionDecl(hasName("func3"))))))))),
       langCxx20OrLater()));
 
   EXPECT_TRUE(
       matches(Code,
               traverse(TK_IgnoreUnlessSpelledInSource,
                        returnStmt(forFunction(functionDecl(hasName("func4"))),
                                   hasReturnValue(cxxTemporaryObjectExpr()))),