Ignore FullExpr when traversing cast sub-expressions

[kimgr]

Summary

Full-expressions are Sema-generated implicit nodes that cover
constant-expressions and expressions-with-cleanup for temporaries.

Ignore those as part of implicit-ignore, and also remove too-aggressive
IgnoreImplicit (which includes nested ImplicitCastExprs, for example) on
unpacked sub-expressions.

Add some unittests to demonstrate that RecursiveASTVisitor sees through
ConstantExpr nodes correctly.

Adjust cxx2a-consteval test to cover diagnostics for nested consteval
expressions that were previously missed.

Fixes bug #53044.

Depends on D119476

[kimgr]

@aaron.ballman This patch replaces https://reviews.llvm.org/D117391 as a potential solution for https://github.com/llvm/llvm-project/issues/53044.

I can't swear that the new diagnostics in cxx2a-consteval.cpp are standards-conforming, but I think they are no less correct than what's already there. Fingers crossed :-)

[aaron.ballman]

I haven't had the chance to review this yet (in C standards meetings again this week), but I did notice that https://reviews.llvm.org/D119095 seems to be touching on a related (or possibly the same) issue. Can you coordinate with the other patch author to make sure nobody is writing patches (or reviewing them) that conflict? Thanks!

[kimgr]

I have a local branch with both D119476 and this patch.

• Rebased on latest main (853ca5472314e109b98e46f0985f27f79e17d2bd)
• Ran ninja check-clang and ninja tools/clang/unittests/Tooling/ToolingTests && tools/clang/unittests/Tooling/ToolingTests without issue

I did some integration testing with D119095 and could not find any conflict. More detail on that differential.

[kimgr]

Gentle weekly ping

[kimgr]

@aaron.ballman Friendly Monday ping.

[aaron.ballman]

The changes here generally look good, and I'm happy to see that more of our test cases are diagnosing similar to other compilers with the changes.

clang/test/SemaCXX/cxx2a-consteval.cpp

365

Why are we dropping this test coverage?

371–372

I usually prefer line continuation characters because I think it makes the test easier to read (it's easy to miss secondary diagnostics on the same line). However, I don't insist on these changes either (but if you make them, please do similar for the other test lines you're touching).

[kimgr]

clang/test/SemaCXX/cxx2a-consteval.cpp

365

Good question, that must've been a mistake. I'll take another look.

371–372

Thanks, I wasn't aware there was support for line continuation. I agree it would benefit readability here, so I'll look into it.

[kimgr]

Address review feedback

• Restore accidentally removed test line
• Clarify diagnostic expectations

[aaron.ballman]

The changes here LGTM!

[aaron.ballman]

I've commit on your behalf in 403d7d8d7093d6637a006f8b3f75382294259d3f, and I'm going to add a release note in a separate commit as I missed that feedback during the review.

[aaron.ballman]

In D119477#3397311, @aaron.ballman wrote:

I've commit on your behalf in 403d7d8d7093d6637a006f8b3f75382294259d3f, and I'm going to add a release note in a separate commit as I missed that feedback during the review.

6b2335cace8378f499b401bcda3f6a862812539a contains the release note.

clang/lib/AST/Expr.cpp

 // Designed for use with IgnoreExprNodes.
 Expr *ignoreImplicitSemaNodes(Expr *E) {
   if (auto *Materialize = dyn_cast<MaterializeTemporaryExpr>(E))
     return Materialize->getSubExpr();
 
   if (auto *Binder = dyn_cast<CXXBindTemporaryExpr>(E))
     return Binder->getSubExpr();
 
+  if (auto *Full = dyn_cast<FullExpr>(E))
+    return Full->getSubExpr();
+
   return E;
 }
 } // namespace
 
 Expr *CastExpr::getSubExprAsWritten() {
   const Expr *SubExpr = nullptr;
 
   for (const CastExpr *E = this; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {
     SubExpr = IgnoreExprNodes(E->getSubExpr(), ignoreImplicitSemaNodes);
 
     // Conversions by constructor and conversion functions have a
     // subexpression describing the call; strip it off.
     if (E->getCastKind() == CK_ConstructorConversion) {
-      SubExpr = IgnoreExprNodes(
-          cast<CXXConstructExpr>(SubExpr->IgnoreImplicit())->getArg(0),
+      SubExpr = IgnoreExprNodes(cast<CXXConstructExpr>(SubExpr)->getArg(0),
                                 ignoreImplicitSemaNodes);
     } else if (E->getCastKind() == CK_UserDefinedConversion) {
-      SubExpr = SubExpr->IgnoreImplicit();
       assert((isa<CXXMemberCallExpr>(SubExpr) || isa<BlockExpr>(SubExpr)) &&
              "Unexpected SubExpr for CK_UserDefinedConversion.");
       if (auto *MCE = dyn_cast<CXXMemberCallExpr>(SubExpr))
         SubExpr = MCE->getImplicitObjectArgument();
     }
   }
 
   return const_cast<Expr *>(SubExpr);

clang/test/SemaCXX/cxx2a-consteval.cpp

 
 void test() {
   constexpr A a{ nullptr };
   { int k = A().ret_i(); }
   { A k = A().ret_a(); } // expected-error {{is not a constant expression}}
   // expected-note@-1 {{is not a constant expression}}
   { A k = to_lvalue_ref(A()); } // expected-error {{is not a constant expression}}
   // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
-  { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{is not a constant expression}}
-  // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
+  { A k = to_lvalue_ref(A().ret_a()); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}}
+  // expected-error@-3 {{'alloc::to_lvalue_ref' is not a constant expression}}
+  // expected-note@-4 {{reference to temporary is not a constant expression}}
+  // expected-note@-5 {{temporary created here}}
   { int k = A().ret_a().ret_i(); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{heap-allocated object is not a constant expression}}
   { int k = by_value_a(A()); }
   { int k = const_a_ref(A()); }
   { int k = const_a_ref(a); }
   { int k = rvalue_ref(A()); }
   { int k = rvalue_ref(std::move(a)); }
   { int k = const_a_ref(A().ret_a()); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{is not a constant expression}}
   { int k = const_a_ref(to_lvalue_ref(A().ret_a())); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{is not a constant expression}}
   { int k = const_a_ref(to_lvalue_ref(std::move(a))); }
   { int k = by_value_a(A().ret_a()); }
   { int k = by_value_a(to_lvalue_ref(static_cast<const A&&>(a))); }
   { int k = (A().ret_a(), A().ret_i()); }// expected-error {{is not a constant expression}}
   // expected-note@-1 {{is not a constant expression}}
   { int k = (const_a_ref(A().ret_a()), A().ret_i()); }
+  // expected-error@-1 {{'alloc::A::ret_a' is not a constant expression}}
+  // expected-note@-2 {{is not a constant expression}}
 }
 
 }
 
 namespace self_referencing {
 
 struct S {
   S* ptr = nullptr;

clang/unittests/Tooling/CastExprTest.cpp

 //===- unittest/Tooling/CastExprTest.cpp ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestVisitor.h"
 
 using namespace clang;
 
 namespace {
 
 struct CastExprVisitor : TestVisitor<CastExprVisitor> {
   std::function<void(ExplicitCastExpr *)> OnExplicitCast;
+  std::function<void(CastExpr *)> OnCast;
 
   bool VisitExplicitCastExpr(ExplicitCastExpr *Expr) {
     if (OnExplicitCast)
       OnExplicitCast(Expr);
     return true;
   }
+
+  bool VisitCastExpr(CastExpr *Expr) {
+    if (OnCast)
+      OnCast(Expr);
+    return true;
+  }
 };
 
 TEST(CastExprTest, GetSubExprAsWrittenThroughMaterializedTemporary) {
   CastExprVisitor Visitor;
   Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
     auto Sub = Expr->getSubExprAsWritten();
     EXPECT_TRUE(isa<DeclRefExpr>(Sub))
         << "Expected DeclRefExpr, but saw " << Sub->getStmtClassName();
Visitor.OnExplicitCast = [](ExplicitCastExpr *Expr) {
     EXPECT_TRUE(isa<IntegerLiteral>(Sub))
         << "Expected IntegerLiteral, but saw " << Sub->getStmtClassName();
   };
   Visitor.runOver("struct S { consteval S(int) {} };\n"
                   "S f() { return S(0); }\n",
                   CastExprVisitor::Lang_CXX2a);
 }
 
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// constructor conversions (https://github.com/llvm/llvm-project/issues/53044):
+//
+// `-ImplicitCastExpr 'X' <ConstructorConversion>
+//   `-ConstantExpr 'X'
+//     |-value: Struct
+//     `-CXXConstructExpr 'X' 'void (const char *)'
+//       `-ImplicitCastExpr 'const char *' <ArrayToPointerDecay>
+//         `-StringLiteral 'const char [7]' lvalue "foobar"
+TEST(CastExprTest, GetCtorConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+    if (Expr->getCastKind() == CK_ConstructorConversion) {
+      auto *Conv = Expr->getConversionFunction();
+      EXPECT_TRUE(isa<CXXConstructorDecl>(Conv))
+          << "Expected CXXConstructorDecl, but saw " << Conv->getDeclKindName();
+    }
+  };
+  Visitor.runOver("struct X { consteval X(const char *) {} };\n"
+                  "void f() { X x = \"foobar\"; }\n",
+                  CastExprVisitor::Lang_CXX2a);
+}
+
+// Verify that getConversionFunction looks through a ConstantExpr for implicit
+// user-defined conversions.
+//
+// `-ImplicitCastExpr 'const char *' <UserDefinedConversion>
+//   `-ConstantExpr 'const char *'
+//     |-value: LValue
+//     `-CXXMemberCallExpr 'const char *'
+//       `-MemberExpr '<bound member function type>' .operator const char *
+//         `-DeclRefExpr 'const X' lvalue Var 'x' 'const X'
+TEST(CastExprTest, GetUserDefinedConversionFunctionThroughConstantExpr) {
+  CastExprVisitor Visitor;
+  Visitor.OnCast = [](CastExpr *Expr) {
+    if (Expr->getCastKind() == CK_UserDefinedConversion) {
+      auto *Conv = Expr->getConversionFunction();
+      EXPECT_TRUE(isa<CXXMethodDecl>(Conv))
+          << "Expected CXXMethodDecl, but saw " << Conv->getDeclKindName();
+    }
+  };
+  Visitor.runOver("struct X {\n"
+                  "  consteval operator const char *() const {\n"
+                  "    return nullptr;\n"
+                  "  }\n"
+                  "};\n"
+                  "const char *f() {\n"
+                  "  constexpr X x;\n"
+                  "  return x;\n"
+                  "}\n",
+                  CastExprVisitor::Lang_CXX2a);
+}
+
 } // namespace